io/attr.c: Disallow specifying both -D and -R options for chattr command
diff mbox series

Message ID 20200723052723.30063-1-yangx.jy@cn.fujitsu.com
State New
Headers show
Series
  • io/attr.c: Disallow specifying both -D and -R options for chattr command
Related show

Commit Message

Xiao Yang July 23, 2020, 5:27 a.m. UTC
-D and -R options are mutually exclusive actually but chattr command
doesn't check it so that always applies -D option when both of them
are specified.  For example:
------------------------------------
# mkdir testdir
# mkdir testdir/tdir
# touch testdir/tfile
# xfs_io -c "chattr -D -R +s" testdir
# xfs_io -c "lsattr -R" testdir
----s----------- testdir/tdir
---------------- testdir/tfile
----s----------- testdir
------------------------------------

Add a check to disallow the combination.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 io/attr.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Christoph Hellwig July 23, 2020, 6:08 a.m. UTC | #1
On Thu, Jul 23, 2020 at 01:27:23PM +0800, Xiao Yang wrote:
> -D and -R options are mutually exclusive actually but chattr command
> doesn't check it so that always applies -D option when both of them
> are specified.  For example:

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Xiao Yang July 23, 2020, 6:39 a.m. UTC | #2
On 2020/7/23 14:08, Christoph Hellwig wrote:
> On Thu, Jul 23, 2020 at 01:27:23PM +0800, Xiao Yang wrote:
>> -D and -R options are mutually exclusive actually but chattr command
>> doesn't check it so that always applies -D option when both of them
>> are specified.  For example:
> Looks good,
>
> Reviewed-by: Christoph Hellwig<hch@lst.de>
Hi,

