Re: [Qemu-devel] [PATCH 2/2] block: in commit, determine base image from the top image

[Paolo Bonzini]

Il 10/10/2012 07:56, Jeff Cody ha scritto:
> This simplifies some code and error checking, and also fixes a bug.
> 
> bdrv_find_backing_image() should only be passed absolute filenames,
> or filenames relative to the chain.  In the QMP message handler for
> block commit, when looking up the base do so from the determined top
> image, so we know it is reachable from top.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/commit.c |  9 ---------
>  blockdev.c     | 21 +++++++++++----------
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 733c914..13d9e82 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -211,15 +211,6 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>          return;
>      }
>  
> -    /* top and base may be valid, but let's make sure that base is reachable
> -     * from top */
> -    if (bdrv_find_backing_image(top, base->filename) != base) {
> -        error_setg(errp,
> -                   "Base (%s) is not reachable from top (%s)",
> -                   base->filename, top->filename);
> -        return;
> -    }
> -
>      overlay_bs = bdrv_find_overlay(bs, top);
>  
>      if (overlay_bs == NULL) {
> diff --git a/blockdev.c b/blockdev.c
> index 99828ad..7052287 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1157,16 +1157,6 @@ void qmp_block_commit(const char *device,
>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>          return;
>      }
> -    if (base && has_base) {
> -        base_bs = bdrv_find_backing_image(bs, base);
> -    } else {
> -        base_bs = bdrv_find_base(bs);
> -    }
> -
> -    if (base_bs == NULL) {
> -        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
> -        return;
> -    }
>  
>      /* default top_bs is the active layer */
>      top_bs = bs;
> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    if (base && has_base) {
> +        base_bs = bdrv_find_backing_image(top_bs, base);
> +    } else {
> +        base_bs = bdrv_find_base(top_bs);
> +    }
> +
> +    if (base_bs == NULL) {
> +        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
> +        return;
> +    }
> +
>      commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
>                  &local_err);
>      if (local_err != NULL) {
> 

This one looks good,

Reviewed-by: Paolo Bonzini <address@hidden>

Paolo