Re: [Qemu-devel] [PATCH v2 09/11] block: add ability for block-stream to use node-name

[Jeff Cody]

On Tue, May 27, 2014 at 02:06:00PM -0600, Eric Blake wrote:
> On 05/27/2014 08:28 AM, Jeff Cody wrote:
> > This adds the ability for block-stream to use node-name arguments
> > for base, to specify the backing image to stream from.
> > 
> > Both 'base' and 'base-node-name' are optional, but mutually exclusive.
> > Either can be specified, but not both together.
> > 
> > The argument for "device" is now optional as well, so long as
> > base-node-name is specified.  From the node-name, we can determine
> > the full device chain.
> > 
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  blockdev.c       | 62 
> > ++++++++++++++++++++++++++++++++++++++++++++++----------
> >  hmp-commands.hx  |  2 +-
> >  hmp.c            | 10 +++++----
> >  qapi-schema.json | 15 ++++++++++----
> >  qmp-commands.hx  |  2 +-
> >  5 files changed, 70 insertions(+), 21 deletions(-)
> > 
> 
> > +
> > +    if (!has_base_node_name && !has_device) {
> > +        error_setg(errp, "'device' required if 'base-node-name' not 
> > specified");
> > +        return;
> > +    }
> > +
> > +
> > +    if (has_device) {
> 
> Is the double blank line intentional?
> 

No... my pinky got carried away :)

> > @@ -1893,15 +1923,25 @@ void qmp_block_stream(const char *device, bool 
> > has_base,
> >          return;
> >      }
> >  
> > -    if (base) {
> > +    if (has_base) {
> >          base_bs = bdrv_find_backing_image(bs, base);
> 
> Hmm - another pre-existing place where the old code was RELYING on null
> initialization (see my complaints in 6/11); but this time, there is no
> earlier patch to where we can hoist this (ivory tower) fix :)
> 
> > +    /* Verify that 'base' is in the same chain as 'top', if 'base' was
> > +     * specified */
> > +    if (base_bs && !bdrv_chain_contains(bs, base_bs)) {
> > +        error_setg(errp, "'%s' and 'top' are not in the same chain", 
> > device);
> 
> Error message copy and paste?  Mentioning 'top' sounds odd, when the
> message is about 'base' not being found in the chain.

Yes, thanks - that should be 'base' not 'top'.
> 
> On the surface: Pedantically, 'device' may be uninitialized here (we can
> get here when has_device is false), practically, it's no different than
> the other places where we rely on pointers being NULL-initialized.
> Either way, if device is not specified, that means you are relying on
> glibc's "(null)" extension for the %s, which is not nice.  Reading
> deeper: the earlier checks guarantee that if has_device is false, then
> 'base_node_name' was already required and 'bs' was determined by
> crawling up the chain, but if that is the case, then
> bdrv_chain_contains(bs, base_bs) will never fail.  So you lucked out and
> 'device' will always be a user-specified string if you reach this error
> message; although I doubt whether Coverity can see that.
> 

Yeah, it is probably worth throwing a ternary in the error message,
for clarity & Coverity.

> 
> > +++ b/hmp.c
> > @@ -1168,11 +1168,13 @@ void hmp_block_set_io_throttle(Monitor *mon, const 
> > QDict *qdict)
> >  void hmp_block_stream(Monitor *mon, const QDict *qdict)
> >  {
> >      Error *error = NULL;
> > -    const char *device = qdict_get_str(qdict, "device");
> > -    const char *base = qdict_get_try_str(qdict, "base");
> > -    int64_t speed = qdict_get_try_int(qdict, "speed", 0);
> > +    const char *device         = qdict_get_str(qdict, "device");
> > +    const char *base           = qdict_get_try_str(qdict, "base");
> 
> Now that 'device' is optional, should you be using qdict_get_try_str?
> 

Yes, definitely should, thanks.

> > +++ b/qapi-schema.json
> > @@ -2600,9 +2600,16 @@
> >  # On successful completion the image file is updated to drop the backing 
> > file
> >  # and the BLOCK_JOB_COMPLETED event is emitted.
> >  #
> > -# @device: the device name
> > +# @device:        #optional The device name.  Optional only if 
> > @base-node-name
> > +#                           is used.
> > +#
> > +# For 'base', either @base or @base-node-name may be set but not both. If
> > +# neither is specified, this is the deepest backing image
> 
> Wrong.  For block_stream, a NULL base means to change the active image
> to have no base at all.  That is, starting from:
> 
> base <- sn1 <- sn2
> 
> calling block_stream with 'base':'base' collapses to
> 
> base <- sn2
> 
> while calling it with base omitted collapses to
> 
> sn2
> 
> I think you want something more along the lines of:
> 
> If neither is specified, the entire chain will be streamed into the
> active image so that it no longer has a backing file.
>

You are right, it is not just streamimg from above the deepest backing
image, but inclusive of it, resulting in no more backing images at
all.  I'll use your wording above.