| Message ID | 457103c2204e849aea3b83ffd78fad049d8d923d.1441014844.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 31.08.2015 12:00, Alberto Garcia wrote: > Snapshots created using blockdev-snapshot-sync are currently opened > using their default options, not even inheriting those from the images > they are based on. > > This patch extends the command by adding an 'options' parameter that > takes a BlockdevOptions structure. Since some of the options that can > be specified overlap with the parameters of blockdev-snapshot-sync, > the following checks are made: > > - The 'driver' field must match the format specified for the snapshot. > - The 'node-name' field and the 'snapshot-node-name' parameters cannot > be specified at the same time. > - The 'file' field can only contain an empty string, since it overlaps > with the 'snapshot-file' parameter. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++------ > hmp.c | 2 +- > qapi/block-core.json | 9 +++++++- > qmp-commands.hx | 3 ++- > 4 files changed, 64 insertions(+), 9 deletions(-) Design question: Would it make sense to instead add a "reference" mode to blockdev-snapshot-sync where you can specify a BDS's node-name instead of snapshot-file to use an existing BDS as the new top layer, ideally an empty one? What we'd then need is a QMP command for creating images. But as far as I know we'll need that anyway sooner or later... Comments on the patch itself below. > diff --git a/blockdev.c b/blockdev.c > index 4a1fc2e..7e904f3 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, > bool has_snapshot_node_name, > const char *snapshot_node_name, > bool has_format, const char *format, > - bool has_mode, NewImageMode mode, Error **errp) > + bool has_mode, NewImageMode mode, > + bool has_options, BlockdevOptions *options, > + Error **errp) > { > BlockdevSnapshot snapshot = { > .has_device = has_device, > @@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, > .format = (char *) format, > .has_mode = has_mode, > .mode = mode, > + .has_options = has_options, > + .options = options, > }; > blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC, > &snapshot, errp); > @@ -1504,6 +1508,48 @@ static void external_snapshot_prepare(BlkTransactionState *common, > > flags = state->old_bs->open_flags; > > + if (action->blockdev_snapshot_sync->has_options) { > + QObject *obj; > + QmpOutputVisitor *ov = qmp_output_visitor_new(); > + visit_type_BlockdevOptions(qmp_output_get_visitor(ov), > + &action->blockdev_snapshot_sync->options, > + NULL, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + qmp_output_visitor_cleanup(ov); > + return; > + } > + > + obj = qmp_output_get_qobject(ov); > + options = qobject_to_qdict(obj); > + qmp_output_visitor_cleanup(ov); > + > + if (strcmp(format, qdict_get_str(options, "driver"))) { With has_format == false, format will be set to "qcow2" by default. So, if the user does not specify the format explicitly, the "driver" field has to be set to "qcow2". I'd rather make specifying @format and @options exclusive, and if @options has been specified, its "driver" field should override the "format" default. > + error_setg(errp, "Can't specify two different image formats"); > + goto fail; > + } > + > + if (has_snapshot_node_name && qdict_haskey(options, "node-name")) { > + error_setg(errp, "Can't specify a node name twice"); > + goto fail; > + } > + > + obj = qdict_get(options, "file"); > + if (obj != NULL) { > + QString *str = qobject_to_qstring(obj); > + if (!str || strcmp(qstring_get_str(str), "")) { > + error_setg(errp, "The 'file' field must be empty"); > + goto fail; > + } > + qdict_del(options, "file"); > + } And the "backing" field must be NULL. > + > + qdict_flatten(options); > + } else { > + options = qdict_new(); > + qdict_put(options, "driver", qstring_from_str(format)); > + } > + > /* create new image w/backing file */ > if (mode != NEW_IMAGE_MODE_EXISTING) { > bdrv_img_create(new_image_file, format, > @@ -1512,19 +1558,15 @@ static void external_snapshot_prepare(BlkTransactionState *common, > NULL, -1, flags, &local_err, false); > if (local_err) { > error_propagate(errp, local_err); > - return; > + goto fail; > } > } > > - options = qdict_new(); > if (has_snapshot_node_name) { > qdict_put(options, "node-name", > qstring_from_str(snapshot_node_name)); > } > - qdict_put(options, "driver", qstring_from_str(format)); > > - /* TODO Inherit bs->options or only take explicit options with an > - * extended QMP command? */ > assert(state->new_bs == NULL); > ret = bdrv_open(&state->new_bs, new_image_file, NULL, options, > flags | BDRV_O_NO_BACKING, &local_err); > @@ -1532,6 +1574,11 @@ static void external_snapshot_prepare(BlkTransactionState *common, > if (ret != 0) { > error_propagate(errp, local_err); > } > + > + return; > + > +fail: > + QDECREF(options); > } > > static void external_snapshot_commit(BlkTransactionState *common) > diff --git a/hmp.c b/hmp.c > index dcc66f1..180a7f6 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) > qmp_blockdev_snapshot_sync(true, device, false, NULL, > filename, false, NULL, > !!format, format, > - true, mode, &err); > + true, mode, false, NULL, &err); > hmp_handle_error(mon, &err); > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7b2efb8..5eb111e 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -697,11 +697,18 @@ > # > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. > +# > +# @options: #optional options for the new device, with the following > +# restrictions for the fields: 'driver' must match the value > +# of @format, As said above, I'd rather make specifying both @options and @format exclusive. Maybe there is even some QAPI magic to enforce that (and for 'node-name', too), I don't know... Max > 'node-name' and @snapshot-node-name cannot be > +# specified at the same time, and 'file' can only contain an > +# empty string (Since 2.5) > ## > { 'struct': 'BlockdevSnapshot', > 'data': { '*device': 'str', '*node-name': 'str', > 'snapshot-file': 'str', '*snapshot-node-name': 'str', > - '*format': 'str', '*mode': 'NewImageMode' } } > + '*format': 'str', '*mode': 'NewImageMode', > + '*options': 'BlockdevOptions' } } > > ## > # @DriveBackup > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ba630b1..bf45e31 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1394,7 +1394,7 @@ EQMP > > { > .name = "blockdev-snapshot-sync", > - .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", > + .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?,options:q?", > .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, > }, > > @@ -1417,6 +1417,7 @@ Arguments: > - "mode": whether and how QEMU should create the snapshot file > (NewImageMode, optional, default "absolute-paths") > - "format": format of new image (json-string, optional) > +- "options": options for the new image (json-dict) > > Example: > >
On 08/31/2015 01:53 PM, Max Reitz wrote: > Design question: Would it make sense to instead add a "reference" mode > to blockdev-snapshot-sync where you can specify a BDS's node-name > instead of snapshot-file to use an existing BDS as the new top layer, > ideally an empty one? Indeed - then blockdev-add can be used to create an unattached BDS (with all appropriate options), and blockdev-snapshot-sync would then attach that BDS as the snapshot-file that wraps an existing BDS (without needing to worry about options). > > What we'd then need is a QMP command for creating images. But as far as > I know we'll need that anyway sooner or later... Can't blockdev-add already be used for that (at least, for supported file types)? If not, what would it take to get it there? > > > Comments on the patch itself below. > > > With has_format == false, format will be set to "qcow2" by default. So, > if the user does not specify the format explicitly, the "driver" field > has to be set to "qcow2". > > I'd rather make specifying @format and @options exclusive, and if > @options has been specified, its "driver" field should override the > "format" default. > >> +++ b/qapi/block-core.json >> @@ -697,11 +697,18 @@ >> # >> # @mode: #optional whether and how QEMU should create a new image, default is >> # 'absolute-paths'. >> +# >> +# @options: #optional options for the new device, with the following >> +# restrictions for the fields: 'driver' must match the value >> +# of @format, > > As said above, I'd rather make specifying both @options and @format > exclusive. > > Maybe there is even some QAPI magic to enforce that (and for > 'node-name', too), I don't know... Not that I know of at the moment, but not to say we can't add some. The closest we can get is with a flat union, but that requires a non-optional discriminator field. Maybe we can tweak qapi to make the discriminator optional (with a default value). Thankfully, it sounds like Markus' work on introspection would at least let management apps learn about a new 'options' argument. >> 'node-name' and @snapshot-node-name cannot be >> +# specified at the same time, and 'file' can only contain an >> +# empty string (Since 2.5) I really don't like the idea of requiring the user to pass in an empty string. Can we make the field optional instead, and require it to be omitted in the context where it would otherwise need to be empty, while still requiring it to be present in existing clients that weren't prepared for it to be optional? It also feels a bit ad hoc that you describe that driver/format must be identical, but that other fields must be mutually exclusive or the empty string; consistency would argue that since you already handle duplication of driver/format by requiring them to be the same, that you should also allow duplication and validation of identical other fields that overlap. (But even better would be finding a way to not allow overlapping fields to appear in the first place).
On 31.08.2015 22:05, Eric Blake wrote: > On 08/31/2015 01:53 PM, Max Reitz wrote: > >> Design question: Would it make sense to instead add a "reference" mode >> to blockdev-snapshot-sync where you can specify a BDS's node-name >> instead of snapshot-file to use an existing BDS as the new top layer, >> ideally an empty one? > > Indeed - then blockdev-add can be used to create an unattached BDS (with > all appropriate options), and blockdev-snapshot-sync would then attach > that BDS as the snapshot-file that wraps an existing BDS (without > needing to worry about options). > >> >> What we'd then need is a QMP command for creating images. But as far as >> I know we'll need that anyway sooner or later... > > Can't blockdev-add already be used for that (at least, for supported > file types)? If not, what would it take to get it there? It would take a blockdev-create-image QMP command. :-) blockdev-add only opens existing images, blockdev-create-image would then create these so they can be opened using blockdev-add. Similar to blockdev-add, it would probably have a single parameter, but it'd be of a different type, called e.g. BlockdevCreateOptions, since it has to reflect the creation options instead of the runtime options for opening existing images. For instance, for qcow2 you could set the refcount-bits value, but not the L2 cache size. Max
Am 31.08.2015 um 22:05 hat Eric Blake geschrieben: > On 08/31/2015 01:53 PM, Max Reitz wrote: > > > Design question: Would it make sense to instead add a "reference" mode > > to blockdev-snapshot-sync where you can specify a BDS's node-name > > instead of snapshot-file to use an existing BDS as the new top layer, > > ideally an empty one? > > Indeed - then blockdev-add can be used to create an unattached BDS (with > all appropriate options), and blockdev-snapshot-sync would then attach > that BDS as the snapshot-file that wraps an existing BDS (without > needing to worry about options). Yes, this is what we should do. The existing blockdev-snapshot-sync should really have been called something like drive-snapshot, it doesn't belong in the blockdev-* family of commands which works only with existing nodes. > >> +++ b/qapi/block-core.json > >> @@ -697,11 +697,18 @@ > >> # > >> # @mode: #optional whether and how QEMU should create a new image, default is > >> # 'absolute-paths'. > >> +# > >> +# @options: #optional options for the new device, with the following > >> +# restrictions for the fields: 'driver' must match the value > >> +# of @format, > > > > As said above, I'd rather make specifying both @options and @format > > exclusive. > > > > Maybe there is even some QAPI magic to enforce that (and for > > 'node-name', too), I don't know... > > Not that I know of at the moment, but not to say we can't add some. The > closest we can get is with a flat union, but that requires a > non-optional discriminator field. Maybe we can tweak qapi to make the > discriminator optional (with a default value). Thankfully, it sounds > like Markus' work on introspection would at least let management apps > learn about a new 'options' argument. Let's avoid such magic and instead add a new, clean blockdev-* style command. Maybe call it simply blockdev-snapshot; the -sync part was added because we knew it wouldn't be the final version of the command. Now we don't have any bdrv_open() in it any more that could by synchronous or asynchronous. Kevin
Am 31.08.2015 um 22:12 hat Max Reitz geschrieben: > On 31.08.2015 22:05, Eric Blake wrote: > > On 08/31/2015 01:53 PM, Max Reitz wrote: > > > >> Design question: Would it make sense to instead add a "reference" mode > >> to blockdev-snapshot-sync where you can specify a BDS's node-name > >> instead of snapshot-file to use an existing BDS as the new top layer, > >> ideally an empty one? > > > > Indeed - then blockdev-add can be used to create an unattached BDS (with > > all appropriate options), and blockdev-snapshot-sync would then attach > > that BDS as the snapshot-file that wraps an existing BDS (without > > needing to worry about options). > > > >> > >> What we'd then need is a QMP command for creating images. But as far as > >> I know we'll need that anyway sooner or later... > > > > Can't blockdev-add already be used for that (at least, for supported > > file types)? If not, what would it take to get it there? > > It would take a blockdev-create-image QMP command. :-) > > blockdev-add only opens existing images, blockdev-create-image would > then create these so they can be opened using blockdev-add. > > Similar to blockdev-add, it would probably have a single parameter, but > it'd be of a different type, called e.g. BlockdevCreateOptions, since it > has to reflect the creation options instead of the runtime options for > opening existing images. For instance, for qcow2 you could set the > refcount-bits value, but not the L2 cache size. Would be nice to have (especially because we would get a schema of the create options), but not absolutely necessary for a blockdev-* style snapshotting command. libvirt seems to cope just fine with calling qemu-img before going to the QMP monitor. Kevin
On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote: >> > Design question: Would it make sense to instead add a "reference" >> > mode to blockdev-snapshot-sync where you can specify a BDS's >> > node-name instead of snapshot-file to use an existing BDS as the >> > new top layer, ideally an empty one? >> >> Indeed - then blockdev-add can be used to create an unattached BDS >> (with all appropriate options), and blockdev-snapshot-sync would then >> attach that BDS as the snapshot-file that wraps an existing BDS >> (without needing to worry about options). > > Yes, this is what we should do. Sounds like a good idea, thanks for the feedback ! >> >> +# @options: #optional options for the new device, with the following >> >> +# restrictions for the fields: 'driver' must match the value >> >> +# of @format, >> > >> > As said above, I'd rather make specifying both @options and @format >> > exclusive. >> > >> > Maybe there is even some QAPI magic to enforce that (and for >> > 'node-name', too), I don't know... >> >> Not that I know of at the moment, but not to say we can't add some. The >> closest we can get is with a flat union, but that requires a >> non-optional discriminator field. Maybe we can tweak qapi to make the >> discriminator optional (with a default value). Thankfully, it sounds >> like Markus' work on introspection would at least let management apps >> learn about a new 'options' argument. This is not necessary if we go for the "reference" mode proposed by Markus, since the "options" parameter would disappear. > Let's avoid such magic and instead add a new, clean blockdev-* style > command. Maybe call it simply blockdev-snapshot; the -sync part was > added because we knew it wouldn't be the final version of the command. > Now we don't have any bdrv_open() in it any more that could by > synchronous or asynchronous. Would you then prefer me to create a new command instead of extending the existing one? What would be the benefit (other than a better name)? Berto
Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben: > On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote: > > >> > Design question: Would it make sense to instead add a "reference" > >> > mode to blockdev-snapshot-sync where you can specify a BDS's > >> > node-name instead of snapshot-file to use an existing BDS as the > >> > new top layer, ideally an empty one? > >> > >> Indeed - then blockdev-add can be used to create an unattached BDS > >> (with all appropriate options), and blockdev-snapshot-sync would then > >> attach that BDS as the snapshot-file that wraps an existing BDS > >> (without needing to worry about options). > > > > Yes, this is what we should do. > > Sounds like a good idea, thanks for the feedback ! > > >> >> +# @options: #optional options for the new device, with the following > >> >> +# restrictions for the fields: 'driver' must match the value > >> >> +# of @format, > >> > > >> > As said above, I'd rather make specifying both @options and @format > >> > exclusive. > >> > > >> > Maybe there is even some QAPI magic to enforce that (and for > >> > 'node-name', too), I don't know... > >> > >> Not that I know of at the moment, but not to say we can't add some. The > >> closest we can get is with a flat union, but that requires a > >> non-optional discriminator field. Maybe we can tweak qapi to make the > >> discriminator optional (with a default value). Thankfully, it sounds > >> like Markus' work on introspection would at least let management apps > >> learn about a new 'options' argument. > > This is not necessary if we go for the "reference" mode proposed by > Markus, since the "options" parameter would disappear. > > > Let's avoid such magic and instead add a new, clean blockdev-* style > > command. Maybe call it simply blockdev-snapshot; the -sync part was > > added because we knew it wouldn't be the final version of the command. > > Now we don't have any bdrv_open() in it any more that could by > > synchronous or asynchronous. > > Would you then prefer me to create a new command instead of extending > the existing one? What would be the benefit (other than a better name)? A clean interface. There is really little overlap with what we have: { 'struct': 'BlockdevSnapshot', 'data': { '*device': 'str', '*node-name': 'str', 'snapshot-file': 'str', '*snapshot-node-name': 'str', '*format': 'str', '*mode': 'NewImageMode' } } When you add an existing node which has been created with blockdev-add as a snapshot, you won't use snapshot-file, snapshot-node-name, format and mode. We would either have to make all of them optional and actually forbid them when a reference is given, or to ensure that they are consistent with the already existing node. That we have both device and node-name (and both marked as optional, while one of them is required) is also not in line with our current practise. If we went further that way, the schema wouldn't really be expressive any more because there would be too many hidden rules of which combinations are allowed and which aren't. What you really need for the version with a reference is just: { 'struct': 'BlockdevSnapshot', 'data': { 'device': 'str', 'snapshot': 'str' } } Where both arguments refer to a node by node-name or backend name. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben: [...] >> Would you then prefer me to create a new command instead of extending >> the existing one? What would be the benefit (other than a better name)? > > A clean interface. There is really little overlap with what we have: > > { 'struct': 'BlockdevSnapshot', > 'data': { '*device': 'str', '*node-name': 'str', > 'snapshot-file': 'str', '*snapshot-node-name': 'str', > '*format': 'str', '*mode': 'NewImageMode' } } > > When you add an existing node which has been created with blockdev-add > as a snapshot, you won't use snapshot-file, snapshot-node-name, format > and mode. We would either have to make all of them optional and actually > forbid them when a reference is given, or to ensure that they are > consistent with the already existing node. That we have both device and > node-name (and both marked as optional, while one of them is required) is > also not in line with our current practise. > > If we went further that way, the schema wouldn't really be expressive > any more because there would be too many hidden rules of which > combinations are allowed and which aren't. Yes, and let me elaborate a bit. QMP permits evolving stuff compatibly. We use this capability to keep things simple and short when we add functionality: we extend existing stuff instead of duplicating and modifying it. Say, add detail to a query result, or add an optional command parameter that modifies behavior. The question to ask is whether the single, extended interface is still simpler than a new interface plus the unchanged old one. I expect introspection to influence our understanding of "simple interface" going forward. Stuff visible in introspection is usually simple. Rules of use introspection can't show are often problematic. [...]
On Tue 01 Sep 2015 04:40:02 PM CEST, Kevin Wolf wrote: >> > Let's avoid such magic and instead add a new, clean blockdev-* >> > style command. Maybe call it simply blockdev-snapshot; the -sync >> > part was added because we knew it wouldn't be the final version of >> > the command. Now we don't have any bdrv_open() in it any more that >> > could by synchronous or asynchronous. Ok, I have a first working prototype of the new command using references as suggested. > { 'struct': 'BlockdevSnapshot', > 'data': { '*device': 'str', '*node-name': 'str', > 'snapshot-file': 'str', '*snapshot-node-name': 'str', > '*format': 'str', '*mode': 'NewImageMode' } } [...] > > What you really need for the version with a reference is just: > > { 'struct': 'BlockdevSnapshot', > 'data': { 'device': 'str', 'snapshot': 'str' } } And what do I do with the old BlockdevSnapshot? I guess it can just be renamed to BlockdevSnapshotSync or something like that? Berto
On 09/02/2015 08:23 AM, Alberto Garcia wrote: > On Tue 01 Sep 2015 04:40:02 PM CEST, Kevin Wolf wrote: > >>>> Let's avoid such magic and instead add a new, clean blockdev-* >>>> style command. Maybe call it simply blockdev-snapshot; the -sync >>>> part was added because we knew it wouldn't be the final version of >>>> the command. Now we don't have any bdrv_open() in it any more that >>>> could by synchronous or asynchronous. > > Ok, I have a first working prototype of the new command using > references as suggested. > >> { 'struct': 'BlockdevSnapshot', >> 'data': { '*device': 'str', '*node-name': 'str', >> 'snapshot-file': 'str', '*snapshot-node-name': 'str', >> '*format': 'str', '*mode': 'NewImageMode' } } > [...] >> >> What you really need for the version with a reference is just: >> >> { 'struct': 'BlockdevSnapshot', >> 'data': { 'device': 'str', 'snapshot': 'str' } } > > And what do I do with the old BlockdevSnapshot? I guess it can just be > renamed to BlockdevSnapshotSync or something like that? Sure. And one of the nice things with the ongoing introspection work is that qapi type names are NOT exposed as ABI (changing the name of a type affects the generated C code, but does not affect what a QMP client sends over the wire), so it is safe to do.
diff --git a/blockdev.c b/blockdev.c index 4a1fc2e..7e904f3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, bool has_snapshot_node_name, const char *snapshot_node_name, bool has_format, const char *format, - bool has_mode, NewImageMode mode, Error **errp) + bool has_mode, NewImageMode mode, + bool has_options, BlockdevOptions *options, + Error **errp) { BlockdevSnapshot snapshot = { .has_device = has_device, @@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, .format = (char *) format, .has_mode = has_mode, .mode = mode, + .has_options = has_options, + .options = options, }; blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC, &snapshot, errp); @@ -1504,6 +1508,48 @@ static void external_snapshot_prepare(BlkTransactionState *common, flags = state->old_bs->open_flags; + if (action->blockdev_snapshot_sync->has_options) { + QObject *obj; + QmpOutputVisitor *ov = qmp_output_visitor_new(); + visit_type_BlockdevOptions(qmp_output_get_visitor(ov), + &action->blockdev_snapshot_sync->options, + NULL, &local_err); + if (local_err) { + error_propagate(errp, local_err); + qmp_output_visitor_cleanup(ov); + return; + } + + obj = qmp_output_get_qobject(ov); + options = qobject_to_qdict(obj); + qmp_output_visitor_cleanup(ov); + + if (strcmp(format, qdict_get_str(options, "driver"))) { + error_setg(errp, "Can't specify two different image formats"); + goto fail; + } + + if (has_snapshot_node_name && qdict_haskey(options, "node-name")) { + error_setg(errp, "Can't specify a node name twice"); + goto fail; + } + + obj = qdict_get(options, "file"); + if (obj != NULL) { + QString *str = qobject_to_qstring(obj); + if (!str || strcmp(qstring_get_str(str), "")) { + error_setg(errp, "The 'file' field must be empty"); + goto fail; + } + qdict_del(options, "file"); + } + + qdict_flatten(options); + } else { + options = qdict_new(); + qdict_put(options, "driver", qstring_from_str(format)); + } + /* create new image w/backing file */ if (mode != NEW_IMAGE_MODE_EXISTING) { bdrv_img_create(new_image_file, format, @@ -1512,19 +1558,15 @@ static void external_snapshot_prepare(BlkTransactionState *common, NULL, -1, flags, &local_err, false); if (local_err) { error_propagate(errp, local_err); - return; + goto fail; } } - options = qdict_new(); if (has_snapshot_node_name) { qdict_put(options, "node-name", qstring_from_str(snapshot_node_name)); } - qdict_put(options, "driver", qstring_from_str(format)); - /* TODO Inherit bs->options or only take explicit options with an - * extended QMP command? */ assert(state->new_bs == NULL); ret = bdrv_open(&state->new_bs, new_image_file, NULL, options, flags | BDRV_O_NO_BACKING, &local_err); @@ -1532,6 +1574,11 @@ static void external_snapshot_prepare(BlkTransactionState *common, if (ret != 0) { error_propagate(errp, local_err); } + + return; + +fail: + QDECREF(options); } static void external_snapshot_commit(BlkTransactionState *common) diff --git a/hmp.c b/hmp.c index dcc66f1..180a7f6 100644 --- a/hmp.c +++ b/hmp.c @@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) qmp_blockdev_snapshot_sync(true, device, false, NULL, filename, false, NULL, !!format, format, - true, mode, &err); + true, mode, false, NULL, &err); hmp_handle_error(mon, &err); } diff --git a/qapi/block-core.json b/qapi/block-core.json index 7b2efb8..5eb111e 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -697,11 +697,18 @@ # # @mode: #optional whether and how QEMU should create a new image, default is # 'absolute-paths'. +# +# @options: #optional options for the new device, with the following +# restrictions for the fields: 'driver' must match the value +# of @format, 'node-name' and @snapshot-node-name cannot be +# specified at the same time, and 'file' can only contain an +# empty string (Since 2.5) ## { 'struct': 'BlockdevSnapshot', 'data': { '*device': 'str', '*node-name': 'str', 'snapshot-file': 'str', '*snapshot-node-name': 'str', - '*format': 'str', '*mode': 'NewImageMode' } } + '*format': 'str', '*mode': 'NewImageMode', + '*options': 'BlockdevOptions' } } ## # @DriveBackup diff --git a/qmp-commands.hx b/qmp-commands.hx index ba630b1..bf45e31 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1394,7 +1394,7 @@ EQMP { .name = "blockdev-snapshot-sync", - .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", + .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?,options:q?", .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, }, @@ -1417,6 +1417,7 @@ Arguments: - "mode": whether and how QEMU should create the snapshot file (NewImageMode, optional, default "absolute-paths") - "format": format of new image (json-string, optional) +- "options": options for the new image (json-dict) Example:
Snapshots created using blockdev-snapshot-sync are currently opened using their default options, not even inheriting those from the images they are based on. This patch extends the command by adding an 'options' parameter that takes a BlockdevOptions structure. Since some of the options that can be specified overlap with the parameters of blockdev-snapshot-sync, the following checks are made: - The 'driver' field must match the format specified for the snapshot. - The 'node-name' field and the 'snapshot-node-name' parameters cannot be specified at the same time. - The 'file' field can only contain an empty string, since it overlaps with the 'snapshot-file' parameter. Signed-off-by: Alberto Garcia <berto@igalia.com> --- blockdev.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++------ hmp.c | 2 +- qapi/block-core.json | 9 +++++++- qmp-commands.hx | 3 ++- 4 files changed, 64 insertions(+), 9 deletions(-)