[2/2] block: in commit, determine base image from the top image

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 <jcody@redhat.com>
---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

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 <jcody@redhat.com>
> ---
>  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 <pbonzini@redhat.com>

Paolo

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block/commit.c |  9 ---------
>  blockdev.c     | 21 +++++++++++----------
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 

> @@ -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);

In particular, if I have the chain:

/dir1/base <- /dir1/snap1(base) <- /dir2/base(/dir1/snap1) <-
/dir2/snap2(base)

and request a commit of "top":"/dir1/snap1", "base":"base", then this
new code will now look up base relative to /dir1/snap1 and find
/dir1/base and proceed with the commit; whereas the old code would look
up base relative to /dir2/snap2 and find /dir2/base and complain that
the arguments were swapped.

I can live with those semantics.

Reviewed-by: Eric Blake <eblake@redhat.com>

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---

> +++ b/blockdev.c

> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    if (base && has_base) {

Code motion, but won't this order trigger valgrind warnings about
jumping based on a read of uninitialized memory?  That is, the canonical
ordering is 'has_base && base' (although the end result is the same,
with the difference only observable in valgrind).