Allow passing BlockdevOptions to blockdev-snapshot-sync

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 <[email protected]>

---
 blockdev.c           | 59 ++++++++++++++++++++++++++++++++++++++++++++++------
 hmp.c                |  2 +-
 qapi/block-core.json |  9 +++++++-
 qmp-commands.hx      |  3 ++-
 4 files changed, 64 insertions(+), 9 deletions(-)

blockdev.c +12 -1

index 4a1fc2e..7e904f3 100644
@@ -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) {

blockdev.c +8 -0

+            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;
+        }

blockdev.c +13 -5

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

hmp.c +1 -1

index dcc66f1..180a7f6 100644
@@ -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);
 }
 

qapi/block-core.json +0 -0

index 7b2efb8..5eb111e 100644

qapi/block-core.json +4 -1

                         '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

qmp-commands.hx +2 -1

index ba630b1..bf45e31 100644
@@ -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:
 

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.

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.

qapi/block-core.json line 703

Quoted 9 lines

+++ b/qapi/block-core.json

qapi/block-core.json +4 -0

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

qapi/block-core.json line 705

Quoted 3 lines

qapi/block-core.json +2 -0

                         'node-name' and @snapshot-node-name cannot be
+#           specified at the same time, and 'file' can only contain an
+#           empty string (Since 2.5)

Quoted 4 lines · Max Reitz

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

Quoted 1 line

Quoted 2 lines

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?

Quoted 5 lines

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

Quoted 13 lines

qapi/block-core.json line 703

Quoted 9 lines

+++ b/qapi/block-core.json

qapi/block-core.json +4 -0

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

Quoted 2 lines

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.

Quoted 13 lines · Max Reitz

Quoted 5 lines

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

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 9 lines

qapi/block-core.json

Quoted 3 lines

qapi/block-core.json +3 -0

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

Quoted 6 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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?

Quoted 6 lines

Quoted 4 lines

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 !

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

qapi/block-core.json

Quoted 3 lines

qapi/block-core.json +3 -0

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 5 lines

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

Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben:

Quoted 2 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

{ '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' } }

Quoted 5 lines · Kevin Wolf

Quoted 5 lines

Quoted 5 lines

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.

Quoted 4 lines

{ 'struct': 'BlockdevSnapshot',
'data': { '*device': 'str', '*node-name': 'str',

            'snapshot-file': 'str', '*snapshot-node-name': 'str',
            '*format': 'str', '*mode': 'NewImageMode' } }

[...]

Quoted 1 line

Quoted 4 lines

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?