Re: [Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit

[Eric Blake]

On 05/14/2014 09:20 PM, 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)

Hmm - we have a bug in existing commands for NOT forcing mutual
exclusion even though the schema says they are exclusive.  For example,
qmp_block_passwd() blindly calls:

    bs = bdrv_lookup_bs(has_device ? device : NULL,
                        has_node_name ? node_name : NULL,
                        &local_err);

and _that_ function blindly favors device name over node name, rather
than erroring out if both are supplied.  I think we should fix that
first - I'd rather that bdrv_lookup_bs either errors out if both names
are supplied (rather than making each caller repeat the work), or that
it checks that if both names are supplied then it resolves to the same bs.

Hmm - if I have device "foo" with chain "base <- top", would
bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that
resides within "device"'s chain) or an error ("base" resolves to a
different bs than "device")?  Again, all the more reason to have a
common function decide the semantics we want, then all other clients
automatically pick up on those semantics.

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

I like this addition.

> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c       | 35 +++++++++++++++++++++++++++++++++--
>  qapi-schema.json | 35 ++++++++++++++++++++++++++---------
>  qmp-commands.hx  | 29 ++++++++++++++++++++++-------
>  3 files changed, 81 insertions(+), 18 deletions(-)
> 



>  
> -    if (top) {
> +    /* Find the 'top' image file for the commit */
> +    if (has_top_node_name) {
> +        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);

Hmm.  Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned
by 'device'?  Later on, you verify that 'base_bs' belongs to 'top_bs',
but if I pass node names, those names could have found a top unrelated
to 'device'; and base related to that top.

Maybe that gives more weight to my idea of possibly allowing
bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even
when device and top_node_name resolve to different bs, but where
top_node_name is a node in the chain of device.

> 
> -    if (has_base && base) {
> +    /* Find the 'base' image file for the commit */
> +    if (has_base_node_name) {
> +        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_base && base) {

Here, it doesn't matter whether you pass device or NULL for the device
name...

>  
> +    /* Verify that 'base' is in the same chain as 'top' */
> +    if (!bdrv_is_in_chain(top_bs, base_bs)) {
> +        error_setg(errp, "'base' and 'top' are not in the same chain");
> +        return;

...because this check is still mandatory to prove that a user with chain:
base <- sn1 <- sn2 <- active

is not calling 'device':'active', 'top-node-name':'sn1',
'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that
base-node-name belongs to device, not that it _also_ belongs to top_bs).


> -# @base:   #optional The file name of the backing image to write data into.
> -#                    If not specified, this is the deepest backing image
> +# For 'base', either @base or @base-node-name may be set but not both. If
> +# neither is specified, this is the deepest backing image

I like how you factored th is out up front...

>  #
> -# @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 the active layer.
> +# @base:          #optional The file name of the backing image to write data
> +#                           into.
> +#
> +# @base-node-name #optional The name of the block driver state node of the
> +#                           backing image to write data into.
> +#                           (Since 2.1)
> +#
> +# For 'top', either @top or @top-node-name must be set but not both.

...but here, you were not consistent.  I'd add "If neither is specified,
this is the active image" here...

> +#
> +# @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 the
> +#                           active layer.

...and drop the second sentence from here.

> +#
> +# @top-node-name: #optional The block driver state node name of the backing
> +#                           image within the image chain, which contains the
> +#                           topmost data to be committed down.
> +#                           (Since 2.1)
>  #
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by 
> itself,
> @@ -2120,17 +2135,19 @@
>  #
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
> -#          If @device does not exist, DeviceNotFound
> +#          If @device does not exist or cannot be determined, DeviceNotFound
>  #          If image commit is not supported by this device, NotSupported
> -#          If @base or @top is invalid, a generic error is returned
> +#          If @base, @top, @base-node-name, @top-node-name invalid, 
> GenericError
>  #          If @speed is invalid, InvalidParameter
> +#          If both @base and @base-node-name are specified, GenericError
> +#          If both @top and @top-node-name are specified, GenericError

These last two are arguably sub-conditions of the earlier statement
about @top and @top-node-name being invalid (since being invalid can
include when both strings are used at once).  It wouldn't hurt my
feelings to reduce the docs by two lines.

>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
> -  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> -            '*speed': 'int' } }
> +  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> +            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }

I'm okay with keeping 'device' mandatory, even though technically the
use of a node-name could imply which device is intended.  That is, as
long as the code correctly errors out when device and top-node-name
don't resolve to the same chain :)


> +
> +For 'top', either top or top-node-name must be set but not both.
> +
> +- "top":            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 the active layer.
> +                    (json-string, optional)

Same blurb about hoisting the 'not specified => active' sentence to the
common text, rather than leaving it in the 'top' text.

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

signature.asc
Description: OpenPGP digital signature