Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
@ 2020-05-22 13:37 Krzysztof Wilczynski
  2020-05-22 15:47 ` Al Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Wilczynski @ 2020-05-22 13:37 UTC (permalink / raw)
  To: Jeff Layton; +Cc: J. Bruce Fields, linux-fsdevel, Krzysztof Wilczyński

From: Krzysztof Wilczyński <kw@linux.com>

Also, remove extra tab after the FASYNC flag, and keep line under 80
characters.  This also resolves the following Coccinelle warning:

  include/linux/fcntl.h:11:13-21: duplicated argument to & or |

And following checkpatch.pl warning:

  WARNING: line over 80 characters
  #10: FILE: include/linux/fcntl.h:10:
  +	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY
        | O_TRUNC | \

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 include/linux/fcntl.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 7bcdcf4f6ab2..be3e445eba18 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -7,9 +7,9 @@
 
 /* List of all valid flags for the open/openat flags argument: */
 #define VALID_OPEN_FLAGS \
-	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
-	 O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
-	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
+	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | \
+	 O_TRUNC | O_APPEND | O_NDELAY | O_NONBLOCK | __O_SYNC | O_DSYNC | \
+	 FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
 	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
 
 /* List of all valid flags for the how->upgrade_mask argument: */
-- 
2.26.2


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

* Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
  2020-05-22 13:37 [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS Krzysztof Wilczynski
@ 2020-05-22 15:47 ` Al Viro
  2020-05-22 17:01   ` Matthew Wilcox
  2020-05-22 17:22   ` Krzysztof Wilczynski
  0 siblings, 2 replies; 6+ messages in thread
From: Al Viro @ 2020-05-22 15:47 UTC (permalink / raw)
  To: Krzysztof Wilczynski; +Cc: Jeff Layton, J. Bruce Fields, linux-fsdevel

On Fri, May 22, 2020 at 01:37:23PM +0000, Krzysztof Wilczynski wrote:
> From: Krzysztof Wilczyński <kw@linux.com>
> 
> Also, remove extra tab after the FASYNC flag, and keep line under 80
> characters.  This also resolves the following Coccinelle warning:
> 
>   include/linux/fcntl.h:11:13-21: duplicated argument to & or |

Now ask yourself what might be the reason for that "duplicated argument".  
Try to figure out what the values of those constants might depend upon.
For extra points, try to guess what has caused the divergences.

Please, post the result of your investigation in followup to this.

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

* Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
  2020-05-22 15:47 ` Al Viro
@ 2020-05-22 17:01   ` Matthew Wilcox
  2020-05-22 17:18     ` Al Viro
  2020-05-22 17:22   ` Krzysztof Wilczynski
  1 sibling, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2020-05-22 17:01 UTC (permalink / raw)
  To: Al Viro; +Cc: Krzysztof Wilczynski, Jeff Layton, J. Bruce Fields, linux-fsdevel

On Fri, May 22, 2020 at 04:47:19PM +0100, Al Viro wrote:
> On Fri, May 22, 2020 at 01:37:23PM +0000, Krzysztof Wilczynski wrote:
> > From: Krzysztof Wilczyński <kw@linux.com>
> > 
> > Also, remove extra tab after the FASYNC flag, and keep line under 80
> > characters.  This also resolves the following Coccinelle warning:
> > 
> >   include/linux/fcntl.h:11:13-21: duplicated argument to & or |
> 
> Now ask yourself what might be the reason for that "duplicated argument".  
> Try to figure out what the values of those constants might depend upon.
> For extra points, try to guess what has caused the divergences.
> 
> Please, post the result of your investigation in followup to this.

I think the patch is actually right, despite the shockingly bad changelog.
He's removed the duplicate 'O_NDELAY' and reformatted the lines.

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

* Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
  2020-05-22 17:01   ` Matthew Wilcox
@ 2020-05-22 17:18     ` Al Viro
  2020-05-22 17:32       ` Krzysztof Wilczynski
  0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2020-05-22 17:18 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Krzysztof Wilczynski, Jeff Layton, J. Bruce Fields, linux-fsdevel

On Fri, May 22, 2020 at 10:01:19AM -0700, Matthew Wilcox wrote:
> On Fri, May 22, 2020 at 04:47:19PM +0100, Al Viro wrote:
> > On Fri, May 22, 2020 at 01:37:23PM +0000, Krzysztof Wilczynski wrote:
> > > From: Krzysztof Wilczyński <kw@linux.com>
> > > 
> > > Also, remove extra tab after the FASYNC flag, and keep line under 80
> > > characters.  This also resolves the following Coccinelle warning:
> > > 
> > >   include/linux/fcntl.h:11:13-21: duplicated argument to & or |
> > 
> > Now ask yourself what might be the reason for that "duplicated argument".  
> > Try to figure out what the values of those constants might depend upon.
> > For extra points, try to guess what has caused the divergences.
> > 
> > Please, post the result of your investigation in followup to this.
> 
> I think the patch is actually right, despite the shockingly bad changelog.
> He's removed the duplicate 'O_NDELAY' and reformatted the lines.

So he has; my apologies - the obvious false duplicate in there would be
O_NDELAY vs. O_NONBLOCK and I'd misread the patch.

Commit message should've been along the lines of "O_NDELAY occurs twice
in definition of VALID_OPEN_FLAGS; get rid of the duplicate", possibly
along with "Note: O_NONBLOCK in the same expression is *not* another
duplicate - on sparc O_NONBLOCK != O_NDELAY (done that way for ABI
compatibility with Solaris back when sparc port began)".

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

* Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
  2020-05-22 15:47 ` Al Viro
  2020-05-22 17:01   ` Matthew Wilcox
@ 2020-05-22 17:22   ` Krzysztof Wilczynski
  1 sibling, 0 replies; 6+ messages in thread
