On Fri, Oct 13, 2023 at 10:56:29AM +0300, Emmanouil Pitsidianakis wrote: > In preparation of raising -Wimplicit-fallthrough to 5, replace all > fall-through comments with the fallthrough attribute pseudo-keyword. > > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidiana...@linaro.org> > --- > block/block-copy.c | 1 + > block/file-posix.c | 1 + > block/io.c | 1 + > block/iscsi.c | 1 + > block/qcow2-cluster.c | 5 ++++- > block/vhdx.c | 17 +++++++++++++---- > 6 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/block/block-copy.c b/block/block-copy.c > index 1c60368d72..b4ceb6a079 100644 > --- a/block/block-copy.c > +++ b/block/block-copy.c ... > case COPY_RANGE_FULL: > ret = bdrv_co_copy_range(s->source, offset, s->target, offset, > nbytes, > 0, s->write_flags); > if (ret >= 0) { > /* Successful copy-range, increase chunk size. */ > *method = COPY_RANGE_FULL; > return 0; > } > > trace_block_copy_copy_range_fail(s, offset, ret); > *method = COPY_READ_WRITE; > /* Fall through to read+write with allocated buffer */ > + fallthrough; > > case COPY_READ_WRITE_CLUSTER: > case COPY_READ_WRITE:
I like how you kept the comments. > +++ b/block/qcow2-cluster.c > @@ -1327,36 +1327,39 @@ static int coroutine_fn > calculate_l2_meta(BlockDriverState *bs, > /* > * Returns true if writing to the cluster pointed to by @l2_entry > * requires a new allocation (that is, if the cluster is unallocated > * or has refcount > 1 and therefore cannot be written in-place). > */ > static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry) > { > switch (qcow2_get_cluster_type(bs, l2_entry)) { > case QCOW2_CLUSTER_NORMAL: > + fallthrough; > case QCOW2_CLUSTER_ZERO_ALLOC: Why is this one needed? It looks two case labels for the same code is okay; the fallthrough attribute is only needed once a case label is no lonter empty. > if (l2_entry & QCOW_OFLAG_COPIED) { > return false; > } > - /* fallthrough */ > + fallthrough; This one makes sense. > case QCOW2_CLUSTER_UNALLOCATED: > + fallthrough; > case QCOW2_CLUSTER_COMPRESSED: > + fallthrough; These two also look spurious. > case QCOW2_CLUSTER_ZERO_PLAIN: > return true; > default: > abort(); > } > } ... > +++ b/block/vhdx.c > @@ -1176,60 +1176,65 @@ static int coroutine_fn GRAPH_RDLOCK > vhdx_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > QEMUIOVector *qiov) ... > /* check the payload block state */ > switch (s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) { > - case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */ > + case PAYLOAD_BLOCK_NOT_PRESENT: > + fallthrough; > case PAYLOAD_BLOCK_UNDEFINED: > + fallthrough; > case PAYLOAD_BLOCK_UNMAPPED: > + fallthrough; > case PAYLOAD_BLOCK_UNMAPPED_v095: > + fallthrough; All four of these look spurious; although the old comment is also spurious, so I'd be happy with deleting it without replacement. > case PAYLOAD_BLOCK_ZERO: > /* return zero */ > qemu_iovec_memset(&hd_qiov, 0, 0, sinfo.bytes_avail); > break; > case PAYLOAD_BLOCK_FULLY_PRESENT: > qemu_co_mutex_unlock(&s->lock); > ret = bdrv_co_preadv(bs->file, sinfo.file_offset, > sinfo.sectors_avail * BDRV_SECTOR_SIZE, > &hd_qiov, 0); > qemu_co_mutex_lock(&s->lock); > if (ret < 0) { > goto exit; > } > break; > case PAYLOAD_BLOCK_PARTIALLY_PRESENT: > /* we don't yet support difference files, fall through > * to error */ > + fallthrough; > default: But keeping this one because of the comment is reasonable. ... > switch (bat_state) { > case PAYLOAD_BLOCK_ZERO: > /* in this case, we need to preserve zero writes for > * data that is not part of this write, so we must pad > * the rest of the buffer to zeroes */ > use_zero_buffers = true; > - /* fall through */ > - case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */ > + fallthrough; > + case PAYLOAD_BLOCK_NOT_PRESENT: This one is necessary; > + fallthrough; > case PAYLOAD_BLOCK_UNMAPPED: > + fallthrough; > case PAYLOAD_BLOCK_UNMAPPED_v095: > + fallthrough; > case PAYLOAD_BLOCK_UNDEFINED: but these three seem spurious. I like the direction this is headed in, but there's enough I pointed out that I'll withhold R-b on this version. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org