In preparation to enable -Wimplicit-fallthrough for Clang, fix
the following warnings by replacing /* fall through */ comments,
and its variants, with the new pseudo-keyword macro fallthrough:
fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
Notice that Clang doesn't recognize /* fall through */ comments as
implicit fall-through markings, so in order to globally enable
-Wimplicit-fallthrough for Clang, these comments need to be
replaced with fallthrough; in the whole codebase.
Link: https://github.com/KSPP/linux/issues/115
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
fs/xfs/libxfs/xfs_ag_resv.c | 4 ++--
fs/xfs/libxfs/xfs_alloc.c | 2 +-
fs/xfs/libxfs/xfs_da_btree.c | 2 +-
fs/xfs/scrub/bmap.c | 2 +-
fs/xfs/scrub/btree.c | 2 +-
fs/xfs/scrub/common.c | 6 +++---
fs/xfs/scrub/dabtree.c | 2 +-
fs/xfs/scrub/repair.c | 2 +-
fs/xfs/xfs_bmap_util.c | 2 +-
fs/xfs/xfs_export.c | 4 ++--
fs/xfs/xfs_file.c | 2 +-
fs/xfs/xfs_inode.c | 2 +-
fs/xfs/xfs_ioctl.c | 4 ++--
fs/xfs/xfs_iomap.c | 2 +-
fs/xfs/xfs_trans_buf.c | 2 +-
15 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index 6c5f8d10589c..8c3c99a9bf83 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -342,7 +342,7 @@ xfs_ag_resv_alloc_extent(
break;
default:
ASSERT(0);
- /* fall through */
+ fallthrough;
case XFS_AG_RESV_NONE:
field = args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
XFS_TRANS_SB_FDBLOCKS;
@@ -384,7 +384,7 @@ xfs_ag_resv_free_extent(
break;
default:
ASSERT(0);
- /* fall through */
+ fallthrough;
case XFS_AG_RESV_NONE:
xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, (int64_t)len);
return;
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index aaa19101bb2a..9eabdeeec492 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3163,7 +3163,7 @@ xfs_alloc_vextent(
}
args->agbno = XFS_FSB_TO_AGBNO(mp, args->fsbno);
args->type = XFS_ALLOCTYPE_NEAR_BNO;
- /* FALLTHROUGH */
+ fallthrough;
case XFS_ALLOCTYPE_FIRST_AG:
/*
* Rotate through the allocation groups looking for a winner.
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 83ac9771bfb5..747ec77912c3 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -282,7 +282,7 @@ xfs_da3_node_read_verify(
__this_address);
break;
}
- /* fall through */
+ fallthrough;
case XFS_DA_NODE_MAGIC:
fa = xfs_da3_node_verify(bp);
if (fa)
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index b5ebf1d1b4db..77d5c4a0f09f 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -271,7 +271,7 @@ xchk_bmap_iextent_xref(
case XFS_DATA_FORK:
if (xfs_is_reflink_inode(info->sc->ip))
break;
- /* fall through */
+ fallthrough;
case XFS_ATTR_FORK:
xchk_xref_is_not_shared(info->sc, agbno,
irec->br_blockcount);
diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
index a94bd8122c60..bd1172358964 100644
--- a/fs/xfs/scrub/btree.c
+++ b/fs/xfs/scrub/btree.c
@@ -44,7 +44,7 @@ __xchk_btree_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ fallthrough;
default:
if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
trace_xchk_ifork_btree_op_error(sc, cur, level,
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index aa874607618a..ce9a44ea6948 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -81,7 +81,7 @@ __xchk_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ fallthrough;
default:
trace_xchk_op_error(sc, agno, bno, *error,
ret_ip);
@@ -134,7 +134,7 @@ __xchk_fblock_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ fallthrough;
default:
trace_xchk_file_op_error(sc, whichfork, offset, *error,
ret_ip);
@@ -694,7 +694,7 @@ xchk_get_inode(
if (error)
return -ENOENT;
error = -EFSCORRUPTED;
- /* fall through */
+ fallthrough;
default:
trace_xchk_op_error(sc,
XFS_INO_TO_AGNO(mp, sc->sm->sm_ino),
diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
index 653f3280e1c1..9f0dbb47c82c 100644
--- a/fs/xfs/scrub/dabtree.c
+++ b/fs/xfs/scrub/dabtree.c
@@ -47,7 +47,7 @@ xchk_da_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
*error = 0;
- /* fall through */
+ fallthrough;
default:
trace_xchk_file_op_error(sc, ds->dargs.whichfork,
xfs_dir2_da_to_db(ds->dargs.geo,
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index c2857d854c83..b8202dd08939 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -947,7 +947,7 @@ xrep_ino_dqattach(
xrep_force_quotacheck(sc, XFS_DQTYPE_GROUP);
if (XFS_IS_PQUOTA_ON(sc->mp) && !sc->ip->i_pdquot)
xrep_force_quotacheck(sc, XFS_DQTYPE_PROJ);
- /* fall through */
+ fallthrough;
case -ESRCH:
error = 0;
break;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index a5e9d7d34023..cc628475f9b6 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -242,7 +242,7 @@ xfs_bmap_count_blocks(
*/
*count += btblocks - 1;
- /* fall through */
+ fallthrough;
case XFS_DINODE_FMT_EXTENTS:
*nextents = xfs_bmap_count_leaves(ifp, count);
break;
diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
index 465fd9e048d4..1da59bdff245 100644
--- a/fs/xfs/xfs_export.c
+++ b/fs/xfs/xfs_export.c
@@ -84,7 +84,7 @@ xfs_fs_encode_fh(
case FILEID_INO32_GEN_PARENT:
fid->i32.parent_ino = XFS_I(parent)->i_ino;
fid->i32.parent_gen = parent->i_generation;
- /*FALLTHRU*/
+ fallthrough;
case FILEID_INO32_GEN:
fid->i32.ino = XFS_I(inode)->i_ino;
fid->i32.gen = inode->i_generation;
@@ -92,7 +92,7 @@ xfs_fs_encode_fh(
case FILEID_INO32_GEN_PARENT | XFS_FILEID_TYPE_64FLAG:
fid64->parent_ino = XFS_I(parent)->i_ino;
fid64->parent_gen = parent->i_generation;
- /*FALLTHRU*/
+ fallthrough;
case FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG:
fid64->ino = XFS_I(inode)->i_ino;
fid64->gen = inode->i_generation;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 396ef36dcd0a..3c0749ab9e40 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -863,7 +863,7 @@ xfs_break_layouts(
error = xfs_break_dax_layouts(inode, &retry);
if (error || retry)
break;
- /* fall through */
+ fallthrough;
case BREAK_WRITE:
error = xfs_break_leased_layouts(inode, iolock, &retry);
break;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0369eb22c1bb..f2846997c3a8 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -848,7 +848,7 @@ xfs_init_new_inode(
xfs_inode_inherit_flags(ip, pip);
if (pip && (pip->i_diflags2 & XFS_DIFLAG2_ANY))
xfs_inode_inherit_flags2(ip, pip);
- /* FALLTHROUGH */
+ fallthrough;
case S_IFLNK:
ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
ip->i_df.if_bytes = 0;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 3925bfcb2365..c4dc6c72ac37 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -558,7 +558,7 @@ xfs_ioc_attrmulti_one(
case ATTR_OP_REMOVE:
value = NULL;
*len = 0;
- /* fall through */
+ fallthrough;
case ATTR_OP_SET:
error = mnt_want_write_file(parfilp);
if (error)
@@ -1544,7 +1544,7 @@ xfs_ioc_getbmap(
switch (cmd) {
case XFS_IOC_GETBMAPA:
bmx.bmv_iflags = BMV_IF_ATTRFORK;
- /*FALLTHRU*/
+ fallthrough;
case XFS_IOC_GETBMAP:
/* struct getbmap is a strict subset of struct getbmapx. */
recsize = sizeof(struct getbmap);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d154f42e2dc6..d8cd2583dedb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1036,7 +1036,7 @@ xfs_buffered_write_iomap_begin(
prealloc_blocks = 0;
goto retry;
}
- /*FALLTHRU*/
+ fallthrough;
default:
goto out_unlock;
}
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 9aced0a00003..d11d032da0b4 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -294,7 +294,7 @@ xfs_trans_read_buf_map(
default:
if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
- /* fall through */
+ fallthrough;
case -ENOMEM:
case -EAGAIN:
return error;
--
2.27.0
On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> the following warnings by replacing /* fall through */ comments,
> and its variants, with the new pseudo-keyword macro fallthrough:
>
> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>
> Notice that Clang doesn't recognize /* fall through */ comments as
> implicit fall-through markings, so in order to globally enable
> -Wimplicit-fallthrough for Clang, these comments need to be
> replaced with fallthrough; in the whole codebase.
>
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
I've already NAKd this twice, so I guess I'll NAK it a third time.
--D
> ---
> fs/xfs/libxfs/xfs_ag_resv.c | 4 ++--
> fs/xfs/libxfs/xfs_alloc.c | 2 +-
> fs/xfs/libxfs/xfs_da_btree.c | 2 +-
> fs/xfs/scrub/bmap.c | 2 +-
> fs/xfs/scrub/btree.c | 2 +-
> fs/xfs/scrub/common.c | 6 +++---
> fs/xfs/scrub/dabtree.c | 2 +-
> fs/xfs/scrub/repair.c | 2 +-
> fs/xfs/xfs_bmap_util.c | 2 +-
> fs/xfs/xfs_export.c | 4 ++--
> fs/xfs/xfs_file.c | 2 +-
> fs/xfs/xfs_inode.c | 2 +-
> fs/xfs/xfs_ioctl.c | 4 ++--
> fs/xfs/xfs_iomap.c | 2 +-
> fs/xfs/xfs_trans_buf.c | 2 +-
> 15 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 6c5f8d10589c..8c3c99a9bf83 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -342,7 +342,7 @@ xfs_ag_resv_alloc_extent(
> break;
> default:
> ASSERT(0);
> - /* fall through */
> + fallthrough;
> case XFS_AG_RESV_NONE:
> field = args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
> XFS_TRANS_SB_FDBLOCKS;
> @@ -384,7 +384,7 @@ xfs_ag_resv_free_extent(
> break;
> default:
> ASSERT(0);
> - /* fall through */
> + fallthrough;
> case XFS_AG_RESV_NONE:
> xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, (int64_t)len);
> return;
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index aaa19101bb2a..9eabdeeec492 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3163,7 +3163,7 @@ xfs_alloc_vextent(
> }
> args->agbno = XFS_FSB_TO_AGBNO(mp, args->fsbno);
> args->type = XFS_ALLOCTYPE_NEAR_BNO;
> - /* FALLTHROUGH */
> + fallthrough;
> case XFS_ALLOCTYPE_FIRST_AG:
> /*
> * Rotate through the allocation groups looking for a winner.
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 83ac9771bfb5..747ec77912c3 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -282,7 +282,7 @@ xfs_da3_node_read_verify(
> __this_address);
> break;
> }
> - /* fall through */
> + fallthrough;
> case XFS_DA_NODE_MAGIC:
> fa = xfs_da3_node_verify(bp);
> if (fa)
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index b5ebf1d1b4db..77d5c4a0f09f 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -271,7 +271,7 @@ xchk_bmap_iextent_xref(
> case XFS_DATA_FORK:
> if (xfs_is_reflink_inode(info->sc->ip))
> break;
> - /* fall through */
> + fallthrough;
> case XFS_ATTR_FORK:
> xchk_xref_is_not_shared(info->sc, agbno,
> irec->br_blockcount);
> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> index a94bd8122c60..bd1172358964 100644
> --- a/fs/xfs/scrub/btree.c
> +++ b/fs/xfs/scrub/btree.c
> @@ -44,7 +44,7 @@ __xchk_btree_process_error(
> /* Note the badness but don't abort. */
> sc->sm->sm_flags |= errflag;
> *error = 0;
> - /* fall through */
> + fallthrough;
> default:
> if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
> trace_xchk_ifork_btree_op_error(sc, cur, level,
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index aa874607618a..ce9a44ea6948 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -81,7 +81,7 @@ __xchk_process_error(
> /* Note the badness but don't abort. */
> sc->sm->sm_flags |= errflag;
> *error = 0;
> - /* fall through */
> + fallthrough;
> default:
> trace_xchk_op_error(sc, agno, bno, *error,
> ret_ip);
> @@ -134,7 +134,7 @@ __xchk_fblock_process_error(
> /* Note the badness but don't abort. */
> sc->sm->sm_flags |= errflag;
> *error = 0;
> - /* fall through */
> + fallthrough;
> default:
> trace_xchk_file_op_error(sc, whichfork, offset, *error,
> ret_ip);
> @@ -694,7 +694,7 @@ xchk_get_inode(
> if (error)
> return -ENOENT;
> error = -EFSCORRUPTED;
> - /* fall through */
> + fallthrough;
> default:
> trace_xchk_op_error(sc,
> XFS_INO_TO_AGNO(mp, sc->sm->sm_ino),
> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
> index 653f3280e1c1..9f0dbb47c82c 100644
> --- a/fs/xfs/scrub/dabtree.c
> +++ b/fs/xfs/scrub/dabtree.c
> @@ -47,7 +47,7 @@ xchk_da_process_error(
> /* Note the badness but don't abort. */
> sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> *error = 0;
> - /* fall through */
> + fallthrough;
> default:
> trace_xchk_file_op_error(sc, ds->dargs.whichfork,
> xfs_dir2_da_to_db(ds->dargs.geo,
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index c2857d854c83..b8202dd08939 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -947,7 +947,7 @@ xrep_ino_dqattach(
> xrep_force_quotacheck(sc, XFS_DQTYPE_GROUP);
> if (XFS_IS_PQUOTA_ON(sc->mp) && !sc->ip->i_pdquot)
> xrep_force_quotacheck(sc, XFS_DQTYPE_PROJ);
> - /* fall through */
> + fallthrough;
> case -ESRCH:
> error = 0;
> break;
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index a5e9d7d34023..cc628475f9b6 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -242,7 +242,7 @@ xfs_bmap_count_blocks(
> */
> *count += btblocks - 1;
>
> - /* fall through */
> + fallthrough;
> case XFS_DINODE_FMT_EXTENTS:
> *nextents = xfs_bmap_count_leaves(ifp, count);
> break;
> diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> index 465fd9e048d4..1da59bdff245 100644
> --- a/fs/xfs/xfs_export.c
> +++ b/fs/xfs/xfs_export.c
> @@ -84,7 +84,7 @@ xfs_fs_encode_fh(
> case FILEID_INO32_GEN_PARENT:
> fid->i32.parent_ino = XFS_I(parent)->i_ino;
> fid->i32.parent_gen = parent->i_generation;
> - /*FALLTHRU*/
> + fallthrough;
> case FILEID_INO32_GEN:
> fid->i32.ino = XFS_I(inode)->i_ino;
> fid->i32.gen = inode->i_generation;
> @@ -92,7 +92,7 @@ xfs_fs_encode_fh(
> case FILEID_INO32_GEN_PARENT | XFS_FILEID_TYPE_64FLAG:
> fid64->parent_ino = XFS_I(parent)->i_ino;
> fid64->parent_gen = parent->i_generation;
> - /*FALLTHRU*/
> + fallthrough;
> case FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG:
> fid64->ino = XFS_I(inode)->i_ino;
> fid64->gen = inode->i_generation;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 396ef36dcd0a..3c0749ab9e40 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -863,7 +863,7 @@ xfs_break_layouts(
> error = xfs_break_dax_layouts(inode, &retry);
> if (error || retry)
> break;
> - /* fall through */
> + fallthrough;
> case BREAK_WRITE:
> error = xfs_break_leased_layouts(inode, iolock, &retry);
> break;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 0369eb22c1bb..f2846997c3a8 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -848,7 +848,7 @@ xfs_init_new_inode(
> xfs_inode_inherit_flags(ip, pip);
> if (pip && (pip->i_diflags2 & XFS_DIFLAG2_ANY))
> xfs_inode_inherit_flags2(ip, pip);
> - /* FALLTHROUGH */
> + fallthrough;
> case S_IFLNK:
> ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
> ip->i_df.if_bytes = 0;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 3925bfcb2365..c4dc6c72ac37 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -558,7 +558,7 @@ xfs_ioc_attrmulti_one(
> case ATTR_OP_REMOVE:
> value = NULL;
> *len = 0;
> - /* fall through */
> + fallthrough;
> case ATTR_OP_SET:
> error = mnt_want_write_file(parfilp);
> if (error)
> @@ -1544,7 +1544,7 @@ xfs_ioc_getbmap(
> switch (cmd) {
> case XFS_IOC_GETBMAPA:
> bmx.bmv_iflags = BMV_IF_ATTRFORK;
> - /*FALLTHRU*/
> + fallthrough;
> case XFS_IOC_GETBMAP:
> /* struct getbmap is a strict subset of struct getbmapx. */
> recsize = sizeof(struct getbmap);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index d154f42e2dc6..d8cd2583dedb 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1036,7 +1036,7 @@ xfs_buffered_write_iomap_begin(
> prealloc_blocks = 0;
> goto retry;
> }
> - /*FALLTHRU*/
> + fallthrough;
> default:
> goto out_unlock;
> }
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 9aced0a00003..d11d032da0b4 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -294,7 +294,7 @@ xfs_trans_read_buf_map(
> default:
> if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
> - /* fall through */
> + fallthrough;
> case -ENOMEM:
> case -EAGAIN:
> return error;
> --
> 2.27.0
>
On 4/20/21 18:38, Darrick J. Wong wrote:
> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> the following warnings by replacing /* fall through */ comments,
>> and its variants, with the new pseudo-keyword macro fallthrough:
>>
>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>
>> Notice that Clang doesn't recognize /* fall through */ comments as
>> implicit fall-through markings, so in order to globally enable
>> -Wimplicit-fallthrough for Clang, these comments need to be
>> replaced with fallthrough; in the whole codebase.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>
> I've already NAKd this twice, so I guess I'll NAK it a third time.
Darrick,
The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
This change is needed, and I would really prefer if this goes upstream through your tree.
Linus has taken these patches directly for a while, now.
Could you consider taking it this time? :)
Thanks
--
Gustavo
>
> --D
>
>> ---
>> fs/xfs/libxfs/xfs_ag_resv.c | 4 ++--
>> fs/xfs/libxfs/xfs_alloc.c | 2 +-
>> fs/xfs/libxfs/xfs_da_btree.c | 2 +-
>> fs/xfs/scrub/bmap.c | 2 +-
>> fs/xfs/scrub/btree.c | 2 +-
>> fs/xfs/scrub/common.c | 6 +++---
>> fs/xfs/scrub/dabtree.c | 2 +-
>> fs/xfs/scrub/repair.c | 2 +-
>> fs/xfs/xfs_bmap_util.c | 2 +-
>> fs/xfs/xfs_export.c | 4 ++--
>> fs/xfs/xfs_file.c | 2 +-
>> fs/xfs/xfs_inode.c | 2 +-
>> fs/xfs/xfs_ioctl.c | 4 ++--
>> fs/xfs/xfs_iomap.c | 2 +-
>> fs/xfs/xfs_trans_buf.c | 2 +-
>> 15 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
>> index 6c5f8d10589c..8c3c99a9bf83 100644
>> --- a/fs/xfs/libxfs/xfs_ag_resv.c
>> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
>> @@ -342,7 +342,7 @@ xfs_ag_resv_alloc_extent(
>> break;
>> default:
>> ASSERT(0);
>> - /* fall through */
>> + fallthrough;
>> case XFS_AG_RESV_NONE:
>> field = args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
>> XFS_TRANS_SB_FDBLOCKS;
>> @@ -384,7 +384,7 @@ xfs_ag_resv_free_extent(
>> break;
>> default:
>> ASSERT(0);
>> - /* fall through */
>> + fallthrough;
>> case XFS_AG_RESV_NONE:
>> xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, (int64_t)len);
>> return;
>> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
>> index aaa19101bb2a..9eabdeeec492 100644
>> --- a/fs/xfs/libxfs/xfs_alloc.c
>> +++ b/fs/xfs/libxfs/xfs_alloc.c
>> @@ -3163,7 +3163,7 @@ xfs_alloc_vextent(
>> }
>> args->agbno = XFS_FSB_TO_AGBNO(mp, args->fsbno);
>> args->type = XFS_ALLOCTYPE_NEAR_BNO;
>> - /* FALLTHROUGH */
>> + fallthrough;
>> case XFS_ALLOCTYPE_FIRST_AG:
>> /*
>> * Rotate through the allocation groups looking for a winner.
>> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
>> index 83ac9771bfb5..747ec77912c3 100644
>> --- a/fs/xfs/libxfs/xfs_da_btree.c
>> +++ b/fs/xfs/libxfs/xfs_da_btree.c
>> @@ -282,7 +282,7 @@ xfs_da3_node_read_verify(
>> __this_address);
>> break;
>> }
>> - /* fall through */
>> + fallthrough;
>> case XFS_DA_NODE_MAGIC:
>> fa = xfs_da3_node_verify(bp);
>> if (fa)
>> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
>> index b5ebf1d1b4db..77d5c4a0f09f 100644
>> --- a/fs/xfs/scrub/bmap.c
>> +++ b/fs/xfs/scrub/bmap.c
>> @@ -271,7 +271,7 @@ xchk_bmap_iextent_xref(
>> case XFS_DATA_FORK:
>> if (xfs_is_reflink_inode(info->sc->ip))
>> break;
>> - /* fall through */
>> + fallthrough;
>> case XFS_ATTR_FORK:
>> xchk_xref_is_not_shared(info->sc, agbno,
>> irec->br_blockcount);
>> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
>> index a94bd8122c60..bd1172358964 100644
>> --- a/fs/xfs/scrub/btree.c
>> +++ b/fs/xfs/scrub/btree.c
>> @@ -44,7 +44,7 @@ __xchk_btree_process_error(
>> /* Note the badness but don't abort. */
>> sc->sm->sm_flags |= errflag;
>> *error = 0;
>> - /* fall through */
>> + fallthrough;
>> default:
>> if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
>> trace_xchk_ifork_btree_op_error(sc, cur, level,
>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
>> index aa874607618a..ce9a44ea6948 100644
>> --- a/fs/xfs/scrub/common.c
>> +++ b/fs/xfs/scrub/common.c
>> @@ -81,7 +81,7 @@ __xchk_process_error(
>> /* Note the badness but don't abort. */
>> sc->sm->sm_flags |= errflag;
>> *error = 0;
>> - /* fall through */
>> + fallthrough;
>> default:
>> trace_xchk_op_error(sc, agno, bno, *error,
>> ret_ip);
>> @@ -134,7 +134,7 @@ __xchk_fblock_process_error(
>> /* Note the badness but don't abort. */
>> sc->sm->sm_flags |= errflag;
>> *error = 0;
>> - /* fall through */
>> + fallthrough;
>> default:
>> trace_xchk_file_op_error(sc, whichfork, offset, *error,
>> ret_ip);
>> @@ -694,7 +694,7 @@ xchk_get_inode(
>> if (error)
>> return -ENOENT;
>> error = -EFSCORRUPTED;
>> - /* fall through */
>> + fallthrough;
>> default:
>> trace_xchk_op_error(sc,
>> XFS_INO_TO_AGNO(mp, sc->sm->sm_ino),
>> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
>> index 653f3280e1c1..9f0dbb47c82c 100644
>> --- a/fs/xfs/scrub/dabtree.c
>> +++ b/fs/xfs/scrub/dabtree.c
>> @@ -47,7 +47,7 @@ xchk_da_process_error(
>> /* Note the badness but don't abort. */
>> sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
>> *error = 0;
>> - /* fall through */
>> + fallthrough;
>> default:
>> trace_xchk_file_op_error(sc, ds->dargs.whichfork,
>> xfs_dir2_da_to_db(ds->dargs.geo,
>> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
>> index c2857d854c83..b8202dd08939 100644
>> --- a/fs/xfs/scrub/repair.c
>> +++ b/fs/xfs/scrub/repair.c
>> @@ -947,7 +947,7 @@ xrep_ino_dqattach(
>> xrep_force_quotacheck(sc, XFS_DQTYPE_GROUP);
>> if (XFS_IS_PQUOTA_ON(sc->mp) && !sc->ip->i_pdquot)
>> xrep_force_quotacheck(sc, XFS_DQTYPE_PROJ);
>> - /* fall through */
>> + fallthrough;
>> case -ESRCH:
>> error = 0;
>> break;
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index a5e9d7d34023..cc628475f9b6 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -242,7 +242,7 @@ xfs_bmap_count_blocks(
>> */
>> *count += btblocks - 1;
>>
>> - /* fall through */
>> + fallthrough;
>> case XFS_DINODE_FMT_EXTENTS:
>> *nextents = xfs_bmap_count_leaves(ifp, count);
>> break;
>> diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
>> index 465fd9e048d4..1da59bdff245 100644
>> --- a/fs/xfs/xfs_export.c
>> +++ b/fs/xfs/xfs_export.c
>> @@ -84,7 +84,7 @@ xfs_fs_encode_fh(
>> case FILEID_INO32_GEN_PARENT:
>> fid->i32.parent_ino = XFS_I(parent)->i_ino;
>> fid->i32.parent_gen = parent->i_generation;
>> - /*FALLTHRU*/
>> + fallthrough;
>> case FILEID_INO32_GEN:
>> fid->i32.ino = XFS_I(inode)->i_ino;
>> fid->i32.gen = inode->i_generation;
>> @@ -92,7 +92,7 @@ xfs_fs_encode_fh(
>> case FILEID_INO32_GEN_PARENT | XFS_FILEID_TYPE_64FLAG:
>> fid64->parent_ino = XFS_I(parent)->i_ino;
>> fid64->parent_gen = parent->i_generation;
>> - /*FALLTHRU*/
>> + fallthrough;
>> case FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG:
>> fid64->ino = XFS_I(inode)->i_ino;
>> fid64->gen = inode->i_generation;
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index 396ef36dcd0a..3c0749ab9e40 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -863,7 +863,7 @@ xfs_break_layouts(
>> error = xfs_break_dax_layouts(inode, &retry);
>> if (error || retry)
>> break;
>> - /* fall through */
>> + fallthrough;
>> case BREAK_WRITE:
>> error = xfs_break_leased_layouts(inode, iolock, &retry);
>> break;
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 0369eb22c1bb..f2846997c3a8 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -848,7 +848,7 @@ xfs_init_new_inode(
>> xfs_inode_inherit_flags(ip, pip);
>> if (pip && (pip->i_diflags2 & XFS_DIFLAG2_ANY))
>> xfs_inode_inherit_flags2(ip, pip);
>> - /* FALLTHROUGH */
>> + fallthrough;
>> case S_IFLNK:
>> ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
>> ip->i_df.if_bytes = 0;
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 3925bfcb2365..c4dc6c72ac37 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -558,7 +558,7 @@ xfs_ioc_attrmulti_one(
>> case ATTR_OP_REMOVE:
>> value = NULL;
>> *len = 0;
>> - /* fall through */
>> + fallthrough;
>> case ATTR_OP_SET:
>> error = mnt_want_write_file(parfilp);
>> if (error)
>> @@ -1544,7 +1544,7 @@ xfs_ioc_getbmap(
>> switch (cmd) {
>> case XFS_IOC_GETBMAPA:
>> bmx.bmv_iflags = BMV_IF_ATTRFORK;
>> - /*FALLTHRU*/
>> + fallthrough;
>> case XFS_IOC_GETBMAP:
>> /* struct getbmap is a strict subset of struct getbmapx. */
>> recsize = sizeof(struct getbmap);
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index d154f42e2dc6..d8cd2583dedb 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -1036,7 +1036,7 @@ xfs_buffered_write_iomap_begin(
>> prealloc_blocks = 0;
>> goto retry;
>> }
>> - /*FALLTHRU*/
>> + fallthrough;
>> default:
>> goto out_unlock;
>> }
>> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
>> index 9aced0a00003..d11d032da0b4 100644
>> --- a/fs/xfs/xfs_trans_buf.c
>> +++ b/fs/xfs/xfs_trans_buf.c
>> @@ -294,7 +294,7 @@ xfs_trans_read_buf_map(
>> default:
>> if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
>> xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
>> - /* fall through */
>> + fallthrough;
>> case -ENOMEM:
>> case -EAGAIN:
>> return error;
>> --
>> 2.27.0
>>
On Tue, 2021-04-20 at 16:38 -0700, Darrick J. Wong wrote:
> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> > In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > the following warnings by replacing /* fall through */ comments,
> > and its variants, with the new pseudo-keyword macro fallthrough:
[]
> > Notice that Clang doesn't recognize /* fall through */ comments as
> > implicit fall-through markings, so in order to globally enable
> > -Wimplicit-fallthrough for Clang, these comments need to be
> > replaced with fallthrough; in the whole codebase.
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Signed-off-by: Gustavo A. R. Silva <[email protected]>
>
> I've already NAKd this twice, so I guess I'll NAK it a third time.
Sorry, I've must have missed it before. Why did you NAK this?
On 4/20/21 18:56, Gustavo A. R. Silva wrote:
>
>
> On 4/20/21 18:38, Darrick J. Wong wrote:
>> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>> the following warnings by replacing /* fall through */ comments,
>>> and its variants, with the new pseudo-keyword macro fallthrough:
>>>
>>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>>>
>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>> implicit fall-through markings, so in order to globally enable
>>> -Wimplicit-fallthrough for Clang, these comments need to be
>>> replaced with fallthrough; in the whole codebase.
>>>
>>> Link: https://github.com/KSPP/linux/issues/115
>>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>>
>> I've already NAKd this twice, so I guess I'll NAK it a third time.
>
> Darrick,
>
> The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
>
> https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
>
> This change is needed, and I would really prefer if this goes upstream through your tree.
>
> Linus has taken these patches directly for a while, now.
>
> Could you consider taking it this time? :)
>
Hi Darrick,
If you don't mind, I will take this in my -next[1] branch for v5.14, so we can globally enable
-Wimplicit-fallthrough for Clang in that release.
We had thousands of these warnings and now we are down to 47 in next-20210526,
22 of which are fixed with this patch.
Thanks
--
Gustavo
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
On Wed, May 26, 2021 at 01:21:06PM -0500, Gustavo A. R. Silva wrote:
>
>
> On 4/20/21 18:56, Gustavo A. R. Silva wrote:
> >
> >
> > On 4/20/21 18:38, Darrick J. Wong wrote:
> >> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> >>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> >>> the following warnings by replacing /* fall through */ comments,
> >>> and its variants, with the new pseudo-keyword macro fallthrough:
> >>>
> >>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> >>>
> >>> Notice that Clang doesn't recognize /* fall through */ comments as
> >>> implicit fall-through markings, so in order to globally enable
> >>> -Wimplicit-fallthrough for Clang, these comments need to be
> >>> replaced with fallthrough; in the whole codebase.
> >>>
> >>> Link: https://github.com/KSPP/linux/issues/115
> >>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> >>
> >> I've already NAKd this twice, so I guess I'll NAK it a third time.
> >
> > Darrick,
> >
> > The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
> >
> > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> >
> > This change is needed, and I would really prefer if this goes upstream through your tree.
> >
> > Linus has taken these patches directly for a while, now.
> >
> > Could you consider taking it this time? :)
> >
>
> Hi Darrick,
>
> If you don't mind, I will take this in my -next[1] branch for v5.14, so we can globally enable
> -Wimplicit-fallthrough for Clang in that release.
>
> We had thousands of these warnings and now we are down to 47 in next-20210526,
> 22 of which are fixed with this patch.
I guess we're all required to kowtow to a bunch of effing bots now.
Hooray for having to have a macro to code-switch for the sake of
stupid compiler writers who refuse to give the rest of us a single
workable way to signal "this switch code block should not end here":
/* fall through */
__attribute__((fallthrough));
do { } while (0) /* fall through */
and soon the ISO geniuses will make it worse by adding to C2x:
[[fallthrough]];
Hooray! Macros to abstractify stupidity!!!!
Dave and I have told you and Miaohe several[1] times[2] to fix[3] the
compiler, but clearly you don't care what we think and have decided to
ram this in through Linus anyway.
Since that is what you choose, do not send me email again.
NAKed-by: Darrick J. Wong <[email protected]>
--D
[1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
[2] https://lore.kernel.org/linux-xfs/20210420230652.GA70650@embeddedor/
[3] https://lore.kernel.org/linux-xfs/[email protected]/
>
> Thanks
> --
> Gustavo
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
On Wed, May 26, 2021 at 02:16:24PM -0700, Darrick J. Wong wrote:
> On Wed, May 26, 2021 at 01:21:06PM -0500, Gustavo A. R. Silva wrote:
> >
> >
> > On 4/20/21 18:56, Gustavo A. R. Silva wrote:
> > >
> > >
> > > On 4/20/21 18:38, Darrick J. Wong wrote:
> > >> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> > >>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > >>> the following warnings by replacing /* fall through */ comments,
> > >>> and its variants, with the new pseudo-keyword macro fallthrough:
> > >>>
> > >>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>>
> > >>> Notice that Clang doesn't recognize /* fall through */ comments as
> > >>> implicit fall-through markings, so in order to globally enable
> > >>> -Wimplicit-fallthrough for Clang, these comments need to be
> > >>> replaced with fallthrough; in the whole codebase.
> > >>>
> > >>> Link: https://github.com/KSPP/linux/issues/115
> > >>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > >>
> > >> I've already NAKd this twice, so I guess I'll NAK it a third time.
> > >
> > > Darrick,
> > >
> > > The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
> > >
> > > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > >
> > > This change is needed, and I would really prefer if this goes upstream through your tree.
> > >
> > > Linus has taken these patches directly for a while, now.
> > >
> > > Could you consider taking it this time? :)
> > >
> >
> > Hi Darrick,
> >
> > If you don't mind, I will take this in my -next[1] branch for v5.14, so we can globally enable
> > -Wimplicit-fallthrough for Clang in that release.
> >
> > We had thousands of these warnings and now we are down to 47 in next-20210526,
> > 22 of which are fixed with this patch.
>
> I guess we're all required to kowtow to a bunch of effing bots now.
> Hooray for having to have a macro to code-switch for the sake of
> stupid compiler writers who refuse to give the rest of us a single
> workable way to signal "this switch code block should not end here":
>
> /* fall through */
> __attribute__((fallthrough));
> do { } while (0) /* fall through */
>
> and soon the ISO geniuses will make it worse by adding to C2x:
>
> [[fallthrough]];
>
> Hooray! Macros to abstractify stupidity!!!!
>
> Dave and I have told you and Miaohe several[1] times[2] to fix[3] the
> compiler, but clearly you don't care what we think and have decided to
> ram this in through Linus anyway.
To clarify, we certainly _do_ care what you think. It's just that
when faced with the difficulties of the compiler's implementations of
handling this, the kernel had to get creative and pick the least-bad of
many bad choices. We're trying to make the kernel safer for everyone,
and this particular C language weakness has caused us a significant
number of bugs. Eradicating it is worth the effort.
All that said, as you pointed out, you _have_ asked before[1] to just
have Linus take it without bothering you directly, so okay, that can be
done. Generally maintainers have wanted these changes to go through their
trees so it doesn't cause them merge pain, but it seems you'd prefer it
the other way around.
-Kees
[1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
"If you feel really passionate about ramming a bunch of pointless churn
into the kernel tree to make my life more painful, send this to Linus
and let him make the change."
> Since that is what you choose, do not send me email again.
>
> NAKed-by: Darrick J. Wong <[email protected]>
>
> --D
>
> [1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
> [2] https://lore.kernel.org/linux-xfs/20210420230652.GA70650@embeddedor/
> [3] https://lore.kernel.org/linux-xfs/[email protected]/
>
> >
> > Thanks
> > --
> > Gustavo
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
--
Kees Cook
On Thu, May 27, 2021 at 02:31:12PM -0700, Kees Cook wrote:
> On Wed, May 26, 2021 at 02:16:24PM -0700, Darrick J. Wong wrote:
> > On Wed, May 26, 2021 at 01:21:06PM -0500, Gustavo A. R. Silva wrote:
> > >
> > >
> > > On 4/20/21 18:56, Gustavo A. R. Silva wrote:
> > > >
> > > >
> > > > On 4/20/21 18:38, Darrick J. Wong wrote:
> > > >> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> > > >>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > > >>> the following warnings by replacing /* fall through */ comments,
> > > >>> and its variants, with the new pseudo-keyword macro fallthrough:
> > > >>>
> > > >>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > > >>>
> > > >>> Notice that Clang doesn't recognize /* fall through */ comments as
> > > >>> implicit fall-through markings, so in order to globally enable
> > > >>> -Wimplicit-fallthrough for Clang, these comments need to be
> > > >>> replaced with fallthrough; in the whole codebase.
> > > >>>
> > > >>> Link: https://github.com/KSPP/linux/issues/115
> > > >>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > > >>
> > > >> I've already NAKd this twice, so I guess I'll NAK it a third time.
> > > >
> > > > Darrick,
> > > >
> > > > The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
> > > >
> > > > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > > >
> > > > This change is needed, and I would really prefer if this goes upstream through your tree.
> > > >
> > > > Linus has taken these patches directly for a while, now.
> > > >
> > > > Could you consider taking it this time? :)
> > > >
> > >
> > > Hi Darrick,
> > >
> > > If you don't mind, I will take this in my -next[1] branch for v5.14, so we can globally enable
> > > -Wimplicit-fallthrough for Clang in that release.
> > >
> > > We had thousands of these warnings and now we are down to 47 in next-20210526,
> > > 22 of which are fixed with this patch.
> >
> > I guess we're all required to kowtow to a bunch of effing bots now.
> > Hooray for having to have a macro to code-switch for the sake of
> > stupid compiler writers who refuse to give the rest of us a single
> > workable way to signal "this switch code block should not end here":
> >
> > /* fall through */
> > __attribute__((fallthrough));
> > do { } while (0) /* fall through */
> >
> > and soon the ISO geniuses will make it worse by adding to C2x:
> >
> > [[fallthrough]];
> >
> > Hooray! Macros to abstractify stupidity!!!!
> >
> > Dave and I have told you and Miaohe several[1] times[2] to fix[3] the
> > compiler, but clearly you don't care what we think and have decided to
> > ram this in through Linus anyway.
>
> To clarify, we certainly _do_ care what you think. It's just that
> when faced with the difficulties of the compiler's implementations of
> handling this, the kernel had to get creative and pick the least-bad of
> many bad choices.
The choices are bad, so **turn it off** in fs/xfs/Makefile and don't go
making us clutter up shared library code that will then have to be
ported to userspace.
--D
> We're trying to make the kernel safer for everyone,
> and this particular C language weakness has caused us a significant
> number of bugs. Eradicating it is worth the effort.
> All that said, as you pointed out, you _have_ asked before[1] to just
> have Linus take it without bothering you directly, so okay, that can be
> done. Generally maintainers have wanted these changes to go through their
> trees so it doesn't cause them merge pain, but it seems you'd prefer it
> the other way around.
>
> -Kees
>
> [1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
> "If you feel really passionate about ramming a bunch of pointless churn
> into the kernel tree to make my life more painful, send this to Linus
> and let him make the change."
>
> > Since that is what you choose, do not send me email again.
> >
> > NAKed-by: Darrick J. Wong <[email protected]>
> >
> > --D
> >
> > [1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
> > [2] https://lore.kernel.org/linux-xfs/20210420230652.GA70650@embeddedor/
> > [3] https://lore.kernel.org/linux-xfs/[email protected]/
> >
> > >
> > > Thanks
> > > --
> > > Gustavo
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
>
> --
> Kees Cook
On Thu, May 27, 2021 at 05:34:54PM -0700, Darrick J. Wong wrote:
> The choices are bad, so **turn it off** in fs/xfs/Makefile and don't go
> making us clutter up shared library code that will then have to be
> ported to userspace.
Ah! So the concern you have is with portable code shared outside of
the kernel? This came up also with the ACPICA code (which is regularly
imported into the kernel tree), and they just included their own macro
directly[1].
Would you prefer something like that, which would be XFS-specific? I'm
just trying to find a way to avoid losing fall-through coverage
in XFS. (Especially since distros are slowly moving toward enabling
-Wimplicit-fallthrough by default since it's a long-standing weakness
in the C language, and has been hiding real bugs for years.)
It seems like the options here could be:
1) Use an XFS-specific macro like ACPICA does, so that the out-of-tree
userspace code can share it (more typing, keep coverage).
2) Add -Wno-implicit-fallthrough to fs/xfs/Makefile (easy, lose coverage).
For 1), which portions are shared between xfsprogs and the kernel? Only
libxfs/ and scrub/? How does the below patch look? I could prepare similar
for all of xfsprogs, or do this only for xfsprogs and leave the stuff
outside of libxfs/ and scrube/ using the kernel's "fallthrough" macro?
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 782fdd08f759..ade529ddb60b 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -184,4 +184,14 @@ struct xfs_ino_geometry {
};
+/* Programmatically mark implicit fallthroughs for GCC and Clang. */
+#ifndef __has_attribute
+#define __has_attribute(x) 0
+#endif
+#if __has_attribute(__fallthrough__)
+#define XFS_FALLTHROUGH __attribute__((__fallthrough__))
+#else
+#define XFS_FALLTHROUGH do { } while (0)
+#endif
+
#endif /* __XFS_SHARED_H__ */
diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index e32a1833d523..2cf035ce6a3e 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -354,7 +354,7 @@ xfs_ag_resv_alloc_extent(
break;
default:
ASSERT(0);
- /* fall through */
+ XFS_FALLTHROUGH;
case XFS_AG_RESV_NONE:
field = args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
XFS_TRANS_SB_FDBLOCKS;
@@ -396,7 +396,7 @@ xfs_ag_resv_free_extent(
break;
default:
ASSERT(0);
- /* fall through */
+ XFS_FALLTHROUGH;
case XFS_AG_RESV_NONE:
xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, (int64_t)len);
return;
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 82b7cbb1f24f..5694e5ac925c 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3174,7 +3174,7 @@ xfs_alloc_vextent(
}
args->agbno = XFS_FSB_TO_AGBNO(mp, args->fsbno);
args->type = XFS_ALLOCTYPE_NEAR_BNO;
- /* FALLTHROUGH */
+ XFS_FALLTHROUGH;
case XFS_ALLOCTYPE_FIRST_AG:
/*
* Rotate through the allocation groups looking for a winner.
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 83ac9771bfb5..20a518757b75 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -282,7 +282,7 @@ xfs_da3_node_read_verify(
__this_address);
break;
}
- /* fall through */
+ XFS_FALLTHROUGH;
case XFS_DA_NODE_MAGIC:
fa = xfs_da3_node_verify(bp);
if (fa)
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index b5ebf1d1b4db..17964bc0c04d 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -271,7 +271,7 @@ xchk_bmap_iextent_xref(
case XFS_DATA_FORK:
if (xfs_is_reflink_inode(info->sc->ip))
break;
- /* fall through */
+ XFS_FALLTHROUGH;
case XFS_ATTR_FORK:
xchk_xref_is_not_shared(info->sc, agbno,
irec->br_blockcount);
diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
index a94bd8122c60..051b79442c68 100644
--- a/fs/xfs/scrub/btree.c
+++ b/fs/xfs/scrub/btree.c
@@ -44,7 +44,7 @@ __xchk_btree_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ XFS_FALLTHROUGH;
default:
if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
trace_xchk_ifork_btree_op_error(sc, cur, level,
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index aa874607618a..30ec2ac1228d 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -81,7 +81,7 @@ __xchk_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ XFS_FALLTHROUGH;
default:
trace_xchk_op_error(sc, agno, bno, *error,
ret_ip);
@@ -134,7 +134,7 @@ __xchk_fblock_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
- /* fall through */
+ XFS_FALLTHROUGH;
default:
trace_xchk_file_op_error(sc, whichfork, offset, *error,
ret_ip);
@@ -694,7 +694,7 @@ xchk_get_inode(
if (error)
return -ENOENT;
error = -EFSCORRUPTED;
- /* fall through */
+ XFS_FALLTHROUGH;
default:
trace_xchk_op_error(sc,
XFS_INO_TO_AGNO(mp, sc->sm->sm_ino),
diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
index 653f3280e1c1..afbfe731e160 100644
--- a/fs/xfs/scrub/dabtree.c
+++ b/fs/xfs/scrub/dabtree.c
@@ -47,7 +47,7 @@ xchk_da_process_error(
/* Note the badness but don't abort. */
sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
*error = 0;
- /* fall through */
+ XFS_FALLTHROUGH;
default:
trace_xchk_file_op_error(sc, ds->dargs.whichfork,
xfs_dir2_da_to_db(ds->dargs.geo,
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index c2857d854c83..68b0f2248b8d 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -947,7 +947,7 @@ xrep_ino_dqattach(
xrep_force_quotacheck(sc, XFS_DQTYPE_GROUP);
if (XFS_IS_PQUOTA_ON(sc->mp) && !sc->ip->i_pdquot)
xrep_force_quotacheck(sc, XFS_DQTYPE_PROJ);
- /* fall through */
+ XFS_FALLTHROUGH;
case -ESRCH:
error = 0;
break;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index a5e9d7d34023..3e467aa11ee5 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -242,7 +242,7 @@ xfs_bmap_count_blocks(
*/
*count += btblocks - 1;
- /* fall through */
+ XFS_FALLTHROUGH;
case XFS_DINODE_FMT_EXTENTS:
*nextents = xfs_bmap_count_leaves(ifp, count);
break;
diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
index 465fd9e048d4..dcf92f90be05 100644
--- a/fs/xfs/xfs_export.c
+++ b/fs/xfs/xfs_export.c
@@ -84,7 +84,7 @@ xfs_fs_encode_fh(
case FILEID_INO32_GEN_PARENT:
fid->i32.parent_ino = XFS_I(parent)->i_ino;
fid->i32.parent_gen = parent->i_generation;
- /*FALLTHRU*/
+ XFS_FALLTHROUGH;
case FILEID_INO32_GEN:
fid->i32.ino = XFS_I(inode)->i_ino;
fid->i32.gen = inode->i_generation;
@@ -92,7 +92,7 @@ xfs_fs_encode_fh(
case FILEID_INO32_GEN_PARENT | XFS_FILEID_TYPE_64FLAG:
fid64->parent_ino = XFS_I(parent)->i_ino;
fid64->parent_gen = parent->i_generation;
- /*FALLTHRU*/
+ XFS_FALLTHROUGH;
case FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG:
fid64->ino = XFS_I(inode)->i_ino;
fid64->gen = inode->i_generation;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 396ef36dcd0a..f4f58e5e8d3b 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -863,7 +863,7 @@ xfs_break_layouts(
error = xfs_break_dax_layouts(inode, &retry);
if (error || retry)
break;
- /* fall through */
+ XFS_FALLTHROUGH;
case BREAK_WRITE:
error = xfs_break_leased_layouts(inode, iolock, &retry);
break;
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 34f2b971ce43..d6cdd87eb2cf 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -100,7 +100,6 @@ xfs_fsmap_owner_to_rmap(
dest->rm_owner = XFS_RMAP_OWN_COW;
break;
case XFS_FMR_OWN_DEFECTIVE: /* not implemented */
- /* fall through */
default:
return -EINVAL;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0369eb22c1bb..cb9ae2cb209a 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -848,7 +848,7 @@ xfs_init_new_inode(
xfs_inode_inherit_flags(ip, pip);
if (pip && (pip->i_diflags2 & XFS_DIFLAG2_ANY))
xfs_inode_inherit_flags2(ip, pip);
- /* FALLTHROUGH */
+ XFS_FALLTHROUGH;
case S_IFLNK:
ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
ip->i_df.if_bytes = 0;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 3925bfcb2365..ae4bffc3a979 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -558,7 +558,7 @@ xfs_ioc_attrmulti_one(
case ATTR_OP_REMOVE:
value = NULL;
*len = 0;
- /* fall through */
+ XFS_FALLTHROUGH;
case ATTR_OP_SET:
error = mnt_want_write_file(parfilp);
if (error)
@@ -1544,7 +1544,7 @@ xfs_ioc_getbmap(
switch (cmd) {
case XFS_IOC_GETBMAPA:
bmx.bmv_iflags = BMV_IF_ATTRFORK;
- /*FALLTHRU*/
+ XFS_FALLTHROUGH;
case XFS_IOC_GETBMAP:
/* struct getbmap is a strict subset of struct getbmapx. */
recsize = sizeof(struct getbmap);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d154f42e2dc6..8fc7afb9b070 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1036,7 +1036,7 @@ xfs_buffered_write_iomap_begin(
prealloc_blocks = 0;
goto retry;
}
- /*FALLTHRU*/
+ XFS_FALLTHROUGH;
default:
goto out_unlock;
}
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 9aced0a00003..dc5d8b8d32e3 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -294,7 +294,7 @@ xfs_trans_read_buf_map(
default:
if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
- /* fall through */
+ XFS_FALLTHROUGH;
case -ENOMEM:
case -EAGAIN:
return error;
-Kees
[1] https://github.com/acpica/acpica/commit/4b9135f5774caa796ddf826448811e8e7f08ef2f
--
Kees Cook
On Fri, May 28, 2021 at 09:48:39AM -0700, Kees Cook wrote:
> On Thu, May 27, 2021 at 05:34:54PM -0700, Darrick J. Wong wrote:
> > The choices are bad, so **turn it off** in fs/xfs/Makefile and don't go
> > making us clutter up shared library code that will then have to be
> > ported to userspace.
>
> Ah! So the concern you have is with portable code shared outside of
> the kernel? This came up also with the ACPICA code (which is regularly
> imported into the kernel tree), and they just included their own macro
> directly[1].
>
> Would you prefer something like that, which would be XFS-specific? I'm
> just trying to find a way to avoid losing fall-through coverage
> in XFS. (Especially since distros are slowly moving toward enabling
> -Wimplicit-fallthrough by default since it's a long-standing weakness
> in the C language, and has been hiding real bugs for years.)
>
> It seems like the options here could be:
> 1) Use an XFS-specific macro like ACPICA does, so that the out-of-tree
> userspace code can share it (more typing, keep coverage).
> 2) Add -Wno-implicit-fallthrough to fs/xfs/Makefile (easy, lose coverage).
>
> For 1), which portions are shared between xfsprogs and the kernel? Only
> libxfs/ and scrub/? How does the below patch look? I could prepare similar
> for all of xfsprogs, or do this only for xfsprogs and leave the stuff
> outside of libxfs/ and scrube/ using the kernel's "fallthrough" macro?
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 782fdd08f759..ade529ddb60b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -184,4 +184,14 @@ struct xfs_ino_geometry {
>
> };
>
> +/* Programmatically mark implicit fallthroughs for GCC and Clang. */
> +#ifndef __has_attribute
> +#define __has_attribute(x) 0
> +#endif
> +#if __has_attribute(__fallthrough__)
> +#define XFS_FALLTHROUGH __attribute__((__fallthrough__))
> +#else
> +#define XFS_FALLTHROUGH do { } while (0)
> +#endif
No. This is Obviously Stupid.
Did you listen to what Darrick just said? Wasn't it "Don't go making
us clutter up shared library code..."?
If we're complaining that replacing obvious comments demonstrating
intent with weird macros is not in our best interest, then replacing
the comments with an eye-bleeding, one off, XFS specific macro
doesn't address the objections being raised.
I don't even know why you are continuing trying to convince us to
take the changes - Darrick has already given you guys the option of
going straight to Linus with them. If you do that then we'll just
have to deal with the undefined macro fallout that results in
userspace.
Please just stop wasting more of our time on this.
-Dave.
--
Dave Chinner
[email protected]