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