diff mbox series

[v3] nfs: remove incorrect fallthrough label

Message ID 20200917214545.199463-1-ndesaulniers@google.com
State New
Headers show
Series [v3] nfs: remove incorrect fallthrough label | expand

Commit Message

Nick Desaulniers Sept. 17, 2020, 9:45 p.m. UTC
There is no case after the default from which to fallthrough to. Clang
will error in this case (unhelpfully without context, see link below)
and GCC will with -Wswitch-unreachable.

The previous commit should have just replaced the comment with a break
statement.

If we consider implicit fallthrough to be a design mistake of C, then
all case statements should be terminated with one of the following
statements:
* break
* continue
* return
* fallthrough
* goto
* (call of function with __attribute__(__noreturn__))

Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro")
Link: https://bugs.llvm.org/show_bug.cgi?id=47539
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v3:
* update the commit message as per Joe.
* collect tags.

Changes v2:
* add break rather than no terminating statement as per Joe.
* add Joe's suggested by tag.
* add blurb about acceptable terminal statements.

 fs/nfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nick Desaulniers Sept. 24, 2020, 5:19 p.m. UTC | #1
Hello maintainers,
Would you mind please picking up this patch?  KernelCI has been
erroring for over a week without it.

On Thu, Sep 17, 2020 at 2:45 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> There is no case after the default from which to fallthrough to. Clang
> will error in this case (unhelpfully without context, see link below)
> and GCC will with -Wswitch-unreachable.
>
> The previous commit should have just replaced the comment with a break
> statement.
>
> If we consider implicit fallthrough to be a design mistake of C, then
> all case statements should be terminated with one of the following
> statements:
> * break
> * continue
> * return
> * fallthrough
> * goto
> * (call of function with __attribute__(__noreturn__))
>
> Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro")
> Link: https://bugs.llvm.org/show_bug.cgi?id=47539
> Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes v3:
> * update the commit message as per Joe.
> * collect tags.
>
> Changes v2:
> * add break rather than no terminating statement as per Joe.
> * add Joe's suggested by tag.
> * add blurb about acceptable terminal statements.
>
>  fs/nfs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index d20326ee0475..eb2401079b04 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -889,7 +889,7 @@ static struct nfs_server *nfs_try_mount_request(struct fs_context *fc)
>                 default:
>                         if (rpcauth_get_gssinfo(flavor, &info) != 0)
>                                 continue;
> -                       fallthrough;
> +                       break;
>                 }
>                 dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
>                 ctx->selected_flavor = flavor;
> --
> 2.28.0.681.g6f77f65b4e-goog
>
Joe Perches Sept. 24, 2020, 5:40 p.m. UTC | #2
On Thu, 2020-09-24 at 10:19 -0700, Nick Desaulniers wrote:
> Hello maintainers,
> Would you mind please picking up this patch?  KernelCI has been
> erroring for over a week without it.

As it's trivial and necessary, why not go through Linus directly?

https://lore.kernel.org/patchwork/patch/1307549/

From: Nick Desaulniers <ndesaulniers@google.com>

There is no case after the default from which to fallthrough to. Clang
will error in this case (unhelpfully without context, see link below)
and GCC will with -Wswitch-unreachable.

The previous commit should have just replaced the comment with a break
statement.

If we consider implicit fallthrough to be a design mistake of C, then
all case statements should be terminated with one of the following
statements:
* break
* continue
* return
* fallthrough
* goto
* (call of function with __attribute__(__noreturn__))

Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro")
Link: https://bugs.llvm.org/show_bug.cgi?id=47539
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v3:
* update the commit message as per Joe.
* collect tags.

Changes v2:
* add break rather than no terminating statement as per Joe.
* add Joe's suggested by tag.
* add blurb about acceptable terminal statements.

 fs/nfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d20326ee0475..eb2401079b04 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -889,7 +889,7 @@  static struct nfs_server *nfs_try_mount_request(struct fs_context *fc)
 		default:
 			if (rpcauth_get_gssinfo(flavor, &info) != 0)
 				continue;
-			fallthrough;
+			break;
 		}
 		dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
 		ctx->selected_flavor = flavor;
Joe Perches Sept. 24, 2020, 6:07 p.m. UTC | #3
On Thu, 2020-09-24 at 10:40 -0700, Joe Perches wrote:
> On Thu, 2020-09-24 at 10:19 -0700, Nick Desaulniers wrote:
> > Hello maintainers,
> > Would you mind please picking up this patch?  KernelCI has been
> > erroring for over a week without it.
> 
> As it's trivial and necessary, why not go through Linus directly?
[]
> Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro")

Real reason why not:

the commit to be fixed is not in Linus' tree.

> https://lore.kernel.org/patchwork/patch/1307549/
Gustavo A. R. Silva Sept. 24, 2020, 6:08 p.m. UTC | #4
On Thu, Sep 24, 2020 at 10:19:08AM -0700, Nick Desaulniers wrote:
> Hello maintainers,
> Would you mind please picking up this patch?  KernelCI has been
> erroring for over a week without it.
>

I can add this to my -next tree and queue it up for the next merge window.

Thanks
--
Gustavo
Anna Schumaker Sept. 24, 2020, 6:11 p.m. UTC | #5
On Thu, Sep 24, 2020 at 2:08 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-09-24 at 10:40 -0700, Joe Perches wrote:
> > On Thu, 2020-09-24 at 10:19 -0700, Nick Desaulniers wrote:
> > > Hello maintainers,
> > > Would you mind please picking up this patch?  KernelCI has been
> > > erroring for over a week without it.
> >
> > As it's trivial and necessary, why not go through Linus directly?
> []
> > Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro")
>
> Real reason why not:
>

I'm planning to take this patch through the NFS tree for 5.10 (along
with the patch that apparently causes the problem). I didn't think it
was urgent so I haven't gotten around to pushing it out yet, but I'll
do so in the next few hours.

Anna

> the commit to be fixed is not in Linus' tree.
>
> > https://lore.kernel.org/patchwork/patch/1307549/
>
>
Mark Brown Sept. 25, 2020, 11:27 a.m. UTC | #6
On Thu, Sep 24, 2020 at 02:11:59PM -0400, Anna Schumaker wrote:
> On Thu, Sep 24, 2020 at 2:08 PM Joe Perches <joe@perches.com> wrote:

> > Real reason why not:

> I'm planning to take this patch through the NFS tree for 5.10 (along
> with the patch that apparently causes the problem). I didn't think it
> was urgent so I haven't gotten around to pushing it out yet, but I'll
> do so in the next few hours.

FWIW NFS is quite widely used by CI systems so any build breaks with it
in -next have a pretty big knock on effect on testing, even beyond the
distruption people working on the build test side of things.
diff mbox series

Patch

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d20326ee0475..eb2401079b04 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -889,7 +889,7 @@  static struct nfs_server *nfs_try_mount_request(struct fs_context *fc)
 		default:
 			if (rpcauth_get_gssinfo(flavor, &info) != 0)
 				continue;
-			fallthrough;
+			break;
 		}
 		dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
 		ctx->selected_flavor = flavor;