[PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Krzysztof Wilczynski]

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

Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Al Viro]

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.

Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Matthew Wilcox]

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.

Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Al Viro]

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

Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Krzysztof Wilczynski]

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

Re: [PATCH] fs: Remove duplicated flag from VALID_OPEN_FLAGS

[Krzysztof Wilczynski]

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