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: 1412 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:
> 

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

In light of Jeff's followup patches to fix bdrv_find_backing_image when
dealing with relative file names, I see another bug here (but impact is
limited to only a poor-quality error message, claiming that 'top was not
found' instead of the intended 'commit of an active top is not
supported').  That is, strcmp() on user-supplied 'top' is not guaranteed
to succeed if the user provided a different spelling for the same file
as was actually recorded in bs->filename, and since you have resorted to
realpath() before strcmp() in bdrv_find_backing_image to cope with
alternat user spellings, I think you also need to use realpath() before
comparison here (probably a new helper function bdrv_is_equal() or some
such name, that compares a user-supplied name with a bs).  Saving this
until a followup patch as part of your phase 2 series to add active
image commit support is fine by me.

-- 
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 --]