| Message ID | 84bf2d4f8b69a5c4638c0db89e555363b7e4b0d4.1401200583.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jcody@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.
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 <jcody@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? > 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.
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 <jcody@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(-) Revisiting the HMP side of things: > +++ b/hmp-commands.hx > @@ -76,7 +76,7 @@ ETEXI > > { > .name = "block_stream", > - .args_type = "device:B,speed:o?,base:s?", > + .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?", The HMP parser is trash. It requires arguments to appear in declaration order, and in order to specify an optional argument at the end of the list, you must specify all earlier arguments. Your proposed change is unworkable, because the ONLY way HMP can supply base-node-name is if the command line already supplied a base argument, but base and base-node-name are incompatible. I'm not quite sure if David's idea of allowing an optional named argument would help: https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg05914.html The idea being that you'd write .args_type = "device:B?,speed:o?,base-node-name:+n,base:s?" where the command is then invoked as either: block_stream dev 0 base # resolve base as a file name or as: block_stream -n base # resolve base as a node name, implicit dev block_stream dev -n base # resolve base as a node name within dev But you can see why I suggested earlier that maybe it's better to just forget about HMP entirely, and make this series focus on QMP (no need to stall the feature addition on the HMP warts, when backports only care about the QMP feature); save the HMP changes for another day (if ever).
On Wed, May 28, 2014 at 06:42:24AM -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 <jcody@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(-) > > Revisiting the HMP side of things: > > > +++ b/hmp-commands.hx > > @@ -76,7 +76,7 @@ ETEXI > > > > { > > .name = "block_stream", > > - .args_type = "device:B,speed:o?,base:s?", > > + .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?", > > The HMP parser is trash. It requires arguments to appear in declaration > order, and in order to specify an optional argument at the end of the > list, you must specify all earlier arguments. Your proposed change is > unworkable, because the ONLY way HMP can supply base-node-name is if the > command line already supplied a base argument, but base and > base-node-name are incompatible. > > I'm not quite sure if David's idea of allowing an optional named > argument would help: > https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg05914.html > > The idea being that you'd write > .args_type = "device:B?,speed:o?,base-node-name:+n,base:s?" > > where the command is then invoked as either: > > block_stream dev 0 base # resolve base as a file name > > or as: > > block_stream -n base # resolve base as a node name, implicit dev > block_stream dev -n base # resolve base as a node name within dev > > But you can see why I suggested earlier that maybe it's better to just > forget about HMP entirely, and make this series focus on QMP (no need to > stall the feature addition on the HMP warts, when backports only care > about the QMP feature); save the HMP changes for another day (if ever). > I agree with not worrying about HMP in this series; the new functionality is targeted towards a management software layer (e.g. libvirt). As you said, if it is desired for some reason, it can be added later.
diff --git a/blockdev.c b/blockdev.c index 983c7da..a4468b4 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1870,22 +1870,52 @@ static void block_job_cb(void *opaque, int ret) bdrv_put_ref_bh_schedule(bs); } -void qmp_block_stream(const char *device, bool has_base, - const char *base, bool has_speed, int64_t speed, +void qmp_block_stream(bool has_device, const char *device, + bool has_base, const char *base, + bool has_base_node_name, const char *base_node_name, + bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, Error **errp) { - BlockDriverState *bs; + BlockDriverState *bs = NULL; BlockDriverState *base_bs = NULL; Error *local_err = NULL; + const char *base_name; if (!has_on_error) { on_error = BLOCKDEV_ON_ERROR_REPORT; } - bs = bdrv_find(device); + if (has_base && has_base_node_name) { + error_setg(errp, "'base' and 'base-node-name' are mutually exclusive"); + return; + } + + if (!has_base_node_name && !has_device) { + error_setg(errp, "'device' required if 'base-node-name' not specified"); + return; + } + + + if (has_device) { + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + } + + 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; + } + bs = bs ?: bdrv_find_active(base_bs); + } + if (!bs) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + error_setg(errp, "Could not find active layer"); return; } @@ -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); - if (base_bs == NULL) { - error_set(errp, QERR_BASE_NOT_FOUND, base); - return; - } } - stream_start(bs, base_bs, base, has_speed ? speed : 0, + if (base_bs == NULL && (has_base || has_base_node_name)) { + error_set(errp, QERR_BASE_NOT_FOUND, base); + return; + } + + base_name = base_bs ? base_bs->filename : NULL; + + /* 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); + return; + } + + stream_start(bs, base_bs, base_name, has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err); if (local_err) { error_propagate(errp, local_err); diff --git a/hmp-commands.hx b/hmp-commands.hx index 2e462c0..17eda87 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -76,7 +76,7 @@ ETEXI { .name = "block_stream", - .args_type = "device:B,speed:o?,base:s?", + .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?", .params = "device [speed [base]]", .help = "copy data from a backing file into a block device", .mhandler.cmd = hmp_block_stream, diff --git a/hmp.c b/hmp.c index ccc35d4..122aa29 100644 --- a/hmp.c +++ 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"); + const char *base_node_name = qdict_get_try_str(qdict, "base_node_name"); + int64_t speed = qdict_get_try_int(qdict, "speed", 0); - qmp_block_stream(device, base != NULL, base, + qmp_block_stream(device != NULL, device, base != NULL, base, + base_node_name != NULL, base_node_name, qdict_haskey(qdict, "speed"), speed, true, BLOCKDEV_ON_ERROR_REPORT, &error); diff --git a/qapi-schema.json b/qapi-schema.json index f790e9a..63e74c5 100644 --- a/qapi-schema.json +++ 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 +# +# @base: #optional the common backing file name # -# @base: #optional the common backing file name +# @base-node-name: #optional the block driver state node name of the +# common backing file. (Since 2.1) # # @speed: #optional the maximum speed, in bytes per second # @@ -2616,8 +2623,8 @@ # Since: 1.1 ## { 'command': 'block-stream', - 'data': { 'device': 'str', '*base': 'str', '*speed': 'int', - '*on-error': 'BlockdevOnError' } } + 'data': { '*device': 'str', '*base': 'str', '*base-node-name': 'str', + '*speed': 'int', '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed: diff --git a/qmp-commands.hx b/qmp-commands.hx index f4fafda..903c63a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -979,7 +979,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,speed:o?,on-error:s?", + .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
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 <jcody@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(-)