From e3edd8c43e5808c19a37d8d74a311f4d9240665a Mon Sep 17 00:00:00 2001 From: Cheyenne Wills Date: Mon, 7 Mar 2022 18:35:03 -0700 Subject: [PATCH] LINUX: Use bitwise & for f_flags test This code is clearly supposed to be masking f_flags with O_ACCMODE, and so should be using a bitwise &, not the boolean &&. Clang complains about this, causing a warning (which breaks the build with --enable-checking): error: use of logical '&&' with constant operand [-Werror, -Wconstant-logical-operand] } else if ((file->f_flags && O_ACCMODE) != O_WRONLY) { ^ ~~~~~~~~~ .../osi_vnodeops.c:3192:28: note: use '&' for a bitwise operation For the current code without this commit the behavior of this check is as follows: When f_flags is: The check is: ==================== ============= O_RDONLY True (as expected) O_WRONLY False (as expected) O_RDWR False (incorrect) has some non-O_ACCMODE False (incorrect) bit is set The incorrect check doesn't cause any problems for overall correctness, but it does mean that in those cases afs_linux_fillpage will not be called and that partially-written pages will be left out of date and any reader will need to fill the page again. Fix this by using the bitwise &. There is also an out of date link to the Linux documentation. Update the comment to point to the current documentation that is in the Linux source tree. Reviewed-on: https://gerrit.openafs.org/14903 Tested-by: BuildBot Reviewed-by: Andrew Deason Reviewed-by: Michael Meffie Reviewed-by: Benjamin Kaduk (cherry picked from commit 8d9545008240d7f285b140503e432f53f41e0724) Change-Id: Iae459ffe87df9a21a90587c02a1ee2c20b360d33 Reviewed-on: https://gerrit.openafs.org/15129 Tested-by: BuildBot Reviewed-by: Cheyenne Wills Reviewed-by: Michael Meffie Reviewed-by: Stephan Wiesand --- src/afs/LINUX/osi_vnodeops.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index b1b64f5239..621e4da38b 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -3438,9 +3438,11 @@ afs_linux_prepare_write(struct file *file, struct page *page, unsigned from, unsigned to) { - /* http://kerneltrap.org/node/4941 details the expected behaviour of - * prepare_write. Essentially, if the page exists within the file, - * and is not being fully written, then we should populate it. + /* + * Linux's Documentation/filesystems/vfs.txt (.rst) details the expected + * behaviour of prepare_write (prior to 2.6.28) and write_begin (2.6.28). + * Essentially, if the page exists within the file, and is not being fully + * written, then we should populate it. */ if (!PageUptodate(page)) { @@ -3457,7 +3459,7 @@ afs_linux_prepare_write(struct file *file, struct page *page, unsigned from, SetPageChecked(page); /* Is the page readable, if it's wronly, we don't care, because we're * not actually going to read from it ... */ - } else if ((file->f_flags && O_ACCMODE) != O_WRONLY) { + } else if ((file->f_flags & O_ACCMODE) != O_WRONLY) { /* We don't care if fillpage fails, because if it does the page * won't be marked as up to date */