Re: [Qemu-devel] [PATCH 12/30] QAPI: add command for live block commit, 'block-commit'

[Eric Blake <eblake@redhat.com>]

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

On 09/28/2012 11:56 AM, Kevin Wolf wrote:
> From: Jeff Cody <jcody@redhat.com>
> 
> The command for live block commit is added, which has the following
> arguments:
> 
> device: the block device to perform the commit on (mandatory)
> base:   the base image to commit into; optional (if not specified,
>         it is the underlying original image)
> top:    the top image of the commit - all data from inside top down
>         to base will be committed into base (mandatory for now; see
>         note, below)

We will need a followup patch, for this to work on chains with relative
backing file names.


> +    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;
> +
> +    if (top) {
> +        if (strcmp(bs->filename, top) != 0) {
> +            top_bs = bdrv_find_backing_image(bs, top);
> +        }
> +    }

Right now, if I have 'base' <- 'snap1'(base) <- 'snap2'(snap1) <-
'active'(snap2), then base_bs and top_bs will be found, but later on in
commit_start, we have:

> 
> +    /* 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;
> +    }

which uses the absolute file name base->filename and fails to find base,
making it impossible to commit into a base file that was referenced via
relative name in the chain.  I think that bdrv_find_backing_image needs
to canonicalize any relative name passed in on input relative to the
directory of the bs where it is starting the search, and then search for
absolute name matches rather than the current approach of searching for
exact string matches (even if the exact string is a relative name).

Also, consider this case of mixed relative and absolute backing files in
the chain:

/dir1/base <- /dir1/file1(base) <- /dir2/base(/dir1/file1) <-
/dir2/file2(base)

The relative name 'base' appears twice in the chain, so you either have
to declare it ambiguous, or else declare that relative names are
canonicalized relative to the starting point (such that
bdrv_find_backing_image(/dir1/file1, "base") gives a different result
than bdrv_find_backing_image(/dir2/file2, "base").  Which means, if I
request block-commit "top":"/dir1/file1", "base":"base", am I requesting
a commit into /dir1/base (good) or into /dir2/base (swapped arguments)?

Fortunately, it looks like if I have 'base' <- 'snap1'(/path/to/base) <-
'snap2'(/path/to/snap1) <- 'active'(/path/to/snap2), then things work
for committing snap2 into snap1, when I specify absolute file names for
top and base.  But here, it would be convenient if I could pass names
relative to the location of active and have them canonicalized into
absolute, so the same fix for relative chains will make absolute chains
easier to use.

Sounds like we need more tests to cover these scenarios.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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