Ah,  I have a question after sending the patch:
Other commands(e.g. cowextsize) including the same options seem to avoid 
the issue by accepting the last option, as below:
--------------------------------------------------------
io/cowextsize.c
141         while ((c = getopt(argc, argv, "DR")) != EOF) {
142                 switch (c) {
143                 case 'D':
144                         recurse_all = 0;
145                         recurse_dir = 1;
146                         break;
147                 case 'R':
148                         recurse_all = 1;
149                         recurse_dir = 0;
150                         break;

Test:
# xfs_io -c "cowextsize -D -R" testdir
[0] testdir/tdir
[0] testdir/tfile
[0] testdir
[root@Fedora-31 ~]# xfs_io -c "cowextsize -R -D" testdir
[0] testdir/tdir
[0] testdir
--------------------------------------------------------

Perhaps, we should use the same solution. (not sure) :-)

Thanks,
Xiao Yang
>
> .
>
Dave Chinner July 23, 2020, 10:10 p.m. UTC | #3
On Thu, Jul 23, 2020 at 02:39:06PM +0800, Xiao Yang wrote:
> On 2020/7/23 14:08, Christoph Hellwig wrote:
> > On Thu, Jul 23, 2020 at 01:27:23PM +0800, Xiao Yang wrote:
> > > -D and -R options are mutually exclusive actually but chattr command
> > > doesn't check it so that always applies -D option when both of them
> > > are specified.  For example:
> > Looks good,
> > 
> > Reviewed-by: Christoph Hellwig<hch@lst.de>
> Hi,
> 
> Ah,  I have a question after sending the patch:
> Other commands(e.g. cowextsize) including the same options seem to avoid the
> issue by accepting the last option, as below:
> --------------------------------------------------------
> io/cowextsize.c
> 141         while ((c = getopt(argc, argv, "DR")) != EOF) {
> 142                 switch (c) {
> 143                 case 'D':
> 144                         recurse_all = 0;
> 145                         recurse_dir = 1;
> 146                         break;
> 147                 case 'R':
> 148                         recurse_all = 1;
> 149                         recurse_dir = 0;
> 150                         break;
> 
> Test:
> # xfs_io -c "cowextsize -D -R" testdir
> [0] testdir/tdir
> [0] testdir/tfile
> [0] testdir
> [root@Fedora-31 ~]# xfs_io -c "cowextsize -R -D" testdir
> [0] testdir/tdir
> [0] testdir
> --------------------------------------------------------
> 
> Perhaps, we should use the same solution. (not sure) :-)

They should all operate the same way and, IMO, the order of the
parameters on the command line should not change the behaviour of
the command. Hence I think erroring out is better than what the
cowextsize code does above.

Cheers,

Dave.
Eric Sandeen July 23, 2020, 10:15 p.m. UTC | #4
On 7/22/20 11:39 PM, Xiao Yang wrote:
> On 2020/7/23 14:08, Christoph Hellwig wrote:
>> On Thu, Jul 23, 2020 at 01:27:23PM +0800, Xiao Yang wrote:
>>> -D and -R options are mutually exclusive actually but chattr command
>>> doesn't check it so that always applies -D option when both of them
>>> are specified.  For example:
>> Looks good,
>>
>> Reviewed-by: Christoph Hellwig<hch@lst.de>
> Hi,
> 
> Ah,  I have a question after sending the patch:
> Other commands(e.g. cowextsize) including the same options seem to avoid the issue by accepting the last option, as below:
> --------------------------------------------------------
> io/cowextsize.c
> 141         while ((c = getopt(argc, argv, "DR")) != EOF) {
> 142                 switch (c) {
> 143                 case 'D':
> 144                         recurse_all = 0;
> 145                         recurse_dir = 1;
> 146                         break;
> 147                 case 'R':
> 148                         recurse_all = 1;
> 149                         recurse_dir = 0;
> 150                         break;

Yep, I meant to look at this but hadn't gotten to it yet.  These should all
be consistent, and I tend to agree with Dave that explicitly conflicting
incompatible options and erroring out is better than silently accepting
the last one specified.

And indeed help specifies that they are exclusive:

        cowextsize_cmd.args = _("[-D | -R] [cowextsize]");

It'd be great if you want to send a V2 that makes the behavior (and
documentation) of any/all commands that accept [-D | -R] consistent.

Thanks,
-Eric
Xiao Yang July 24, 2020, 1:09 a.m. UTC | #5
On 2020/7/24 6:15, Eric Sandeen wrote:
> On 7/22/20 11:39 PM, Xiao Yang wrote:
>> On 2020/7/23 14:08, Christoph Hellwig wrote:
>>> On Thu, Jul 23, 2020 at 01:27:23PM +0800, Xiao Yang wrote:
>>>> -D and -R options are mutually exclusive actually but chattr command
>>>> doesn't check it so that always applies -D option when both of them
>>>> are specified.  For example:
>>> Looks good,
>>>
>>> Reviewed-by: Christoph Hellwig<hch@lst.de>
>> Hi,
>>
>> Ah,  I have a question after sending the patch:
>> Other commands(e.g. cowextsize) including the same options seem to avoid the issue by accepting the last option, as below:
>> --------------------------------------------------------
>> io/cowextsize.c
>> 141         while ((c = getopt(argc, argv, "DR")) != EOF) {
>> 142                 switch (c) {
>> 143                 case 'D':
>> 144                         recurse_all = 0;
>> 145                         recurse_dir = 1;
>> 146                         break;
>> 147                 case 'R':
>> 148                         recurse_all = 1;
>> 149                         recurse_dir = 0;
>> 150                         break;
> Yep, I meant to look at this but hadn't gotten to it yet.  These should all
> be consistent, and I tend to agree with Dave that explicitly conflicting
> incompatible options and erroring out is better than silently accepting
> the last one specified.
>
> And indeed help specifies that they are exclusive:
>
>          cowextsize_cmd.args = _("[-D | -R] [cowextsize]");
>
> It'd be great if you want to send a V2 that makes the behavior (and
> documentation) of any/all commands that accept [-D | -R] consistent.
Hi Eric, Dave

Thanks for your suggestions.
I will send v2 patch to make the behavior consistent.

Thanks,
Xiao Yang
> Thanks,
> -Eric
>
>
> .
>

Patch
diff mbox series

diff --git a/io/attr.c b/io/attr.c
index 80e28514..f82a0881 100644
--- a/io/attr.c
+++ b/io/attr.c
@@ -320,6 +320,13 @@  chattr_f(
 		}
 	}
 
+	if (recurse_all && recurse_dir) {
+		fprintf(stderr, _("%s: -R and -D options are mutually exclusive\n"),
+			progname);
+		exitcode = 1;
+		return 0;
+	}
+
 	if (recurse_all || recurse_dir) {
 		nftw(name, chattr_callback,
 			100, FTW_PHYS | FTW_MOUNT | FTW_DEPTH);