Re: [Qemu-devel] [PATCH v2 5/7] QAPI: add command for live block commit, 'block-commit'

[Eric Blake]

On 09/25/2012 10:29 AM, Jeff Cody wrote:
> 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. optional (if not specified,
>         it is one below the active image) - see note below

This says that for 'base' <- 'mid' <- 'active', omitting 'top' is the
same as specifying 'top':'mid'.

> speed:  maximum speed, in bytes/sec
> 
> note: eventually this will support merging down the active layer,
>       but that code is not yet complete.  If the active layer is passed
>       in currently as top, or top is left to the default, then an error
>       will be returned.

This says that for 'base' <- 'mid' <- 'active', omitting 'top' is an
error (because it would be the same as an explicit 'top:'active').

Let's check the code...

> +    if (top && has_top) {
> +        /* if we want to allow the active layer,
> +         * use 'bdrv_find_image()' here */
> +        top_bs = bdrv_find_backing_image(bs, top);
> +    } else {
> +        /* default is one below the active layer, unless that is
> +         * the base */
> +        top_bs = bs->backing_hd;

Aha - the former is correct as coded (you default to 'top':'mid' in my
example), so the 'note' in your commit message needs editing.

On the other hand, when we ever DO get around to adding live commit,
which is the saner default?  That is, am I more likely to want to do
live commit from 'active', or more likely to do commit of the layer
below 'active'?  If live commit is the more desirable case, then that
argues that THIS commit should always error out if !has_top, and that
the future patch will default top_bs = bs, and the rest of your commit
message and documentation would need tweaking accordingly.

I don't have a preference either way (libvirt can arrange to always pass
the top argument, and thus avoid the confusion when top is omitted), but
if anyone else cares, now is the time to get the API right before we are
locked in to it.

> +++ b/qapi-schema.json
> @@ -1468,6 +1468,41 @@
>    'returns': 'str' }
>  
>  ##
> +# @block-commit
> +#
> +# Live commit of data from overlay image nodes into backing nodes - i.e.,
> +# writes data between 'top' and 'base' into 'base'.
> +#
> +# @device:  the name of the device
> +#
> +# @base:   #optional The file name of the backing image to write data into.
> +#                    If not specified, this is the deepest backing image
> +#
> +# @top:    #optional The file name of the backing image within the image 
> chain,
> +#                    which contains the topmost data to be committed down.
> +#                    If not specified, this is one layer below the active
> +#                    layer (i.e. active->backing_hd).
> +#
> +#                    If top == base, that is an error.
> +#

This documentation of @top matches the first paragraph of your commit
message.

> +#
> +# @speed:  #optional the maximum speed, in bytes per second
> +#
> +# Returns: Nothing on success
> +#          If commit or stream is already active on this device, DeviceInUse
> +#          If @device does not exist, DeviceNotFound
> +#          If image commit is not supported by this device, NotSupported
> +#          If @base does not exist, a generic error is returned
> +#          If @top does not exist, a generic error is returned

These two lines could be merged, or even made more generic (for example,
based on the rest of this thread, you will also be erroring out if base
and top exist, but appear as swapped arguments):

If @base or @top is invalid, a generic error is returned

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature