| Message ID | 20200522133723.1091937-1-kw@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Remove duplicated flag from VALID_OPEN_FLAGS | expand |
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.
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.
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)".
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
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
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: */