Re: [Qemu-devel] [PATCH v3 05/16] block: Convert bs->file to BdrvChild

[Jeff Cody <jcody@redhat.com>]

On Fri, Oct 09, 2015 at 02:15:30PM +0200, Kevin Wolf wrote:
> This patch removes the temporary duplication between bs->file and
> bs->file_child by converting everything to BdrvChild.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 63 ++++++++++++++++++++++-------------------------
>  block/blkdebug.c          | 32 +++++++++++++-----------
>  block/blkverify.c         | 33 ++++++++++++++-----------
>  block/bochs.c             |  8 +++---
>  block/cloop.c             | 10 ++++----
>  block/dmg.c               | 20 +++++++--------
>  block/io.c                | 50 ++++++++++++++++++-------------------
>  block/parallels.c         | 38 ++++++++++++++--------------
>  block/qapi.c              |  2 +-
>  block/qcow.c              | 42 ++++++++++++++++---------------
>  block/qcow2-cache.c       | 11 +++++----
>  block/qcow2-cluster.c     | 38 ++++++++++++++++------------
>  block/qcow2-refcount.c    | 45 +++++++++++++++++----------------
>  block/qcow2-snapshot.c    | 30 +++++++++++-----------
>  block/qcow2.c             | 62 ++++++++++++++++++++++++----------------------
>  block/qed-table.c         |  4 +--
>  block/qed.c               | 32 ++++++++++++------------
>  block/raw_bsd.c           | 40 +++++++++++++++---------------
>  block/snapshot.c          | 12 ++++-----
>  block/vdi.c               | 17 +++++++------
>  block/vhdx-log.c          | 25 ++++++++++---------
>  block/vhdx.c              | 36 ++++++++++++++-------------
>  block/vmdk.c              | 27 ++++++++++----------
>  block/vpc.c               | 34 +++++++++++++------------
>  include/block/block.h     |  8 +++++-
>  include/block/block_int.h |  3 +--
>  26 files changed, 378 insertions(+), 344 deletions(-)
>

[snip lots of code]

> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index bc247f4..117fce6 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -427,10 +427,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      s->state = 1;
>  
>      /* Open the backing file */
> -    assert(bs->file == NULL);
> -    ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
> -                          bs, &child_file, false, &local_err);
> -    if (ret < 0) {
> +    bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
> +                               bs, &child_file, false, &local_err);
> +    if (local_err) {
> +        ret = -EINVAL;
>          error_propagate(errp, local_err);
>          goto out;
>      }
> @@ -449,7 +449,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      goto out;
>  
>  fail_unref:
> -    bdrv_unref(bs->file);
> +    bdrv_unref(bs->file->bs);


Would it be better to use bdrv_unref_child() here?

[snip lots of code]

