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

[Eric Blake]

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?

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

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.


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

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

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

signature.asc
Description: OpenPGP digital signature