Re: [Qemu-devel] [PATCH v2 07/11] block: Accept node-name arguments for block-commit

[Eric Blake]

On 05/27/2014 08:28 AM, Jeff Cody wrote:
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 
> The filename and node-name are mutually exclusive to each other;
> i.e.:
>     "top" and "top-node-name" are mutually exclusive (enforced)
>     "base" and "base-node-name" are mutually exclusive (enforced)
> 
> The "device" argument now becomes optional as well, because with
> a node-name we can identify the block device chain.  It is only
> optional if a node-name is not specified.
> 
> The preferred and recommended method now of using block-commit
> is to specify the BDS to operate on via their node-name arguments.
> 
> This also adds an explicit check that base_bs is in the chain of
> top_bs, because if they are referenced by their unique node-name
> arguments, the discovery process does not intrinsically verify
> they are in the same chain.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c       | 83 
> +++++++++++++++++++++++++++++++++++++++++++++++++-------
>  qapi-schema.json | 38 +++++++++++++++++++-------
>  qmp-commands.hx  | 33 ++++++++++++++++------
>  3 files changed, 126 insertions(+), 28 deletions(-)
> 

> +    /* default top_bs is the active layer, if NULL */
> +    top_bs = top_bs ?: bs;
> +
> +    if (has_top && top) {
>          if (strcmp(bs->filename, top) != 0) {

Cool!  This part fixes the complaint I had in 6/11.  If you have a
reason to rebase, you could hoist this one-line if change into that
patch, while still keeping my R-b on both patches.

Everything else here looks sane (lots of conditions before starting, but
all of them made sense, and while there are five separate optional
arguments that can interplay, my read of your logic says you correctly
covered all the cases)

> +++ b/qapi-schema.json
> @@ -2097,14 +2097,30 @@
>  # 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
> +# @device:        #optional The name of the device.  Optional only if
> +#                           node-names are used for both base and top

More precisely, it is also optional if node-name is used for base and
top/top-node-name is omitted, or if node-name is used for top and
base/base-node-name is omitted; but that's splitting hairs, so keep your
current wording.


> +# For 'top', either @top or @top-node-name must be set but not both. If
> +# neither is specified, this is the active layer
> +#
> +# @top:           #optional The file name of the backing image within the 
> image
> +#                           chain, which contains the topmost data to be
> +#                           committed down.
> +#

We aren't consistent on whether to use a trailing '.'; if you have a
respin for other reasons, it's worth thinking about; but it's not a
cause for a respin by itself.

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature