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