From: Krzysztof Wilczynski @ 2020-05-22 17:22 UTC (permalink / raw)
  To: Al Viro; +Cc: Jeff Layton, J. Bruce Fields, linux-fsdevel

On 20-05-22 16:47:19, Al Viro wrote:

Hello Al,

Thank you for review.  This is going to be an invaluable learning for
experience me.  Also, apologies for causing you to do more work.

[...]
> Now ask yourself what might be the reason for that "duplicated argument".
> Try to figure out what the values of those constants might depend upon.
> For extra points, try to guess what has caused the divergences.
>
> Please, post the result of your investigation in followup to this.

I had a look at O_NONBLOCK and O_NDELAY, and the values of these are
platform-specific, as per:

include/uapi/asm-generic/fcntl.h:
  #define O_NONBLOCK      00004000
  #define O_NDELAY	O_NONBLOCK

arch/sparc/include/uapi/asm/fcntl.h:
  #define O_NONBLOCK  0x4000
  #if defined(__sparc__) && defined(__arch64__)
  #define O_NDELAY    0x0004
  #else
  #define O_NDELAY    (0x0004 | O_NONBLOCK)
  #endif

arch/alpha/include/uapi/asm/fcntl.h:
  #define O_NONBLOCK   00004

arch/mips/include/uapi/asm/fcntl.h:
  #define O_NONBLOCK   0x0080

For Sparc, there we handle it as a special case, for example:

linux/fs/ioctl.c ->
  ioctl_fionbio():

  #ifdef __sparc__
          /* SunOS compatibility item. */
          if (O_NONBLOCK != O_NDELAY)
                  flag |= O_NDELAY;
  #endif

There is also a comment in the fs/fcntl.c that adds clarification:

fs/fcntl.c ->
  fcntl_init():

  /*
   * Please add new bits here to ensure allocation uniqueness.
   * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
   * is defined as O_NONBLOCK on some platforms and not on others.
   */

The behaviour of O_NONBLOCK and O_NDELAY also is platform-dependent,
where O_NDELAY might just return 0 instead of returning an error and
setting errno to either EAGAIN or EWOULDBLOCK.

I also took a look at commit 80f18379a7c3 ("fs: add a VALID_OPEN_FLAGS")
that introduced the new definition.

After looking at the warning coming from Coccinelle, I had a look and
assumed that flag O_NDELAY was added to the VALID_OPEN_FLAGS twice
accidentally.

I am still unsure why we would want to keep O_NDELAY twice?  Setting the
same bits multiple would be a non-operation to the best of my knowledge.

But, I sincerely apologise for a not very clear commit message and not
mentioning the flag in the subject and explaining what has been done
better.  I will send a v2, if that is OK with you?

Again, thank you for you time!

Krzysztof

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

* Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS
  2020-05-22 17:18     ` Al Viro
@ 2020-05-22 17:32       ` Krzysztof Wilczynski
  0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Wilczynski @ 2020-05-22 17:32 UTC (permalink / raw)
  To: Al Viro; +Cc: Matthew Wilcox, Jeff Layton, J. Bruce Fields, linux-fsdevel

Hello Al and Matthew!

On 20-05-22 18:18:56, Al Viro wrote:

[...]
> > I think the patch is actually right, despite the shockingly bad changelog.
> > He's removed the duplicate 'O_NDELAY' and reformatted the lines.
> 
> So he has; my apologies - the obvious false duplicate in there would be
> O_NDELAY vs. O_NONBLOCK and I'd misread the patch.
> 
> Commit message should've been along the lines of "O_NDELAY occurs twice
> in definition of VALID_OPEN_FLAGS; get rid of the duplicate", possibly
> along with "Note: O_NONBLOCK in the same expression is *not* another
> duplicate - on sparc O_NONBLOCK != O_NDELAY (done that way for ABI
> compatibility with Solaris back when sparc port began)".

Apologies.  The v2 is coming and will have proper commit message along
with subject that explains what the intended change is, etc.

Krzysztof

^ 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-05-22 13:37 [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS Krzysztof Wilczynski
2020-05-22 15:47 ` Al Viro
2020-05-22 17:01   ` Matthew Wilcox
2020-05-22 17:18     ` Al Viro
2020-05-22 17:32       ` Krzysztof Wilczynski
2020-05-22 17:22   ` Krzysztof Wilczynski

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/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-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

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


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