Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
@ 2020-07-23  5:27 Xiao Yang
  2020-07-23  6:08 ` Christoph Hellwig
  0 siblings, 1 reply; 6+ messages in thread
From: Xiao Yang @ 2020-07-23  5:27 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Xiao Yang

-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(+)

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);
-- 
2.21.0




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
  2020-07-23  5:27 [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command Xiao Yang
@ 2020-07-23  6:08 ` Christoph Hellwig
  2020-07-23  6:39   ` Xiao Yang
  0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2020-07-23  6:08 UTC (permalink / raw)
  To: Xiao Yang; +Cc: sandeen, darrick.wong, linux-xfs

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
  2020-07-23  6:08 ` Christoph Hellwig
@ 2020-07-23  6:39   ` Xiao Yang
  2020-07-23 22:10     ` Dave Chinner
  2020-07-23 22:15     ` Eric Sandeen
  0 siblings, 2 replies; 6+ messages in thread
From: Xiao Yang @ 2020-07-23  6:39 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, darrick.wong, linux-xfs

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
>
> .
>




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
  2020-07-23  6:39   ` Xiao Yang
@ 2020-07-23 22:10     ` Dave Chinner
  2020-07-23 22:15     ` Eric Sandeen
  1 sibling, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2020-07-23 22:10 UTC (permalink / raw)
  To: Xiao Yang; +Cc: Christoph Hellwig, sandeen, darrick.wong, linux-xfs

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.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
  2020-07-23  6:39   ` Xiao Yang
  2020-07-23 22:10     ` Dave Chinner
@ 2020-07-23 22:15     ` Eric Sandeen
  2020-07-24  1:09       ` Xiao Yang
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2020-07-23 22:15 UTC (permalink / raw)
  To: Xiao Yang, Christoph Hellwig; +Cc: darrick.wong, linux-xfs

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command
  2020-07-23 22:15     ` Eric Sandeen
@ 2020-07-24  1:09       ` Xiao Yang
  0 siblings, 0 replies; 6+ messages in thread
From: Xiao Yang @ 2020-07-24  1:09 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Christoph Hellwig, darrick.wong, linux-xfs

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
>
>
> .
>




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23  5:27 [PATCH] io/attr.c: Disallow specifying both -D and -R options for chattr command Xiao Yang
2020-07-23  6:08 ` Christoph Hellwig
2020-07-23  6:39   ` Xiao Yang
2020-07-23 22:10     ` Dave Chinner
2020-07-23 22:15     ` Eric Sandeen
2020-07-24  1:09       ` Xiao Yang

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git