[Qemu-devel] [PATCH 18/22] qapi: Convert blockdev_snapshot_sync

[Luiz Capitulino]

Unfortunately, this conversion required an additional change.

In the old QMP command, the 'snapshot-file' argument is specified as
optional. The idea is to take the snapshot internally if 'snapshot-file'
is not passed. However, internal snapshots are not supported yet so
the command returns a MissingParamater error if 'snapshot-file' is not
passed. Which makes the argument actually required and will cause
compatibility breakage if we change that in the future.

To fix this the QAPI converted blockdev_snapshot_sync command makes the
'snapshot-file' argument required. Again, in practice it's actually required,
so this is not incompatible.

If we do implement internal snapshots someday, we'll need a new argument
for it.

Note that this discussion doesn't affect HMP.

Signed-off-by: Luiz Capitulino <address@hidden>
---
 blockdev.c       |   49 +++++++++++++++++--------------------------------
 blockdev.h       |    2 --
 hmp-commands.hx  |    2 +-
 hmp.c            |   19 +++++++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   29 +++++++++++++++++++++++++++++
 qmp-commands.hx  |    6 ++----
 7 files changed, 69 insertions(+), 39 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5270404..c832782 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -601,28 +601,20 @@ void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
-int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
+                                bool has_format, const char *format,
+                                Error **errp)
 {
-    const char *device = qdict_get_str(qdict, "device");
-    const char *filename = qdict_get_try_str(qdict, "snapshot-file");
-    const char *format = qdict_get_try_str(qdict, "format");
     BlockDriverState *bs;
     BlockDriver *drv, *old_drv, *proto_drv;
     int ret = 0;
     int flags;
     char old_filename[1024];
 
-    if (!filename) {
-        qerror_report(QERR_MISSING_PARAMETER, "snapshot-file");
-        ret = -1;
-        goto out;
-    }
-
     bs = bdrv_find(device);
     if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, device);
-        ret = -1;
-        goto out;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
     }
 
     pstrcpy(old_filename, sizeof(old_filename), bs->filename);
@@ -630,35 +622,34 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
     old_drv = bs->drv;
     flags = bs->open_flags;
 
-    if (!format) {
+    if (!has_format) {
         format = "qcow2";
     }
 
     drv = bdrv_find_format(format);
     if (!drv) {
-        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
-        ret = -1;
-        goto out;
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return;
     }
 
-    proto_drv = bdrv_find_protocol(filename);
+    proto_drv = bdrv_find_protocol(snapshot_file);
     if (!proto_drv) {
-        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
-        ret = -1;
-        goto out;
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return;
     }
 
-    ret = bdrv_img_create(filename, format, bs->filename,
+    ret = bdrv_img_create(snapshot_file, format, bs->filename,
                           bs->drv->format_name, NULL, -1, flags);
     if (ret) {
-        goto out;
+        error_set(errp, QERR_UNDEFINED_ERROR);
+        return;
     }
 
     bdrv_drain_all();
     bdrv_flush(bs);
 
     bdrv_close(bs);
-    ret = bdrv_open(bs, filename, flags, drv);
+    ret = bdrv_open(bs, snapshot_file, flags, drv);
     /*
      * If reopening the image file we just created fails, fall back
      * and try to re-open the original image. If that fails too, we
@@ -667,17 +658,11 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
     if (ret != 0) {
         ret = bdrv_open(bs, old_filename, flags, old_drv);
         if (ret != 0) {
-            qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+            error_set(errp, QERR_OPEN_FILE_FAILED, old_filename);
         } else {
-            qerror_report(QERR_OPEN_FILE_FAILED, filename);
+            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
         }
     }
-out:
-    if (ret) {
-        ret = -1;
-    }
-
-    return ret;
 }
 
 static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
diff --git a/blockdev.h b/blockdev.h
index b69a549..f1b6396 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -64,6 +64,4 @@ int do_change_block(Monitor *mon, const char *device,
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_set_io_throttle(Monitor *mon,
                              const QDict *qdict, QObject **ret_data);
-int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
-
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 734a364..131b21c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -840,7 +840,7 @@ ETEXI
                       "If format is specified, the snapshot file will\n\t\t\t"
                       "be created in that format. Otherwise the\n\t\t\t"
                       "snapshot will be internal! (currently unsupported)",
-        .mhandler.cmd_new = do_snapshot_blkdev,
+        .mhandler.cmd = hmp_snapshot_blkdev,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index db199b2..934e931 100644
--- a/hmp.c
+++ b/hmp.c
@@ -643,3 +643,22 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
     qmp_block_resize(device, size, &errp);
     hmp_handle_error(mon, &errp);
 }
+
+void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *filename = qdict_get_try_str(qdict, "snapshot-file");
+    const char *format = qdict_get_try_str(qdict, "format");
+    Error *errp = NULL;
+
+    if (!filename) {
+        /* In the future, if 'snapshot-file' is not specified, the snapshot
+           will be taken internally. Today it's actually required. */
+        error_set(&errp, QERR_MISSING_PARAMETER, "snapshot-file");
+        hmp_handle_error(mon, &errp);
+        return;
+    }
+
+    qmp_blockdev_snapshot_sync(device, filename, !!format, format, &errp);
+    hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index c236fd2..e4227d3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -45,5 +45,6 @@ void hmp_set_link(Monitor *mon, const QDict *qdict);
 void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index f33fe00..0ed39f3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1070,3 +1070,32 @@
 # Since: 0.14.0
 ##
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
+
+##
+# @blockdev-snapshot-sync
+#
+# Generates a synchronous snapshot of a block device.
+#
+# @device:  the name of the device to generate the snapshot from.
+#
+# @snapshot-file: the target of the new image. If the file exists, or if it
+#                 is a device, the snapshot will be created in the existing
+#                 file/device. If does not exist, a new file will be created.
+#
+# @format: #optional the format of the snapshot image, default is 'qcow2'.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @snapshot-file can't be opened, OpenFileFailed
+#          If @format is invalid, InvalidBlockFormat
+#
+# Notes: One of the last steps taken by this command is to close the current
+#        image being used by @device and open the @snapshot-file one. If that
+#        fails, the command will try to reopen the original image file. If
+#        that also fails OpenFileFailed will be returned and the guest may get
+#        unexpected errors.
+#
+# Since 0.14.0
+##
+{ 'command': 'blockdev-snapshot-sync',
+  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 721b157..9d68206 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -665,10 +665,8 @@ EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:B,snapshot-file:s?,format:s?",
-        .params     = "device [new-image-file] [format]",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_snapshot_blkdev,
+        .args_type  = "device:B,snapshot-file:s,format:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
     },
 
 SQMP
-- 
1.7.8.110.g4cb5d1.dirty