posix-acl: avoid -Wempty-body warning

Commit Message

Arnd Bergmann March 22, 2021, 11:38 a.m. UTC

From: Arnd Bergmann <[e-mail]>

The fallthrough comment for an ignored cmpxchg() return value
produces a harmless warning with 'make W=1':

fs/posix_acl.c: In function 'get_acl':
fs/posix_acl.c:127:36: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
  127 |                 /* fall through */ ;
      |                                    ^

Rewrite it as gcc suggests as a step towards a clean W=1 build.
On most architectures, we could just drop the if() entirely, but
in some cases this causes a different warning.

Signed-off-by: Arnd Bergmann <[e-mail]>
---
 fs/posix_acl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Christian Brauner March 22, 2021, 12:15 p.m. UTC | #1

On Mon, Mar 22, 2021 at 12:38:24PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[e-mail]>
> 
> The fallthrough comment for an ignored cmpxchg() return value
> produces a harmless warning with 'make W=1':
> 
> fs/posix_acl.c: In function 'get_acl':
> fs/posix_acl.c:127:36: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
>   127 |                 /* fall through */ ;
>       |                                    ^
> 
> Rewrite it as gcc suggests as a step towards a clean W=1 build.
> On most architectures, we could just drop the if() entirely, but
> in some cases this causes a different warning.

And you don't see the warning for the second unconditional
cmpxchg(p, sentinel, ACL_NOT_CACHED);
below?

> 
> Signed-off-by: Arnd Bergmann <[e-mail]>
> ---

In any case that should be fine,
Reviewed-by: Christian Brauner <[e-mail]>

Arnd Bergmann March 22, 2021, 1:02 p.m. UTC | #2

On Mon, Mar 22, 2021 at 1:15 PM Christian Brauner
<[e-mail]> wrote:
> On Mon, Mar 22, 2021 at 12:38:24PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[e-mail]>
> >
> > The fallthrough comment for an ignored cmpxchg() return value
> > produces a harmless warning with 'make W=1':
> >
> > fs/posix_acl.c: In function 'get_acl':
> > fs/posix_acl.c:127:36: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
> >   127 |                 /* fall through */ ;
> >       |                                    ^
> >
> > Rewrite it as gcc suggests as a step towards a clean W=1 build.
> > On most architectures, we could just drop the if() entirely, but
> > in some cases this causes a different warning.
>
> And you don't see the warning for the second unconditional
> cmpxchg(p, sentinel, ACL_NOT_CACHED);
> below?

I would have expected both to show that warning, didn't notice the other
one.  I now see that all architectures use statement expressions for cmpxchg()
and xchg() these days, after we fixed m68k, alpha and ia64, so the
changelog text here no longer makes sense.

Should I just remove the if() then?

        Arnd

Christian Brauner March 22, 2021, 1:08 p.m. UTC | #3

On Mon, Mar 22, 2021 at 02:02:54PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 22, 2021 at 1:15 PM Christian Brauner
> <[e-mail]> wrote:
> > On Mon, Mar 22, 2021 at 12:38:24PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <[e-mail]>
> > >
> > > The fallthrough comment for an ignored cmpxchg() return value
> > > produces a harmless warning with 'make W=1':
> > >
> > > fs/posix_acl.c: In function 'get_acl':
> > > fs/posix_acl.c:127:36: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
> > >   127 |                 /* fall through */ ;
> > >       |                                    ^
> > >
> > > Rewrite it as gcc suggests as a step towards a clean W=1 build.
> > > On most architectures, we could just drop the if() entirely, but
> > > in some cases this causes a different warning.
> >
> > And you don't see the warning for the second unconditional
> > cmpxchg(p, sentinel, ACL_NOT_CACHED);
> > below?
> 
> I would have expected both to show that warning, didn't notice the other
> one.  I now see that all architectures use statement expressions for cmpxchg()
> and xchg() these days, after we fixed m68k, alpha and ia64, so the
> changelog text here no longer makes sense.
> 
> Should I just remove the if() then?

I think so. It seems like the straightforward thing to do. The comment
above this cmpxchg() also explains clearly what the expectations are.
At least to me the visual hint due to the "!= ACL_NOT_CACHED" check in
the if condition doesn't provide any additional clarity.

Christian

Patch

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index f3309a7edb49..ee6b040c8b43 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -123,8 +123,9 @@  struct posix_acl *get_acl(struct inode *inode, int type)
 	 * to just call ->get_acl to fetch the ACL ourself.  (This is going to
 	 * be an unlikely race.)
 	 */
-	if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED)
-		/* fall through */ ;
+	if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) {
+		/* fall through */
+	}
 
 	/*
 	 * Normally, the ACL returned by ->get_acl will be cached.