> diff --git a/block/qcow.c b/block/qcow.c
> index 6e35db1..4d20cd5 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -100,7 +100,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>      int ret;
>      QCowHeader header;
>  
> -    ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
> +    ret = bdrv_pread(bs->file->bs, 0, &header, sizeof(header));
>      if (ret < 0) {
>          goto fail;
>      }
> @@ -193,7 +193,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    ret = bdrv_pread(bs->file, s->l1_table_offset, s->l1_table,
> +    ret = bdrv_pread(bs->file->bs, s->l1_table_offset, s->l1_table,
>                 s->l1_size * sizeof(uint64_t));
>      if (ret < 0) {
>          goto fail;
> @@ -205,7 +205,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      /* alloc L2 cache (max. 64k * 16 * 8 = 8 MB) */
>      s->l2_cache =
> -        qemu_try_blockalign(bs->file,
> +        qemu_try_blockalign(bs->file->bs,
>                              s->l2_size * L2_CACHE_SIZE * sizeof(uint64_t));
>      if (s->l2_cache == NULL) {
>          error_setg(errp, "Could not allocate L2 table cache");
> @@ -224,7 +224,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>              ret = -EINVAL;
>              goto fail;
>          }
> -        ret = bdrv_pread(bs->file, header.backing_file_offset,
> +        ret = bdrv_pread(bs->file->bs, header.backing_file_offset,
>                     bs->backing_file, len);
>          if (ret < 0) {
>              goto fail;
> @@ -369,13 +369,13 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>          if (!allocate)
>              return 0;
>          /* allocate a new l2 entry */
> -        l2_offset = bdrv_getlength(bs->file);
> +        l2_offset = bdrv_getlength(bs->file->bs);
>          /* round to cluster size */
>          l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
>          /* update the L1 entry */
>          s->l1_table[l1_index] = l2_offset;
>          tmp = cpu_to_be64(l2_offset);
> -        if (bdrv_pwrite_sync(bs->file,
> +        if (bdrv_pwrite_sync(bs->file->bs,
>                  s->l1_table_offset + l1_index * sizeof(tmp),
>                  &tmp, sizeof(tmp)) < 0)
>              return 0;
> @@ -405,11 +405,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>      l2_table = s->l2_cache + (min_index << s->l2_bits);
>      if (new_l2_table) {
>          memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
> -        if (bdrv_pwrite_sync(bs->file, l2_offset, l2_table,
> +        if (bdrv_pwrite_sync(bs->file->bs, l2_offset, l2_table,
>                  s->l2_size * sizeof(uint64_t)) < 0)
>              return 0;
>      } else {
> -        if (bdrv_pread(bs->file, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
> +        if (bdrv_pread(bs->file->bs, l2_offset, l2_table,
> +                       s->l2_size * sizeof(uint64_t)) !=
>              s->l2_size * sizeof(uint64_t))
>              return 0;
>      }
> @@ -430,20 +431,21 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                 overwritten */
>              if (decompress_cluster(bs, cluster_offset) < 0)
>                  return 0;
> -            cluster_offset = bdrv_getlength(bs->file);
> +            cluster_offset = bdrv_getlength(bs->file->bs);
>              cluster_offset = (cluster_offset + s->cluster_size - 1) &
>                  ~(s->cluster_size - 1);
>              /* write the cluster content */
> -            if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, s->cluster_size) !=
> +            if (bdrv_pwrite(bs->file->bs, cluster_offset, s->cluster_cache,
> +                            s->cluster_size) !=
>                  s->cluster_size)
>                  return -1;
>          } else {
> -            cluster_offset = bdrv_getlength(bs->file);
> +            cluster_offset = bdrv_getlength(bs->file->bs);
>              if (allocate == 1) {
>                  /* round to cluster size */
>                  cluster_offset = (cluster_offset + s->cluster_size - 1) &
>                      ~(s->cluster_size - 1);
> -                bdrv_truncate(bs->file, cluster_offset + s->cluster_size);
> +                bdrv_truncate(bs->file->bs, cluster_offset + s->cluster_size);
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
>                  if (bs->encrypted &&
> @@ -463,7 +465,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                                  errno = EIO;
>                                  return -1;
>                              }
> -                            if (bdrv_pwrite(bs->file, cluster_offset + i * 512,
> +                            if (bdrv_pwrite(bs->file->bs, cluster_offset + i * 512,

This exceeds 80 characters... but qcow.c has numerous style issues, so
I am not bothered by it.


[snip lots of code]


> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 6ede629..7844f8e 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -72,7 +72,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>  #endif
>  
>      new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> -    new_l1_table = qemu_try_blockalign(bs->file,
> +    new_l1_table = qemu_try_blockalign(bs->file->bs,
>                                         align_offset(new_l1_size2, 512));
>      if (new_l1_table == NULL) {
>          return -ENOMEM;
> @@ -105,7 +105,8 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
>      for(i = 0; i < s->l1_size; i++)
>          new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
> -    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset, new_l1_table, new_l1_size2);
> +    ret = bdrv_pwrite_sync(bs->file->bs, new_l1_table_offset,
> +                           new_l1_table, new_l1_size2);
>      if (ret < 0)
>          goto fail;
>      for(i = 0; i < s->l1_size; i++)
> @@ -115,7 +116,8 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
>      cpu_to_be32w((uint32_t*)data, new_l1_size);
>      stq_be_p(data + 4, new_l1_table_offset);
> -    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size), data,sizeof(data));
> +    ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, l1_size),
> +                           data, sizeof(data));
>      if (ret < 0) {
>          goto fail;
>      }
> @@ -182,8 +184,9 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
>      }
>  
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_UPDATE);
> -    ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset + 8 * l1_start_index,
> -        buf, sizeof(buf));
> +    ret = bdrv_pwrite_sync(bs->file->bs,
> +                           s->l1_table_offset + 8 * l1_start_index,
> +                           buf, sizeof(buf));
>      if (ret < 0) {
>          return ret;
>      }
> @@ -440,7 +443,8 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
>      }
>  
>      BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
> -    ret = bdrv_co_writev(bs->file, (cluster_offset >> 9) + n_start, n, &qiov);
> +    ret = bdrv_co_writev(bs->file->bs, (cluster_offset >> 9) + n_start, n,
> +                         &qiov);
>      if (ret < 0) {
>          goto out;
>      }
> @@ -817,7 +821,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>  
>      /*
>       * If this was a COW, we need to decrease the refcount of the old cluster.
> -     * Also flush bs->file to get the right order for L2 and refcount update.
> +     * Also flush bs->file->bs to get the right order for L2 and refcount update.

Over by 1 character :).  Up to you if you want to fix it.


[snip lots of code]

> diff --git a/include/block/block.h b/include/block/block.h
> index 2dd6630..7ebb35d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -585,7 +585,13 @@ typedef enum {
>      BLKDBG_EVENT_MAX,
>  } BlkDebugEvent;
>  
> -#define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
> +#define BLKDBG_EVENT(child, evt) \
> +    do { \
> +        if (child) { \
> +            bdrv_debug_event(child->bs, evt); \
> +        } \
> +    } while(0)

According to style guidelines, this should have a space: while (0).
Again, your choice if you want to fix it or not.