| Message ID | 20200723052723.30063-1-yangx.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io/attr.c: Disallow specifying both -D and -R options for chattr command | expand |
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>
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 > > . >
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.
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
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 > > > . >
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);
-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(+)