Re: [RFC PATCH v2 02/78] block: add fallthrough pseudo-keyword

[Eric Blake]

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.pitsidianakis@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