Re: [Qemu-devel] [PATCH 07/16] block: Convert bs->backing_hd to BdrvChild

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4973 bytes --]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This is the final step in converting all of the BlockDriverState
> pointers that block drivers use to BdrvChild.
> 
> After this patch, bs->children contains the full list of child nodes
> that are referenced by a given BDS, and these children are only
> referenced through BdrvChild, so that updating the pointer in there is
> enough for changing edges in the graph.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 116 +++++++++++++++++++++++++---------------------
>  block/io.c                |  24 +++++-----
>  block/mirror.c            |   7 +--
>  block/qapi.c              |   8 ++--
>  block/qcow.c              |   4 +-
>  block/qcow2-cluster.c     |   4 +-
>  block/qcow2.c             |   6 +--
>  block/qed.c               |  12 ++---
>  block/stream.c            |  10 ++--
>  block/vmdk.c              |  21 +++++----
>  block/vvfat.c             |   6 +--
>  blockdev.c                |   6 +--
>  include/block/block_int.h |  12 +++--
>  qemu-img.c                |   8 ++--
>  14 files changed, 130 insertions(+), 114 deletions(-)
> 

[...]

> diff --git a/block/io.c b/block/io.c
> index 8a27efa..d7e742a 100644
> --- a/block/io.c
> +++ b/block/io.c

[...]

> @@ -1604,7 +1604,7 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
>                                int64_t sector_num,
>                                int nb_sectors, int *pnum)
>  {
> -    return bdrv_get_block_status_above(bs, bs->backing_hd,
> +    return bdrv_get_block_status_above(bs, backing_bs(bs),
>                                         sector_num, nb_sectors, pnum);
>  }
>  
> @@ -1662,7 +1662,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>              n = pnum_inter;
>          }
>  
> -        intermediate = intermediate->backing_hd;
> +        intermediate = intermediate->backing ? intermediate->backing->bs : NULL;

backing_bs(intermediate)?

>      }
>  
>      *pnum = n;
> diff --git a/block/mirror.c b/block/mirror.c
> index a258926..259e11a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -371,7 +371,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
>          if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
>              /* drop the bs loop chain formed by the swap: break the loop then
>               * trigger the unref from the top one */
> -            BlockDriverState *p = s->base->backing_hd;
> +            BlockDriverState *p = s->base->backing
> +                                ? s->base->backing->bs : NULL;

Maybe you don't want to use backing_bs() outside of the core block
layer, but it could be used here, too.

(There are two similar expressions in block/stream.c, and maybe
elsewhere, too)

>              bdrv_set_backing_hd(s->base, NULL);
>              bdrv_unref(p);
>          }

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 32b04b4..bc158ff 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2508,10 +2508,10 @@ void qmp_drive_backup(const char *device, const char *target,
>      /* See if we have a backing HD we can use to create our new image
>       * on top of. */
>      if (sync == MIRROR_SYNC_MODE_TOP) {
> -        source = bs->backing_hd;
> -        if (!source) {
> +        if (!bs->backing) {
>              sync = MIRROR_SYNC_MODE_FULL;
>          }
> +        source = bs->backing->bs;

That doesn't seem right. In case of !bs->backing, this won't abort but
just continue and run into bs->backing->bs, which should therefore
probably be backing_bs(bs) instead.

>      }
>      if (sync == MIRROR_SYNC_MODE_NONE) {
>          source = bs;
> @@ -2716,7 +2716,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>      }
>  
>      flags = bs->open_flags | BDRV_O_RDWR;
> -    source = bs->backing_hd;
> +    source = bs->backing ? bs->backing->bs : NULL;

Why not source = backing_bs(bs)?

>      if (!source && sync == MIRROR_SYNC_MODE_TOP) {
>          sync = MIRROR_SYNC_MODE_FULL;
>      }

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 6ff4e85..c4454da 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -2206,11 +2206,11 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
>          if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
>              break;
>          }
> -        bs = bs->backing_hd;
> -        if (bs == NULL) {
> +        if (bs->backing == NULL) {
>              ret = 0;
>              break;
>          }
> +        bs = bs->backing->bs;

This changes behavior. bs needs to be set to NULL in the
if (bs->backing == NULL) block, or the break will break: Before, if
bs->backing_hd == NULL, the loop was left with bs == NULL. Now, bs won't
be NULL anymore (but its value is used after the loop and stored in e->bs).

Max

>  
>          depth++;
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]