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

[Jeff Cody <jcody@redhat.com>]

On Thu, May 15, 2014 at 09:42:07AM -0600, Eric Blake wrote:
> 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.  

Those are a slightly different scenario, however.  In this patch, we
are dealing with a filename string, and a node-name string.  There is
more of a 1:1 relationship between those two (both identify a
particular image).  The other existing commands, however, deal with a
device name string (identifying a drive chain) or a node-name string
(identifying a particular image).

That said, I think the other commands should be modified to enforce
the documentation, but it doesn't really relate to this patch.

> 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 find this an odd function to begin with, because node_name uniquely
identifies any given BDS from any chain.  Device name will uniquely
identify a whole chain, by returning its top-most BDS.

The function seems confused.


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

I disagree on this part, at least partially.  It think it needs to be
changed, but I don't think it needs to be part of this series.  I
think that can be a separate series, to address that across the other
QAPI commands that accept node-name and device name


> 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 <jcody@redhat.com>
> > ---
> >  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.
>

It is not needed - it is not relevant for the active commit case (the
current check will catch that, since top_bs == bs).  And for the
non-active commit case, commit_start() will return an error if
'top_bs' is not in 'bs', so the case of 'base_bs' or 'top_bs' not
being in 'device' is already covered.

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

This conditional is just to determine if we are finding the backing
image referenced by the string "base" or just using the default
bottom-most base image.

> >  
> > +    /* 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...
>

OK, thanks.

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

OK, thanks.

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

OK.  I called it out explicitly, since it is currently different
behavior from the other functions.

> >  # 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 :)
> 

I had thought about making it optional, but I do think it is better as
mandatory - it makes sure the user knows which chain they are
operating on, so it acts as a (small) check against user mistakes.

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

OK, thanks.