[Qemu-block] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Alberto Garcia]

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 <address@hidden>
---
 blockdev.c           | 59 ++++++++++++++++++++++++++++++++++++++++++++++------
 hmp.c                |  2 +-
 qapi/block-core.json |  9 +++++++-
 qmp-commands.hx      |  3 ++-
 4 files changed, 64 insertions(+), 9 deletions(-)

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:
 
-- 
2.5.0