[PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups

[PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups

[Maxim Levitsky]

This patch series is bunch of cleanups
to the hmp monitor code.

This series only touched blockdev related hmp handlers.

No functional changes expected other that
light error message changes by the last patch.

This was inspired by this bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1719169

Basically some users still parse hmp error messages,
and they would like to have them prefixed with 'Error:'

In commit 66363e9a43f649360a3f74d2805c9f864da027eb we added
the hmp_handle_error which does exactl that but some hmp handlers
don't use it.

In this patch series, I moved all the block related hmp handlers
into blockdev-hmp-cmds.c, and then made them use this function
to report the errors.

I hope I didn't change too much code, I just felt that if
I touch this code, I can also make it easier to find these
handlers, that were scattered over 3 different files.

Best regards,
	Maxim Levitsky

Maxim Levitsky (9):
  monitor: uninline add_init_drive
  monitor: rename device-hotplug.c to blockdev-hmp-cmds.c
  monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
  monitor: move hmp_drive_mirror and hmp_drive_backup to
    blockdev-hmp-cmds.c
  monitor: move hmp_block_job* to blockdev-hmp-cmd.c
  monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c
  monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c
  monitor: move hmp_info_block* to blockdev-hmp-cmds.c
  monitor/hmp: Prefer to use hmp_handle_error for error reporting in
    block hmp commands

 MAINTAINERS         |   1 +
 Makefile.objs       |   4 +-
 blockdev-hmp-cmds.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
 blockdev.c          |  95 -------
 device-hotplug.c    |  91 ------
 monitor/hmp-cmds.c  | 465 -------------------------------
 6 files changed, 659 insertions(+), 653 deletions(-)
 create mode 100644 blockdev-hmp-cmds.c
 delete mode 100644 device-hotplug.c

-- 
2.17.2

[PATCH 1/9] monitor: uninline add_init_drive

[Maxim Levitsky]

This is only used by hmp_drive_add.
The code is just a bit shorter this way.

No functional changes

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 device-hotplug.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/device-hotplug.c b/device-hotplug.c
index f01d53774b..5ce73f0cff 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -34,42 +34,35 @@
 #include "monitor/monitor.h"
 #include "block/block_int.h"
 
-static DriveInfo *add_init_drive(const char *optstr)
+
+void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    DriveInfo *dinfo;
+    DriveInfo *dinfo = NULL;
     QemuOpts *opts;
     MachineClass *mc;
+    const char *optstr = qdict_get_str(qdict, "opts");
+    bool node = qdict_get_try_bool(qdict, "node", false);
+
+    if (node) {
+        hmp_drive_add_node(mon, optstr);
+        return;
+    }
 
     opts = drive_def(optstr);
     if (!opts)
-        return NULL;
+        return;
 
     mc = MACHINE_GET_CLASS(current_machine);
     dinfo = drive_new(opts, mc->block_default_type, &err);
     if (err) {
         error_report_err(err);
         qemu_opts_del(opts);
-        return NULL;
-    }
-
-    return dinfo;
-}
-
-void hmp_drive_add(Monitor *mon, const QDict *qdict)
-{
-    DriveInfo *dinfo = NULL;
-    const char *opts = qdict_get_str(qdict, "opts");
-    bool node = qdict_get_try_bool(qdict, "node", false);
-
-    if (node) {
-        hmp_drive_add_node(mon, opts);
-        return;
+        goto err;
     }
 
-    dinfo = add_init_drive(opts);
     if (!dinfo) {
-        goto err;
+        return;
     }
 
     switch (dinfo->type) {
-- 
2.17.2

[PATCH 2/9] monitor: rename device-hotplug.c to blockdev-hmp-cmds.c

[Maxim Levitsky]

These days device-hotplug.c only contains the hmp_drive_add
In the next patch, rest of hmp_drive* functions will be moved
there.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 MAINTAINERS                             | 1 +
 Makefile.objs                           | 4 ++--
 device-hotplug.c => blockdev-hmp-cmds.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)
 rename device-hotplug.c => blockdev-hmp-cmds.c (98%)

diff --git a/MAINTAINERS b/MAINTAINERS
index dfb7932608..658c38edf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1855,6 +1855,7 @@ Block QAPI, monitor, command line
 M: Markus Armbruster <armbru@redhat.com>
 S: Supported
 F: blockdev.c
+F: blockdev-hmp-cmds.c
 F: block/qapi.c
 F: qapi/block*.json
 F: qapi/transaction.json
diff --git a/Makefile.objs b/Makefile.objs
index 11ba1a36bd..cb33c1873c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,13 +43,13 @@ io-obj-y = io/
 # single QEMU executable should support all CPUs and machines.
 
 ifeq ($(CONFIG_SOFTMMU),y)
-common-obj-y = blockdev.o blockdev-nbd.o block/
+common-obj-y = blockdev.o blockdev-nbd.o blockdev-hmp-cmds.o block/
 common-obj-y += bootdevice.o iothread.o
 common-obj-y += dump/
 common-obj-y += job-qmp.o
 common-obj-y += monitor/
 common-obj-y += net/
-common-obj-y += qdev-monitor.o device-hotplug.o
+common-obj-y += qdev-monitor.o
 common-obj-$(CONFIG_WIN32) += os-win32.o
 common-obj-$(CONFIG_POSIX) += os-posix.o
 
diff --git a/device-hotplug.c b/blockdev-hmp-cmds.c
similarity index 98%
rename from device-hotplug.c
rename to blockdev-hmp-cmds.c
index 5ce73f0cff..21ff6fa9a9 100644
--- a/device-hotplug.c
+++ b/blockdev-hmp-cmds.c
@@ -1,5 +1,5 @@
 /*
- * QEMU device hotplug helpers
+ * Blockdev HMP commands
  *
  * Copyright (c) 2004 Fabrice Bellard
  *
-- 
2.17.2

[PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c          | 95 --------------------------------------------
 2 files changed, 96 insertions(+), 96 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index 21ff6fa9a9..8884618238 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -33,7 +33,7 @@
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "block/block_int.h"
-
+#include "qapi/qapi-commands-block.h"
 
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
@@ -82,3 +82,98 @@ err:
         blk_unref(blk);
     }
 }
+
+void hmp_drive_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    BlockBackend *blk;
+    BlockDriverState *bs;
+    AioContext *aio_context;
+    Error *local_err = NULL;
+
+    bs = bdrv_find_node(id);
+    if (bs) {
+        qmp_blockdev_del(id, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
+        return;
+    }
+
+    blk = blk_by_name(id);
+    if (!blk) {
+        error_report("Device '%s' not found", id);
+        return;
+    }
+
+    if (!blk_legacy_dinfo(blk)) {
+        error_report("Deleting device added with blockdev-add"
+                     " is not supported");
+        return;
+    }
+
+    aio_context = blk_get_aio_context(blk);
+    aio_context_acquire(aio_context);
+
+    bs = blk_bs(blk);
+    if (bs) {
+        if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
+            error_report_err(local_err);
+            aio_context_release(aio_context);
+            return;
+        }
+
+        blk_remove_bs(blk);
+    }
+
+    /* Make the BlockBackend and the attached BlockDriverState anonymous */
+    monitor_remove_blk(blk);
+
+    /* If this BlockBackend has a device attached to it, its refcount will be
+     * decremented when the device is removed; otherwise we have to do so here.
+     */
+    if (blk_get_attached_dev(blk)) {
+        /* Further I/O must not pause the guest */
+        blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
+                         BLOCKDEV_ON_ERROR_REPORT);
+    } else {
+        blk_unref(blk);
+    }
+
+    aio_context_release(aio_context);
+}
+
+void hmp_commit(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    BlockBackend *blk;
+    int ret;
+
+    if (!strcmp(device, "all")) {
+        ret = blk_commit_all();
+    } else {
+        BlockDriverState *bs;
+        AioContext *aio_context;
+
+        blk = blk_by_name(device);
+        if (!blk) {
+            error_report("Device '%s' not found", device);
+            return;
+        }
+        if (!blk_is_available(blk)) {
+            error_report("Device '%s' has no medium", device);
+            return;
+        }
+
+        bs = blk_bs(blk);
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
+
+        ret = bdrv_commit(bs);
+
+        aio_context_release(aio_context);
+    }
+    if (ret < 0) {
+        error_report("'commit' error for '%s': %s", device, strerror(-ret));
+    }
+}
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..df43e0aaef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1074,41 +1074,6 @@ static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
     return blk;
 }
 
-void hmp_commit(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    BlockBackend *blk;
-    int ret;
-
-    if (!strcmp(device, "all")) {
-        ret = blk_commit_all();
-    } else {
-        BlockDriverState *bs;
-        AioContext *aio_context;
-
-        blk = blk_by_name(device);
-        if (!blk) {
-            error_report("Device '%s' not found", device);
-            return;
-        }
-        if (!blk_is_available(blk)) {
-            error_report("Device '%s' has no medium", device);
-            return;
-        }
-
-        bs = blk_bs(blk);
-        aio_context = bdrv_get_aio_context(bs);
-        aio_context_acquire(aio_context);
-
-        ret = bdrv_commit(bs);
-
-        aio_context_release(aio_context);
-    }
-    if (ret < 0) {
-        error_report("'commit' error for '%s': %s", device, strerror(-ret));
-    }
-}
-
 static void blockdev_do_action(TransactionAction *action, Error **errp)
 {
     TransactionActionList list;
@@ -3101,66 +3066,6 @@ BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
     return ret;
 }
 
-void hmp_drive_del(Monitor *mon, const QDict *qdict)
-{
-    const char *id = qdict_get_str(qdict, "id");
-    BlockBackend *blk;
-    BlockDriverState *bs;
-    AioContext *aio_context;
-    Error *local_err = NULL;
-
-    bs = bdrv_find_node(id);
-    if (bs) {
-        qmp_blockdev_del(id, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-        }
-        return;
-    }
-
-    blk = blk_by_name(id);
-    if (!blk) {
-        error_report("Device '%s' not found", id);
-        return;
-    }
-
-    if (!blk_legacy_dinfo(blk)) {
-        error_report("Deleting device added with blockdev-add"
-                     " is not supported");
-        return;
-    }
-
-    aio_context = blk_get_aio_context(blk);
-    aio_context_acquire(aio_context);
-
-    bs = blk_bs(blk);
-    if (bs) {
-        if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
-            error_report_err(local_err);
-            aio_context_release(aio_context);
-            return;
-        }
-
-        blk_remove_bs(blk);
-    }
-
-    /* Make the BlockBackend and the attached BlockDriverState anonymous */
-    monitor_remove_blk(blk);
-
-    /* If this BlockBackend has a device attached to it, its refcount will be
-     * decremented when the device is removed; otherwise we have to do so here.
-     */
-    if (blk_get_attached_dev(blk)) {
-        /* Further I/O must not pause the guest */
-        blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
-                         BLOCKDEV_ON_ERROR_REPORT);
-    } else {
-        blk_unref(blk);
-    }
-
-    aio_context_release(aio_context);
-}
-
 void qmp_block_resize(bool has_device, const char *device,
                       bool has_node_name, const char *node_name,
                       int64_t size, Error **errp)
-- 
2.17.2

[PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c  | 58 ------------------------------------------
 2 files changed, 61 insertions(+), 58 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index 8884618238..5ae899a324 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -34,6 +34,8 @@
 #include "monitor/monitor.h"
 #include "block/block_int.h"
 #include "qapi/qapi-commands-block.h"
+#include "qapi/qmp/qerror.h"
+#include "monitor/hmp.h"
 
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
@@ -177,3 +179,62 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
         error_report("'commit' error for '%s': %s", device, strerror(-ret));
     }
 }
+
+void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
+{
+    const char *filename = qdict_get_str(qdict, "target");
+    const char *format = qdict_get_try_str(qdict, "format");
+    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+    bool full = qdict_get_try_bool(qdict, "full", false);
+    Error *err = NULL;
+    DriveMirror mirror = {
+        .device = (char *)qdict_get_str(qdict, "device"),
+        .target = (char *)filename,
+        .has_format = !!format,
+        .format = (char *)format,
+        .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
+        .has_mode = true,
+        .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS,
+        .unmap = true,
+    };
+
+    if (!filename) {
+        error_setg(&err, QERR_MISSING_PARAMETER, "target");
+        hmp_handle_error(mon, &err);
+        return;
+    }
+    qmp_drive_mirror(&mirror, &err);
+    hmp_handle_error(mon, &err);
+}
+
+void hmp_drive_backup(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *filename = qdict_get_str(qdict, "target");
+    const char *format = qdict_get_try_str(qdict, "format");
+    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+    bool full = qdict_get_try_bool(qdict, "full", false);
+    bool compress = qdict_get_try_bool(qdict, "compress", false);
+    Error *err = NULL;
+    DriveBackup backup = {
+        .device = (char *)device,
+        .target = (char *)filename,
+        .has_format = !!format,
+        .format = (char *)format,
+        .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
+        .has_mode = true,
+        .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS,
+        .has_compress = !!compress,
+        .compress = compress,
+    };
+
+    if (!filename) {
+        error_setg(&err, QERR_MISSING_PARAMETER, "target");
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    qmp_drive_backup(&backup, &err);
+    hmp_handle_error(mon, &err);
+}
+
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..aa94a15d74 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1338,64 +1338,6 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
-{
-    const char *filename = qdict_get_str(qdict, "target");
-    const char *format = qdict_get_try_str(qdict, "format");
-    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
-    bool full = qdict_get_try_bool(qdict, "full", false);
-    Error *err = NULL;
-    DriveMirror mirror = {
-        .device = (char *)qdict_get_str(qdict, "device"),
-        .target = (char *)filename,
-        .has_format = !!format,
-        .format = (char *)format,
-        .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-        .has_mode = true,
-        .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS,
-        .unmap = true,
-    };
-
-    if (!filename) {
-        error_setg(&err, QERR_MISSING_PARAMETER, "target");
-        hmp_handle_error(mon, &err);
-        return;
-    }
-    qmp_drive_mirror(&mirror, &err);
-    hmp_handle_error(mon, &err);
-}
-
-void hmp_drive_backup(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *filename = qdict_get_str(qdict, "target");
-    const char *format = qdict_get_try_str(qdict, "format");
-    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
-    bool full = qdict_get_try_bool(qdict, "full", false);
-    bool compress = qdict_get_try_bool(qdict, "compress", false);
-    Error *err = NULL;
-    DriveBackup backup = {
-        .device = (char *)device,
-        .target = (char *)filename,
-        .has_format = !!format,
-        .format = (char *)format,
-        .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-        .has_mode = true,
-        .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS,
-        .has_compress = !!compress,
-        .compress = compress,
-    };
-
-    if (!filename) {
-        error_setg(&err, QERR_MISSING_PARAMETER, "target");
-        hmp_handle_error(mon, &err);
-        return;
-    }
-
-    qmp_drive_backup(&backup, &err);
-    hmp_handle_error(mon, &err);
-}
-
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
-- 
2.17.2

[PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c  | 52 ---------------------------------------------
 2 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index 5ae899a324..e333de27b1 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -238,3 +238,55 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+
+void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+    int64_t value = qdict_get_int(qdict, "speed");
+
+    qmp_block_job_set_speed(device, value, &error);
+
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+    bool force = qdict_get_try_bool(qdict, "force", false);
+
+    qmp_block_job_cancel(device, true, force, &error);
+
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+
+    qmp_block_job_pause(device, &error);
+
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+
+    qmp_block_job_resume(device, &error);
+
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+
+    qmp_block_job_complete(device, &error);
+
+    hmp_handle_error(mon, &error);
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index aa94a15d74..326276cced 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1976,58 +1976,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &error);
 }
 
-void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
-{
-    Error *error = NULL;
-    const char *device = qdict_get_str(qdict, "device");
-    int64_t value = qdict_get_int(qdict, "speed");
-
-    qmp_block_job_set_speed(device, value, &error);
-
-    hmp_handle_error(mon, &error);
-}
-
-void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
-{
-    Error *error = NULL;
-    const char *device = qdict_get_str(qdict, "device");
-    bool force = qdict_get_try_bool(qdict, "force", false);
-
-    qmp_block_job_cancel(device, true, force, &error);
-
-    hmp_handle_error(mon, &error);
-}
-
-void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
-{
-    Error *error = NULL;
-    const char *device = qdict_get_str(qdict, "device");
-
-    qmp_block_job_pause(device, &error);
-
-    hmp_handle_error(mon, &error);
-}
-
-void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
-{
-    Error *error = NULL;
-    const char *device = qdict_get_str(qdict, "device");
-
-    qmp_block_job_resume(device, &error);
-
-    hmp_handle_error(mon, &error);
-}
-
-void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
-{
-    Error *error = NULL;
-    const char *device = qdict_get_str(qdict, "device");
-
-    qmp_block_job_complete(device, &error);
-
-    hmp_handle_error(mon, &error);
-}
-
 typedef struct HMPMigrationStatus
 {
     QEMUTimer *timer;
-- 
2.17.2

[PATCH 6/9] monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c  | 46 --------------------------------------------
 2 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index e333de27b1..f3d22c7dd3 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -290,3 +290,50 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
 
     hmp_handle_error(mon, &error);
 }
+
+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");
+    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+    enum NewImageMode mode;
+    Error *err = NULL;
+
+    if (!filename) {
+        /* In the future, if 'snapshot-file' is not specified, the snapshot
+           will be taken internally. Today it's actually required. */
+        error_setg(&err, QERR_MISSING_PARAMETER, "snapshot-file");
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    qmp_blockdev_snapshot_sync(true, device, false, NULL,
+                               filename, false, NULL,
+                               !!format, format,
+                               true, mode, &err);
+    hmp_handle_error(mon, &err);
+}
+
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    Error *err = NULL;
+
+    qmp_blockdev_snapshot_internal_sync(device, name, &err);
+    hmp_handle_error(mon, &err);
+}
+
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    const char *id = qdict_get_try_str(qdict, "id");
+    Error *err = NULL;
+
+    qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+                                               true, name, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 326276cced..2acdcd6e1e 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1338,52 +1338,6 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-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");
-    bool reuse = qdict_get_try_bool(qdict, "reuse", false);
-    enum NewImageMode mode;
-    Error *err = NULL;
-
-    if (!filename) {
-        /* In the future, if 'snapshot-file' is not specified, the snapshot
-           will be taken internally. Today it's actually required. */
-        error_setg(&err, QERR_MISSING_PARAMETER, "snapshot-file");
-        hmp_handle_error(mon, &err);
-        return;
-    }
-
-    mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    qmp_blockdev_snapshot_sync(true, device, false, NULL,
-                               filename, false, NULL,
-                               !!format, format,
-                               true, mode, &err);
-    hmp_handle_error(mon, &err);
-}
-
-void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *name = qdict_get_str(qdict, "name");
-    Error *err = NULL;
-
-    qmp_blockdev_snapshot_internal_sync(device, name, &err);
-    hmp_handle_error(mon, &err);
-}
-
-void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *name = qdict_get_str(qdict, "name");
-    const char *id = qdict_get_try_str(qdict, "id");
-    Error *err = NULL;
-
-    qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
-                                               true, name, &err);
-    hmp_handle_error(mon, &err);
-}
 
 void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
-- 
2.17.2

[PATCH 7/9] monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c  | 64 ---------------------------------------------
 2 files changed, 63 insertions(+), 64 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index f3d22c7dd3..76951352b1 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -337,3 +337,66 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
                                                true, name, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_block_resize(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    int64_t size = qdict_get_int(qdict, "size");
+    Error *err = NULL;
+
+    qmp_block_resize(true, device, false, NULL, size, &err);
+    hmp_handle_error(mon, &err);
+}
+
+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);
+
+    qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
+                     false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
+                     &error);
+
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_passwd(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *password = qdict_get_str(qdict, "password");
+    Error *err = NULL;
+
+    qmp_block_passwd(true, device, false, NULL, password, &err);
+    hmp_handle_error(mon, &err);
+}
+
+void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    char *device = (char *) qdict_get_str(qdict, "device");
+    BlockIOThrottle throttle = {
+        .bps = qdict_get_int(qdict, "bps"),
+        .bps_rd = qdict_get_int(qdict, "bps_rd"),
+        .bps_wr = qdict_get_int(qdict, "bps_wr"),
+        .iops = qdict_get_int(qdict, "iops"),
+        .iops_rd = qdict_get_int(qdict, "iops_rd"),
+        .iops_wr = qdict_get_int(qdict, "iops_wr"),
+    };
+
+    /* qmp_block_set_io_throttle has separate parameters for the
+     * (deprecated) block device name and the qdev ID but the HMP
+     * version has only one, so we must decide which one to pass. */
+    if (blk_by_name(device)) {
+        throttle.has_device = true;
+        throttle.device = device;
+    } else {
+        throttle.has_id = true;
+        throttle.id = device;
+    }
+
+    qmp_block_set_io_throttle(&throttle, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2acdcd6e1e..8be48e0af6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1309,16 +1309,6 @@ void hmp_set_link(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-void hmp_block_passwd(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *password = qdict_get_str(qdict, "password");
-    Error *err = NULL;
-
-    qmp_block_passwd(true, device, false, NULL, password, &err);
-    hmp_handle_error(mon, &err);
-}
-
 void hmp_balloon(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
@@ -1328,17 +1318,6 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-void hmp_block_resize(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    int64_t size = qdict_get_int(qdict, "size");
-    Error *err = NULL;
-
-    qmp_block_resize(true, device, false, NULL, size, &err);
-    hmp_handle_error(mon, &err);
-}
-
-
 void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
     int saved_vm_running  = runstate_is_running();
@@ -1887,49 +1866,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
-{
-    Error *err = NULL;
-    char *device = (char *) qdict_get_str(qdict, "device");
-    BlockIOThrottle throttle = {
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
-
-    /* qmp_block_set_io_throttle has separate parameters for the
-     * (deprecated) block device name and the qdev ID but the HMP
-     * version has only one, so we must decide which one to pass. */
-    if (blk_by_name(device)) {
-        throttle.has_device = true;
-        throttle.device = device;
-    } else {
-        throttle.has_id = true;
-        throttle.id = device;
-    }
-
-    qmp_block_set_io_throttle(&throttle, &err);
-    hmp_handle_error(mon, &err);
-}
-
-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);
-
-    qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
-                     false, NULL, qdict_haskey(qdict, "speed"), speed, true,
-                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
-                     &error);
-
-    hmp_handle_error(mon, &error);
-}
-
 typedef struct HMPMigrationStatus
 {
     QEMUTimer *timer;
-- 
2.17.2

[PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 247 ++++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c  | 245 -------------------------------------------
 2 files changed, 247 insertions(+), 245 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index 76951352b1..c943dccd03 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -33,6 +33,7 @@
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "block/block_int.h"
+#include "block/qapi.h"
 #include "qapi/qapi-commands-block.h"
 #include "qapi/qmp/qerror.h"
 #include "monitor/hmp.h"
@@ -400,3 +401,249 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
     qmp_block_set_io_throttle(&throttle, &err);
     hmp_handle_error(mon, &err);
 }
+
+static void print_block_info(Monitor *mon, BlockInfo *info,
+                             BlockDeviceInfo *inserted, bool verbose)
+{
+    ImageInfo *image_info;
+
+    assert(!info || !info->has_inserted || info->inserted == inserted);
+
+    if (info && *info->device) {
+        monitor_printf(mon, "%s", info->device);
+        if (inserted && inserted->has_node_name) {
+            monitor_printf(mon, " (%s)", inserted->node_name);
+        }
+    } else {
+        assert(info || inserted);
+        monitor_printf(mon, "%s",
+                       inserted && inserted->has_node_name ? inserted->node_name
+                       : info && info->has_qdev ? info->qdev
+                       : "<anonymous>");
+    }
+
+    if (inserted) {
+        monitor_printf(mon, ": %s (%s%s%s)\n",
+                       inserted->file,
+                       inserted->drv,
+                       inserted->ro ? ", read-only" : "",
+                       inserted->encrypted ? ", encrypted" : "");
+    } else {
+        monitor_printf(mon, ": [not inserted]\n");
+    }
+
+    if (info) {
+        if (info->has_qdev) {
+            monitor_printf(mon, "    Attached to:      %s\n", info->qdev);
+        }
+        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
+            monitor_printf(mon, "    I/O status:       %s\n",
+                           BlockDeviceIoStatus_str(info->io_status));
+        }
+
+        if (info->removable) {
+            monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
+                           info->locked ? "" : "not ",
+                           info->tray_open ? "open" : "closed");
+        }
+    }
+
+
+    if (!inserted) {
+        return;
+    }
+
+    monitor_printf(mon, "    Cache mode:       %s%s%s\n",
+                   inserted->cache->writeback ? "writeback" : "writethrough",
+                   inserted->cache->direct ? ", direct" : "",
+                   inserted->cache->no_flush ? ", ignore flushes" : "");
+
+    if (inserted->has_backing_file) {
+        monitor_printf(mon,
+                       "    Backing file:     %s "
+                       "(chain depth: %" PRId64 ")\n",
+                       inserted->backing_file,
+                       inserted->backing_file_depth);
+    }
+
+    if (inserted->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF) {
+        monitor_printf(mon, "    Detect zeroes:    %s\n",
+                BlockdevDetectZeroesOptions_str(inserted->detect_zeroes));
+    }
+
+    if (inserted->bps  || inserted->bps_rd  || inserted->bps_wr  ||
+        inserted->iops || inserted->iops_rd || inserted->iops_wr)
+    {
+        monitor_printf(mon, "    I/O throttling:   bps=%" PRId64
+                        " bps_rd=%" PRId64  " bps_wr=%" PRId64
+                        " bps_max=%" PRId64
+                        " bps_rd_max=%" PRId64
+                        " bps_wr_max=%" PRId64
+                        " iops=%" PRId64 " iops_rd=%" PRId64
+                        " iops_wr=%" PRId64
+                        " iops_max=%" PRId64
+                        " iops_rd_max=%" PRId64
+                        " iops_wr_max=%" PRId64
+                        " iops_size=%" PRId64
+                        " group=%s\n",
+                        inserted->bps,
+                        inserted->bps_rd,
+                        inserted->bps_wr,
+                        inserted->bps_max,
+                        inserted->bps_rd_max,
+                        inserted->bps_wr_max,
+                        inserted->iops,
+                        inserted->iops_rd,
+                        inserted->iops_wr,
+                        inserted->iops_max,
+                        inserted->iops_rd_max,
+                        inserted->iops_wr_max,
+                        inserted->iops_size,
+                        inserted->group);
+    }
+
+    if (verbose) {
+        monitor_printf(mon, "\nImages:\n");
+        image_info = inserted->image;
+        while (1) {
+                bdrv_image_info_dump(image_info);
+            if (image_info->has_backing_image) {
+                image_info = image_info->backing_image;
+            } else {
+                break;
+            }
+        }
+    }
+}
+
+void hmp_info_block(Monitor *mon, const QDict *qdict)
+{
+    BlockInfoList *block_list, *info;
+    BlockDeviceInfoList *blockdev_list, *blockdev;
+    const char *device = qdict_get_try_str(qdict, "device");
+    bool verbose = qdict_get_try_bool(qdict, "verbose", false);
+    bool nodes = qdict_get_try_bool(qdict, "nodes", false);
+    bool printed = false;
+
+    /* Print BlockBackend information */
+    if (!nodes) {
+        block_list = qmp_query_block(NULL);
+    } else {
+        block_list = NULL;
+    }
+
+    for (info = block_list; info; info = info->next) {
+        if (device && strcmp(device, info->value->device)) {
+            continue;
+        }
+
+        if (info != block_list) {
+            monitor_printf(mon, "\n");
+        }
+
+        print_block_info(mon, info->value, info->value->has_inserted
+                                           ? info->value->inserted : NULL,
+                         verbose);
+        printed = true;
+    }
+
+    qapi_free_BlockInfoList(block_list);
+
+    if ((!device && !nodes) || printed) {
+        return;
+    }
+
+    /* Print node information */
+    blockdev_list = qmp_query_named_block_nodes(NULL);
+    for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) {
+        assert(blockdev->value->has_node_name);
+        if (device && strcmp(device, blockdev->value->node_name)) {
+            continue;
+        }
+
+        if (blockdev != blockdev_list) {
+            monitor_printf(mon, "\n");
+        }
+
+        print_block_info(mon, NULL, blockdev->value, verbose);
+    }
+    qapi_free_BlockDeviceInfoList(blockdev_list);
+}
+
+void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
+{
+    BlockStatsList *stats_list, *stats;
+
+    stats_list = qmp_query_blockstats(false, false, NULL);
+
+    for (stats = stats_list; stats; stats = stats->next) {
+        if (!stats->value->has_device) {
+            continue;
+        }
+
+        monitor_printf(mon, "%s:", stats->value->device);
+        monitor_printf(mon, " rd_bytes=%" PRId64
+                       " wr_bytes=%" PRId64
+                       " rd_operations=%" PRId64
+                       " wr_operations=%" PRId64
+                       " flush_operations=%" PRId64
+                       " wr_total_time_ns=%" PRId64
+                       " rd_total_time_ns=%" PRId64
+                       " flush_total_time_ns=%" PRId64
+                       " rd_merged=%" PRId64
+                       " wr_merged=%" PRId64
+                       " idle_time_ns=%" PRId64
+                       "\n",
+                       stats->value->stats->rd_bytes,
+                       stats->value->stats->wr_bytes,
+                       stats->value->stats->rd_operations,
+                       stats->value->stats->wr_operations,
+                       stats->value->stats->flush_operations,
+                       stats->value->stats->wr_total_time_ns,
+                       stats->value->stats->rd_total_time_ns,
+                       stats->value->stats->flush_total_time_ns,
+                       stats->value->stats->rd_merged,
+                       stats->value->stats->wr_merged,
+                       stats->value->stats->idle_time_ns);
+    }
+
+    qapi_free_BlockStatsList(stats_list);
+}
+
+void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
+{
+    BlockJobInfoList *list;
+    Error *err = NULL;
+
+    list = qmp_query_block_jobs(&err);
+    assert(!err);
+
+    if (!list) {
+        monitor_printf(mon, "No active jobs\n");
+        return;
+    }
+
+    while (list) {
+        if (strcmp(list->value->type, "stream") == 0) {
+            monitor_printf(mon, "Streaming device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        } else {
+            monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->type,
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        }
+        list = list->next;
+    }
+
+    qapi_free_BlockJobInfoList(list);
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8be48e0af6..1008902bc3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -468,213 +468,6 @@ void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict)
                    qmp_query_migrate_cache_size(NULL) >> 10);
 }
 
-static void print_block_info(Monitor *mon, BlockInfo *info,
-                             BlockDeviceInfo *inserted, bool verbose)
-{
-    ImageInfo *image_info;
-
-    assert(!info || !info->has_inserted || info->inserted == inserted);
-
-    if (info && *info->device) {
-        monitor_printf(mon, "%s", info->device);
-        if (inserted && inserted->has_node_name) {
-            monitor_printf(mon, " (%s)", inserted->node_name);
-        }
-    } else {
-        assert(info || inserted);
-        monitor_printf(mon, "%s",
-                       inserted && inserted->has_node_name ? inserted->node_name
-                       : info && info->has_qdev ? info->qdev
-                       : "<anonymous>");
-    }
-
-    if (inserted) {
-        monitor_printf(mon, ": %s (%s%s%s)\n",
-                       inserted->file,
-                       inserted->drv,
-                       inserted->ro ? ", read-only" : "",
-                       inserted->encrypted ? ", encrypted" : "");
-    } else {
-        monitor_printf(mon, ": [not inserted]\n");
-    }
-
-    if (info) {
-        if (info->has_qdev) {
-            monitor_printf(mon, "    Attached to:      %s\n", info->qdev);
-        }
-        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
-            monitor_printf(mon, "    I/O status:       %s\n",
-                           BlockDeviceIoStatus_str(info->io_status));
-        }
-
-        if (info->removable) {
-            monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
-                           info->locked ? "" : "not ",
-                           info->tray_open ? "open" : "closed");
-        }
-    }
-
-
-    if (!inserted) {
-        return;
-    }
-
-    monitor_printf(mon, "    Cache mode:       %s%s%s\n",
-                   inserted->cache->writeback ? "writeback" : "writethrough",
-                   inserted->cache->direct ? ", direct" : "",
-                   inserted->cache->no_flush ? ", ignore flushes" : "");
-
-    if (inserted->has_backing_file) {
-        monitor_printf(mon,
-                       "    Backing file:     %s "
-                       "(chain depth: %" PRId64 ")\n",
-                       inserted->backing_file,
-                       inserted->backing_file_depth);
-    }
-
-    if (inserted->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF) {
-        monitor_printf(mon, "    Detect zeroes:    %s\n",
-                BlockdevDetectZeroesOptions_str(inserted->detect_zeroes));
-    }
-
-    if (inserted->bps  || inserted->bps_rd  || inserted->bps_wr  ||
-        inserted->iops || inserted->iops_rd || inserted->iops_wr)
-    {
-        monitor_printf(mon, "    I/O throttling:   bps=%" PRId64
-                        " bps_rd=%" PRId64  " bps_wr=%" PRId64
-                        " bps_max=%" PRId64
-                        " bps_rd_max=%" PRId64
-                        " bps_wr_max=%" PRId64
-                        " iops=%" PRId64 " iops_rd=%" PRId64
-                        " iops_wr=%" PRId64
-                        " iops_max=%" PRId64
-                        " iops_rd_max=%" PRId64
-                        " iops_wr_max=%" PRId64
-                        " iops_size=%" PRId64
-                        " group=%s\n",
-                        inserted->bps,
-                        inserted->bps_rd,
-                        inserted->bps_wr,
-                        inserted->bps_max,
-                        inserted->bps_rd_max,
-                        inserted->bps_wr_max,
-                        inserted->iops,
-                        inserted->iops_rd,
-                        inserted->iops_wr,
-                        inserted->iops_max,
-                        inserted->iops_rd_max,
-                        inserted->iops_wr_max,
-                        inserted->iops_size,
-                        inserted->group);
-    }
-
-    if (verbose) {
-        monitor_printf(mon, "\nImages:\n");
-        image_info = inserted->image;
-        while (1) {
-                bdrv_image_info_dump(image_info);
-            if (image_info->has_backing_image) {
-                image_info = image_info->backing_image;
-            } else {
-                break;
-            }
-        }
-    }
-}
-
-void hmp_info_block(Monitor *mon, const QDict *qdict)
-{
-    BlockInfoList *block_list, *info;
-    BlockDeviceInfoList *blockdev_list, *blockdev;
-    const char *device = qdict_get_try_str(qdict, "device");
-    bool verbose = qdict_get_try_bool(qdict, "verbose", false);
-    bool nodes = qdict_get_try_bool(qdict, "nodes", false);
-    bool printed = false;
-
-    /* Print BlockBackend information */
-    if (!nodes) {
-        block_list = qmp_query_block(NULL);
-    } else {
-        block_list = NULL;
-    }
-
-    for (info = block_list; info; info = info->next) {
-        if (device && strcmp(device, info->value->device)) {
-            continue;
-        }
-
-        if (info != block_list) {
-            monitor_printf(mon, "\n");
-        }
-
-        print_block_info(mon, info->value, info->value->has_inserted
-                                           ? info->value->inserted : NULL,
-                         verbose);
-        printed = true;
-    }
-
-    qapi_free_BlockInfoList(block_list);
-
-    if ((!device && !nodes) || printed) {
-        return;
-    }
-
-    /* Print node information */
-    blockdev_list = qmp_query_named_block_nodes(NULL);
-    for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) {
-        assert(blockdev->value->has_node_name);
-        if (device && strcmp(device, blockdev->value->node_name)) {
-            continue;
-        }
-
-        if (blockdev != blockdev_list) {
-            monitor_printf(mon, "\n");
-        }
-
-        print_block_info(mon, NULL, blockdev->value, verbose);
-    }
-    qapi_free_BlockDeviceInfoList(blockdev_list);
-}
-
-void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
-{
-    BlockStatsList *stats_list, *stats;
-
-    stats_list = qmp_query_blockstats(false, false, NULL);
-
-    for (stats = stats_list; stats; stats = stats->next) {
-        if (!stats->value->has_device) {
-            continue;
-        }
-
-        monitor_printf(mon, "%s:", stats->value->device);
-        monitor_printf(mon, " rd_bytes=%" PRId64
-                       " wr_bytes=%" PRId64
-                       " rd_operations=%" PRId64
-                       " wr_operations=%" PRId64
-                       " flush_operations=%" PRId64
-                       " wr_total_time_ns=%" PRId64
-                       " rd_total_time_ns=%" PRId64
-                       " flush_total_time_ns=%" PRId64
-                       " rd_merged=%" PRId64
-                       " wr_merged=%" PRId64
-                       " idle_time_ns=%" PRId64
-                       "\n",
-                       stats->value->stats->rd_bytes,
-                       stats->value->stats->wr_bytes,
-                       stats->value->stats->rd_operations,
-                       stats->value->stats->wr_operations,
-                       stats->value->stats->flush_operations,
-                       stats->value->stats->wr_total_time_ns,
-                       stats->value->stats->rd_total_time_ns,
-                       stats->value->stats->flush_total_time_ns,
-                       stats->value->stats->rd_merged,
-                       stats->value->stats->wr_merged,
-                       stats->value->stats->idle_time_ns);
-    }
-
-    qapi_free_BlockStatsList(stats_list);
-}
 
 #ifdef CONFIG_VNC
 /* Helper for hmp_info_vnc_clients, _servers */
@@ -1054,44 +847,6 @@ void hmp_info_pci(Monitor *mon, const QDict *qdict)
     qapi_free_PciInfoList(info_list);
 }
 
-void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
-{
-    BlockJobInfoList *list;
-    Error *err = NULL;
-
-    list = qmp_query_block_jobs(&err);
-    assert(!err);
-
-    if (!list) {
-        monitor_printf(mon, "No active jobs\n");
-        return;
-    }
-
-    while (list) {
-        if (strcmp(list->value->type, "stream") == 0) {
-            monitor_printf(mon, "Streaming device %s: Completed %" PRId64
-                           " of %" PRId64 " bytes, speed limit %" PRId64
-                           " bytes/s\n",
-                           list->value->device,
-                           list->value->offset,
-                           list->value->len,
-                           list->value->speed);
-        } else {
-            monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
-                           " of %" PRId64 " bytes, speed limit %" PRId64
-                           " bytes/s\n",
-                           list->value->type,
-                           list->value->device,
-                           list->value->offset,
-                           list->value->len,
-                           list->value->speed);
-        }
-        list = list->next;
-    }
-
-    qapi_free_BlockJobInfoList(list);
-}
-
 void hmp_info_tpm(Monitor *mon, const QDict *qdict)
 {
     TPMInfoList *info_list, *info;
-- 
2.17.2

[PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

This way they all will be prefixed with 'Error:' which some parsers
(e.g libvirt need)


Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 blockdev-hmp-cmds.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
index c943dccd03..197994716f 100644
--- a/blockdev-hmp-cmds.c
+++ b/blockdev-hmp-cmds.c
@@ -59,7 +59,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
     mc = MACHINE_GET_CLASS(current_machine);
     dinfo = drive_new(opts, mc->block_default_type, &err);
     if (err) {
-        error_report_err(err);
         qemu_opts_del(opts);
         goto err;
     }
@@ -73,7 +72,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "OK\n");
         break;
     default:
-        monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
+        error_setg(&err, "Can't hot-add drive to type %d", dinfo->type);
         goto err;
     }
     return;
@@ -84,6 +83,7 @@ err:
         monitor_remove_blk(blk);
         blk_unref(blk);
     }
+    hmp_handle_error(mon, &err);
 }
 
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
@@ -105,14 +105,14 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
     blk = blk_by_name(id);
     if (!blk) {
-        error_report("Device '%s' not found", id);
-        return;
+        error_setg(&local_err, "Device '%s' not found", id);
+        goto err;
     }
 
     if (!blk_legacy_dinfo(blk)) {
-        error_report("Deleting device added with blockdev-add"
-                     " is not supported");
-        return;
+        error_setg(&local_err,
+                   "Deleting device added with blockdev-add is not supported");
+        goto err;
     }
 
     aio_context = blk_get_aio_context(blk);
@@ -121,9 +121,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
     bs = blk_bs(blk);
     if (bs) {
         if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
-            error_report_err(local_err);
             aio_context_release(aio_context);
-            return;
+            goto err;
         }
 
         blk_remove_bs(blk);
@@ -144,12 +143,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
     }
 
     aio_context_release(aio_context);
+err:
+    hmp_handle_error(mon, &local_err);
 }
 
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
     BlockBackend *blk;
+    Error *local_err = NULL;
     int ret;
 
     if (!strcmp(device, "all")) {
@@ -160,12 +162,12 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 
         blk = blk_by_name(device);
         if (!blk) {
-            error_report("Device '%s' not found", device);
-            return;
+            error_setg(&local_err, "Device '%s' not found", device);
+            goto err;
         }
         if (!blk_is_available(blk)) {
-            error_report("Device '%s' has no medium", device);
-            return;
+            error_setg(&local_err, "Device '%s' has no medium", device);
+            goto err;
         }
 
         bs = blk_bs(blk);
@@ -177,8 +179,13 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
         aio_context_release(aio_context);
     }
     if (ret < 0) {
-        error_report("'commit' error for '%s': %s", device, strerror(-ret));
+        error_setg(&local_err,
+                   "'commit' error for '%s': %s", device, strerror(-ret));
+        goto err;
     }
+    return;
+err:
+    hmp_handle_error(mon, &local_err);
 }
 
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
-- 
2.17.2

Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups

[Dr. David Alan Gilbert]

* Maxim Levitsky (mlevitsk@redhat.com) wrote:
> This patch series is bunch of cleanups
> to the hmp monitor code.
> 
> This series only touched blockdev related hmp handlers.

Hi Maxim,
  This looks mostly OK to me from the HMP side - with one change;
can you please move the blockdev-hmp-cmds.c into either monitor/ or
block/; with that


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

we also need a corresponding review by a block person.

Dave


> No functional changes expected other that
> light error message changes by the last patch.
> 
> This was inspired by this bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> 
> Basically some users still parse hmp error messages,
> and they would like to have them prefixed with 'Error:'
> 
> In commit 66363e9a43f649360a3f74d2805c9f864da027eb we added
> the hmp_handle_error which does exactl that but some hmp handlers
> don't use it.
> 
> In this patch series, I moved all the block related hmp handlers
> into blockdev-hmp-cmds.c, and then made them use this function
> to report the errors.
> 
> I hope I didn't change too much code, I just felt that if
> I touch this code, I can also make it easier to find these
> handlers, that were scattered over 3 different files.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (9):
>   monitor: uninline add_init_drive
>   monitor: rename device-hotplug.c to blockdev-hmp-cmds.c
>   monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
>   monitor: move hmp_drive_mirror and hmp_drive_backup to
>     blockdev-hmp-cmds.c
>   monitor: move hmp_block_job* to blockdev-hmp-cmd.c
>   monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c
>   monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c
>   monitor: move hmp_info_block* to blockdev-hmp-cmds.c
>   monitor/hmp: Prefer to use hmp_handle_error for error reporting in
>     block hmp commands
> 
>  MAINTAINERS         |   1 +
>  Makefile.objs       |   4 +-
>  blockdev-hmp-cmds.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c          |  95 -------
>  device-hotplug.c    |  91 ------
>  monitor/hmp-cmds.c  | 465 -------------------------------
>  6 files changed, 659 insertions(+), 653 deletions(-)
>  create mode 100644 blockdev-hmp-cmds.c
>  delete mode 100644 device-hotplug.c
> 
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups

[Kevin Wolf]

Am 22.11.2019 um 11:15 hat Dr. David Alan Gilbert geschrieben:
> * Maxim Levitsky (mlevitsk@redhat.com) wrote:
> > This patch series is bunch of cleanups
> > to the hmp monitor code.
> > 
> > This series only touched blockdev related hmp handlers.
> 
> Hi Maxim,
>   This looks mostly OK to me from the HMP side - with one change;
> can you please move the blockdev-hmp-cmds.c into either monitor/ or
> block/; with that

Let's create a block/monitor/hmp.c then. There is more stuff that could
move into that directory later.

Kevin

Re: [PATCH 1/9] monitor: uninline add_init_drive

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> This is only used by hmp_drive_add.
> The code is just a bit shorter this way.
>
> No functional changes
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  device-hotplug.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/device-hotplug.c b/device-hotplug.c
> index f01d53774b..5ce73f0cff 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -34,42 +34,35 @@
>  #include "monitor/monitor.h"
>  #include "block/block_int.h"
>  
> -static DriveInfo *add_init_drive(const char *optstr)
> +
> +void hmp_drive_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -    DriveInfo *dinfo;
> +    DriveInfo *dinfo = NULL;

Superfluous initializer.

>      QemuOpts *opts;
>      MachineClass *mc;
> +    const char *optstr = qdict_get_str(qdict, "opts");
> +    bool node = qdict_get_try_bool(qdict, "node", false);
> +
> +    if (node) {
> +        hmp_drive_add_node(mon, optstr);
> +        return;
> +    }
>  
>      opts = drive_def(optstr);
>      if (!opts)
> -        return NULL;
> +        return;
>  
>      mc = MACHINE_GET_CLASS(current_machine);
>      dinfo = drive_new(opts, mc->block_default_type, &err);
>      if (err) {
>          error_report_err(err);
>          qemu_opts_del(opts);
> -        return NULL;
> -    }
> -
> -    return dinfo;
> -}
> -
> -void hmp_drive_add(Monitor *mon, const QDict *qdict)
> -{
> -    DriveInfo *dinfo = NULL;
> -    const char *opts = qdict_get_str(qdict, "opts");
> -    bool node = qdict_get_try_bool(qdict, "node", false);
> -
> -    if (node) {
> -        hmp_drive_add_node(mon, opts);
> -        return;
> +        goto err;
>      }
>  
> -    dinfo = add_init_drive(opts);
>      if (!dinfo) {
> -        goto err;
> +        return;
>      }
>  
>      switch (dinfo->type) {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  blockdev-hmp-cmds.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
>  monitor/hmp-cmds.c  | 58 ------------------------------------------
>  2 files changed, 61 insertions(+), 58 deletions(-)

Licensing issue: blockdev-hmp-cmds.c is BSD, the code you moved there is
GPLv2+.  The combined work must be GPLv2+.

Re: [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  blockdev-hmp-cmds.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  monitor/hmp-cmds.c  | 52 ---------------------------------------------
>  2 files changed, 52 insertions(+), 52 deletions(-)
>
> diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> index 5ae899a324..e333de27b1 100644
> --- a/blockdev-hmp-cmds.c
> +++ b/blockdev-hmp-cmds.c
> @@ -238,3 +238,55 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +

Is this extra blank line intentional?

> +void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
> +{
> +    Error *error = NULL;
> +    const char *device = qdict_get_str(qdict, "device");
> +    int64_t value = qdict_get_int(qdict, "speed");
> +
> +    qmp_block_job_set_speed(device, value, &error);
> +
> +    hmp_handle_error(mon, &error);
> +}
[...]

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  blockdev-hmp-cmds.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c          | 95 --------------------------------------------
>  2 files changed, 96 insertions(+), 96 deletions(-)
>
> diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> index 21ff6fa9a9..8884618238 100644
> --- a/blockdev-hmp-cmds.c
> +++ b/blockdev-hmp-cmds.c
> @@ -33,7 +33,7 @@
>  #include "sysemu/sysemu.h"
>  #include "monitor/monitor.h"
>  #include "block/block_int.h"
> -
> +#include "qapi/qapi-commands-block.h"

I prefer keeping qapi/ stuff together.  Please add this right before
#include "qapi/qmp/qdict.h".

[...]

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  blockdev-hmp-cmds.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c          | 95 --------------------------------------------
>  2 files changed, 96 insertions(+), 96 deletions(-)

hmp_drive_add() and hmp_drive_del() are now in the same .c, which feels
right.  Their declarations are still in separate .h.  Suggest to move
hmp_drive_add() from sysemu/sysemu.h to sysemu/blockdev.h.  Or maybe
create a separate .h for the block HMP stuff, just like you created a
separate .c.

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Markus Armbruster]

I think it makes sense to collect *all* block HMP stuff here.

Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...

I guess hmp_change() has to stay there, because it's both block and ui.

Left in blockdev.c: hmp_drive_add_node().

Quick grep for possible files to check:

$ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
MAINTAINERS
blockdev-hmp-cmds.c
blockdev.c
cpus.c
dump/dump.c
hw/display/qxl.c
hw/scsi/vhost-scsi.c
hw/usb/dev-storage.c
include/monitor/monitor.h
migration/migration.c
monitor/hmp-cmds.c
monitor/hmp.c
monitor/misc.c
monitor/qmp-cmds.c
qdev-monitor.c
vl.c

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Markus Armbruster]

Title is too long.  blockdev-hmp-cmds.c will become
block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest

    block/monitor: Prefer to use hmp_handle_error() to report HMP errors

Maxim Levitsky <mlevitsk@redhat.com> writes:

> This way they all will be prefixed with 'Error:' which some parsers
> (e.g libvirt need)

Sadly, "all" is far from true.  Consider

    void hmp_drive_add(Monitor *mon, const QDict *qdict)
    {
        Error *err = NULL;
        DriveInfo *dinfo = NULL;
        QemuOpts *opts;
        MachineClass *mc;
        const char *optstr = qdict_get_str(qdict, "opts");
        bool node = qdict_get_try_bool(qdict, "node", false);

        if (node) {
            hmp_drive_add_node(mon, optstr);
            return;
        }

        opts = drive_def(optstr);
        if (!opts)
            return;


hmp_drive_add_node() uses error_report() and error_report_err().  Easy
enough to fix if you move the function here, as I suggested in my review
of PATCH 8.

drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
error_report_err().  You can't change qemu_opts_parse_noisily() to use
hmp_handle_error().  You'd have to convert drive_def() to Error, which
involves switching it to qemu_opts_parse() + qemu_opts_print_help().

These are just the first two error paths in this file.  There's much
more.  Truly routing all HMP errors through hmp_handle_error() takes a
*massive* Error conversion effort, with a high risk of missing Error
conversions, followed by a never-ending risk of non-Error stuff creeping
in.

There must be an easier way.

Consider vreport():

    switch (type) {
    case REPORT_TYPE_ERROR:
        break;
    case REPORT_TYPE_WARNING:
        error_printf("warning: ");
        break;
    case REPORT_TYPE_INFO:
        error_printf("info: ");
        break;
    }

Adding the prefix here (either unconditionally, or if cur_mon) covers
all HMP errors reported with error_report() & friends in one blow.

That leaves the ones that are still reported with monitor_printf().
Converting those to error_report() looks far more tractable to me.

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  blockdev-hmp-cmds.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> index c943dccd03..197994716f 100644
> --- a/blockdev-hmp-cmds.c
> +++ b/blockdev-hmp-cmds.c
> @@ -59,7 +59,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
>      mc = MACHINE_GET_CLASS(current_machine);
>      dinfo = drive_new(opts, mc->block_default_type, &err);
>      if (err) {
> -        error_report_err(err);
>          qemu_opts_del(opts);
>          goto err;
>      }
> @@ -73,7 +72,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "OK\n");
>          break;
>      default:
> -        monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
> +        error_setg(&err, "Can't hot-add drive to type %d", dinfo->type);
>          goto err;
>      }
>      return;
> @@ -84,6 +83,7 @@ err:
>          monitor_remove_blk(blk);
>          blk_unref(blk);
>      }
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_drive_del(Monitor *mon, const QDict *qdict)
> @@ -105,14 +105,14 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>  
>      blk = blk_by_name(id);
>      if (!blk) {
> -        error_report("Device '%s' not found", id);
> -        return;
> +        error_setg(&local_err, "Device '%s' not found", id);
> +        goto err;

Having to create Error objects just so we can use hmp_handle_error() is
awkward.  Tolerable if using hmp_handle_error() improves matters.  I'm
not sure it does.

>      }
>  
>      if (!blk_legacy_dinfo(blk)) {
> -        error_report("Deleting device added with blockdev-add"
> -                     " is not supported");
> -        return;
> +        error_setg(&local_err,
> +                   "Deleting device added with blockdev-add is not supported");
> +        goto err;
>      }
>  
>      aio_context = blk_get_aio_context(blk);
> @@ -121,9 +121,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>      bs = blk_bs(blk);
>      if (bs) {
>          if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
> -            error_report_err(local_err);
>              aio_context_release(aio_context);
> -            return;
> +            goto err;
>          }
>  
>          blk_remove_bs(blk);
> @@ -144,12 +143,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>      }
>  
>      aio_context_release(aio_context);
> +err:
> +    hmp_handle_error(mon, &local_err);
>  }
>  
>  void hmp_commit(Monitor *mon, const QDict *qdict)
>  {
>      const char *device = qdict_get_str(qdict, "device");
>      BlockBackend *blk;
> +    Error *local_err = NULL;
>      int ret;
>  
>      if (!strcmp(device, "all")) {
> @@ -160,12 +162,12 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
>  
>          blk = blk_by_name(device);
>          if (!blk) {
> -            error_report("Device '%s' not found", device);
> -            return;
> +            error_setg(&local_err, "Device '%s' not found", device);
> +            goto err;
>          }
>          if (!blk_is_available(blk)) {
> -            error_report("Device '%s' has no medium", device);
> -            return;
> +            error_setg(&local_err, "Device '%s' has no medium", device);
> +            goto err;
>          }
>  
>          bs = blk_bs(blk);
> @@ -177,8 +179,13 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
>          aio_context_release(aio_context);
>      }
>      if (ret < 0) {
> -        error_report("'commit' error for '%s': %s", device, strerror(-ret));
> +        error_setg(&local_err,
> +                   "'commit' error for '%s': %s", device, strerror(-ret));
> +        goto err;
>      }
> +    return;
> +err:
> +    hmp_handle_error(mon, &local_err);
>  }
>  
>  void hmp_drive_mirror(Monitor *mon, const QDict *qdict)

Re: [PATCH 1/9] monitor: uninline add_init_drive

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:13 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > This is only used by hmp_drive_add.
> > The code is just a bit shorter this way.
> > 
> > No functional changes
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  device-hotplug.c | 33 +++++++++++++--------------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> > 
> > diff --git a/device-hotplug.c b/device-hotplug.c
> > index f01d53774b..5ce73f0cff 100644
> > --- a/device-hotplug.c
> > +++ b/device-hotplug.c
> > @@ -34,42 +34,35 @@
> >  #include "monitor/monitor.h"
> >  #include "block/block_int.h"
> >  
> > -static DriveInfo *add_init_drive(const char *optstr)
> > +
> > +void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >  {
> >      Error *err = NULL;
> > -    DriveInfo *dinfo;
> > +    DriveInfo *dinfo = NULL;
> 
> Superfluous initializer.
True, fixed now.
> 
> >      QemuOpts *opts;
> >      MachineClass *mc;
> > +    const char *optstr = qdict_get_str(qdict, "opts");
> > +    bool node = qdict_get_try_bool(qdict, "node", false);
> > +
> > +    if (node) {
> > +        hmp_drive_add_node(mon, optstr);
> > +        return;
> > +    }
> >  
> >      opts = drive_def(optstr);
> >      if (!opts)
> > -        return NULL;
> > +        return;
> >  
> >      mc = MACHINE_GET_CLASS(current_machine);
> >      dinfo = drive_new(opts, mc->block_default_type, &err);
> >      if (err) {
> >          error_report_err(err);
> >          qemu_opts_del(opts);
> > -        return NULL;
> > -    }
> > -
> > -    return dinfo;
> > -}
> > -
> > -void hmp_drive_add(Monitor *mon, const QDict *qdict)
> > -{
> > -    DriveInfo *dinfo = NULL;
> > -    const char *opts = qdict_get_str(qdict, "opts");
> > -    bool node = qdict_get_try_bool(qdict, "node", false);
> > -
> > -    if (node) {
> > -        hmp_drive_add_node(mon, opts);
> > -        return;
> > +        goto err;
> >      }
> >  
> > -    dinfo = add_init_drive(opts);
> >      if (!dinfo) {
> > -        goto err;
> > +        return;
> >      }
> >  
> >      switch (dinfo->type) {
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> 

Best regards,
	Maxim Levitsky

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:29 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  blockdev-hmp-cmds.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> >  blockdev.c          | 95 --------------------------------------------
> >  2 files changed, 96 insertions(+), 96 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index 21ff6fa9a9..8884618238 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -33,7 +33,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "monitor/monitor.h"
> >  #include "block/block_int.h"
> > -
> > +#include "qapi/qapi-commands-block.h"
> 
> I prefer keeping qapi/ stuff together.  Please add this right before
> #include "qapi/qmp/qdict.h".

Absolutely no problem!

Best regards,
	Maxim Levitsky

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:50 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  blockdev-hmp-cmds.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> >  blockdev.c          | 95 --------------------------------------------
> >  2 files changed, 96 insertions(+), 96 deletions(-)
> 
> hmp_drive_add() and hmp_drive_del() are now in the same .c, which feels
> right.  Their declarations are still in separate .h.  Suggest to move
> hmp_drive_add() from sysemu/sysemu.h to sysemu/blockdev.h.  Or maybe
> create a separate .h for the block HMP stuff, just like you created a
> separate .c.
> 
> 

Agree, I totally forgot about the headers.
I added include/block/blockdev-hmp-cmds.h for that now.

Best regards,
	Maxim Levitsky

Re: [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:24 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  blockdev-hmp-cmds.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
> >  monitor/hmp-cmds.c  | 52 ---------------------------------------------
> >  2 files changed, 52 insertions(+), 52 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index 5ae899a324..e333de27b1 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -238,3 +238,55 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
> >      hmp_handle_error(mon, &err);
> >  }
> >  
> > +
> 
> Is this extra blank line intentional?

Yes, it gives me 1 black line spacing between all functions.

Best regards,
	Maxim Levitsky

Re: [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:22 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  blockdev-hmp-cmds.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
> >  monitor/hmp-cmds.c  | 58 ------------------------------------------
> >  2 files changed, 61 insertions(+), 58 deletions(-)
> 
> Licensing issue: blockdev-hmp-cmds.c is BSD, the code you moved there is
> GPLv2+.  The combined work must be GPLv2+.
> 
> 

Fixed, thanks!

Best regards,
	Maxim Levitsky

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
> Title is too long.  blockdev-hmp-cmds.c will become
> block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
> 
>     block/monitor: Prefer to use hmp_handle_error() to report HMP errors
> 
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > This way they all will be prefixed with 'Error:' which some parsers
> > (e.g libvirt need)
> 
> Sadly, "all" is far from true.  Consider
> 
>     void hmp_drive_add(Monitor *mon, const QDict *qdict)
>     {
>         Error *err = NULL;
>         DriveInfo *dinfo = NULL;
>         QemuOpts *opts;
>         MachineClass *mc;
>         const char *optstr = qdict_get_str(qdict, "opts");
>         bool node = qdict_get_try_bool(qdict, "node", false);
> 
>         if (node) {
>             hmp_drive_add_node(mon, optstr);
>             return;
>         }
> 
>         opts = drive_def(optstr);
>         if (!opts)
>             return;
> 
> 
> hmp_drive_add_node() uses error_report() and error_report_err().  Easy
> enough to fix if you move the function here, as I suggested in my review
> of PATCH 8.
To be honest that involves exporting the monitor_bdrv_states variable and
bds_tree_init, which were both static before, but I created a patch that does that,
If that is all right, I'll squash it with some of my patches.


> 
> drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
> error_report_err().  You can't change qemu_opts_parse_noisily() to use
> hmp_handle_error().  You'd have to convert drive_def() to Error, which
> involves switching it to qemu_opts_parse() + qemu_opts_print_help().
> 
> These are just the first two error paths in this file.  There's much
> more.  Truly routing all HMP errors through hmp_handle_error() takes a
> *massive* Error conversion effort, with a high risk of missing Error
> conversions, followed by a never-ending risk of non-Error stuff creeping
> in.
Oops. Active can of worms is detected. Take cover!

> 
> There must be an easier way.
> 
> Consider vreport():
> 
>     switch (type) {
>     case REPORT_TYPE_ERROR:
>         break;
>     case REPORT_TYPE_WARNING:
>         error_printf("warning: ");
>         break;
>     case REPORT_TYPE_INFO:
>         error_printf("info: ");
>         break;
>     }
> 
> Adding the prefix here (either unconditionally, or if cur_mon) covers
> all HMP errors reported with error_report() & friends in one blow.

This is a very good idea.
If feels like this should be done unconditionally, although that will
break probably some scripts that depend on exact value of the error message (but to be honest,
scripts shouldn't be doing that in first place).

Doing that with cur_mon (took me some time to figure out what that is) will
limit the damage but its a bit of a hack.


I think that this is a very good change anyway though so if everyone agrees,
I will be more that happy to do this change.
Thoughts?

> 
> That leaves the ones that are still reported with monitor_printf().
> Converting those to error_report() looks far more tractable to me.
Yep, in fact I grepped the tree for monitor_printf and there are not
that much instances of this used for error reporting, so it might
be possible to have 'error' prefix on all monitor errors that way
and not only for the block layer.

> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  blockdev-hmp-cmds.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index c943dccd03..197994716f 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -59,7 +59,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >      mc = MACHINE_GET_CLASS(current_machine);
> >      dinfo = drive_new(opts, mc->block_default_type, &err);
> >      if (err) {
> > -        error_report_err(err);
> >          qemu_opts_del(opts);
> >          goto err;
> >      }
> > @@ -73,7 +72,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >          monitor_printf(mon, "OK\n");
> >          break;
> >      default:
> > -        monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
> > +        error_setg(&err, "Can't hot-add drive to type %d", dinfo->type);
> >          goto err;
> >      }
> >      return;
> > @@ -84,6 +83,7 @@ err:
> >          monitor_remove_blk(blk);
> >          blk_unref(blk);
> >      }
> > +    hmp_handle_error(mon, &err);
> >  }
> >  
> >  void hmp_drive_del(Monitor *mon, const QDict *qdict)
> > @@ -105,14 +105,14 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >  
> >      blk = blk_by_name(id);
> >      if (!blk) {
> > -        error_report("Device '%s' not found", id);
> > -        return;
> > +        error_setg(&local_err, "Device '%s' not found", id);
> > +        goto err;
> 
> Having to create Error objects just so we can use hmp_handle_error() is
> awkward.  Tolerable if using hmp_handle_error() improves matters.  I'm
> not sure it does.

Well, if we go with your suggestion of adding the 'error' prefix to vreport
that will not be needed. In fact then the only use of hmp_handle_error
would be to skip reporting if err is NULL, thus this function can be either
dropped or moved to common monitor code.

> 
> >      }
> >  
> >      if (!blk_legacy_dinfo(blk)) {
> > -        error_report("Deleting device added with blockdev-add"
> > -                     " is not supported");
> > -        return;
> > +        error_setg(&local_err,
> > +                   "Deleting device added with blockdev-add is not supported");
> > +        goto err;
> >      }
> >  
> >      aio_context = blk_get_aio_context(blk);
> > @@ -121,9 +121,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >      bs = blk_bs(blk);
> >      if (bs) {
> >          if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
> > -            error_report_err(local_err);
> >              aio_context_release(aio_context);
> > -            return;
> > +            goto err;
> >          }
> >  
> >          blk_remove_bs(blk);
> > @@ -144,12 +143,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      aio_context_release(aio_context);
> > +err:
> > +    hmp_handle_error(mon, &local_err);
> >  }
> >  
> >  void hmp_commit(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *device = qdict_get_str(qdict, "device");
> >      BlockBackend *blk;
> > +    Error *local_err = NULL;
> >      int ret;
> >  
> >      if (!strcmp(device, "all")) {
> > @@ -160,12 +162,12 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
> >  
> >          blk = blk_by_name(device);
> >          if (!blk) {
> > -            error_report("Device '%s' not found", device);
> > -            return;
> > +            error_setg(&local_err, "Device '%s' not found", device);
> > +            goto err;
> >          }
> >          if (!blk_is_available(blk)) {
> > -            error_report("Device '%s' has no medium", device);
> > -            return;
> > +            error_setg(&local_err, "Device '%s' has no medium", device);
> > +            goto err;
> >          }
> >  
> >          bs = blk_bs(blk);
> > @@ -177,8 +179,13 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
> >          aio_context_release(aio_context);
> >      }
> >      if (ret < 0) {
> > -        error_report("'commit' error for '%s': %s", device, strerror(-ret));
> > +        error_setg(&local_err,
> > +                   "'commit' error for '%s': %s", device, strerror(-ret));
> > +        goto err;
> >      }
> > +    return;
> > +err:
> > +    hmp_handle_error(mon, &local_err);
> >  }
> >  
> >  void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
> 
> 

Best regards,
	Maxim Levitsky

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> I think it makes sense to collect *all* block HMP stuff here.
> 
> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> 
> I guess hmp_change() has to stay there, because it's both block and ui.
> 
> Left in blockdev.c: hmp_drive_add_node().

Thank you very much. I added these and bunch more to my patchset.

> 
> Quick grep for possible files to check:
> 
> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> MAINTAINERS
> blockdev-hmp-cmds.c
> 

> blockdev.c
hmp_drive_add_node is there and I moved it too.


> cpus.c
Nothing suspicious

> dump/dump.c
qmp_dump_guest_memory is only monitor reference there I think

> hw/display/qxl.c
No way that is related to the block layer

> hw/scsi/vhost-scsi.c
All right, the monitor_fd_param is an interesting thing.
Not related to block though.

> hw/usb/dev-storage.c
All right, this for no reason includes monitor/monitor.h,
added patch to remove this because why not.

> include/monitor/monitor.h
Nothing suspicious

> migration/migration.c
Nothing suspicious

> monitor/hmp-cmds.c
Added hmp_qemu_io

Maybe I need to add hmp_delvm too?
savevm/delvm do old style snapshots
which are stored to the first block device


> monitor/hmp.c
There are some block references in monitor_find_completion,
but I guess it is not worth it to move that

> monitor/misc.c
vm_completion for delvm/loadvm.

> monitor/qmp-cmds.c
Nothing hmp related at first glance.

> qdev-monitor.c
blk_by_qdev_id - used by both hmp and qmp code

> vl.c
Hopefully nothing hmp+block related, I searched the file for
few things but I can't be fully sure.
Out of the curiosity do you know why this file is called like that,
since it hosts qemu main(), shouldn't it be called main.c ?


Best regards and thanks for the detailed review!
	Maxim Levitsky

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
>> I think it makes sense to collect *all* block HMP stuff here.
>> 
>> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
>> 
>> I guess hmp_change() has to stay there, because it's both block and ui.
>> 
>> Left in blockdev.c: hmp_drive_add_node().
>
> Thank you very much. I added these and bunch more to my patchset.
>
>> 
>> Quick grep for possible files to check:
>> 
>> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
>> MAINTAINERS
>> blockdev-hmp-cmds.c
>> 
>
>> blockdev.c
> hmp_drive_add_node is there and I moved it too.
>
>
>> cpus.c
> Nothing suspicious
>
>> dump/dump.c
> qmp_dump_guest_memory is only monitor reference there I think
>
>> hw/display/qxl.c
> No way that is related to the block layer
>
>> hw/scsi/vhost-scsi.c
> All right, the monitor_fd_param is an interesting thing.
> Not related to block though.
>
>> hw/usb/dev-storage.c
> All right, this for no reason includes monitor/monitor.h,
> added patch to remove this because why not.
>
>> include/monitor/monitor.h
> Nothing suspicious
>
>> migration/migration.c
> Nothing suspicious
>
>> monitor/hmp-cmds.c
> Added hmp_qemu_io
>
> Maybe I need to add hmp_delvm too?
> savevm/delvm do old style snapshots
> which are stored to the first block device

One foot in the block subsystem, the other foot in the migration
subsystem.  I'm not sure where this should go.  Kevin?

>> monitor/hmp.c
> There are some block references in monitor_find_completion,
> but I guess it is not worth it to move that
>
>> monitor/misc.c
> vm_completion for delvm/loadvm.

Having completion close to whatever it completes would be nice, I guess.

When in doubt, leave the savevm / delvm stuff alone.

>> monitor/qmp-cmds.c
> Nothing hmp related at first glance.
>
>> qdev-monitor.c
> blk_by_qdev_id - used by both hmp and qmp code
>
>> vl.c
> Hopefully nothing hmp+block related, I searched the file for
> few things but I can't be fully sure.
> Out of the curiosity do you know why this file is called like that,
> since it hosts qemu main(), shouldn't it be called main.c ?

Its first commit 0824d6fc67 "for hard core developpers only: a new user
mode linux project :-)" calls the executable "vl", and has

    void help(void)
    {
        printf("Virtual Linux version " QEMU_VERSION ", Copyright (c) 2003 Fabrice Bellard\n"
               "usage: vl [-h] bzImage initrd [kernel parameters...]\n"
               "\n"
    [...]
        exit(1);
    }

The executable was renamed soon after.  I guess the source file name has
made people wonder ever since.

>
> Best regards and thanks for the detailed review!
> 	Maxim Levitsky

You're welcome!

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
>> Title is too long.  blockdev-hmp-cmds.c will become
>> block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
>> 
>>     block/monitor: Prefer to use hmp_handle_error() to report HMP errors
>> 
>> Maxim Levitsky <mlevitsk@redhat.com> writes:
>> 
>> > This way they all will be prefixed with 'Error:' which some parsers
>> > (e.g libvirt need)
>> 
>> Sadly, "all" is far from true.  Consider
>> 
>>     void hmp_drive_add(Monitor *mon, const QDict *qdict)
>>     {
>>         Error *err = NULL;
>>         DriveInfo *dinfo = NULL;
>>         QemuOpts *opts;
>>         MachineClass *mc;
>>         const char *optstr = qdict_get_str(qdict, "opts");
>>         bool node = qdict_get_try_bool(qdict, "node", false);
>> 
>>         if (node) {
>>             hmp_drive_add_node(mon, optstr);
>>             return;
>>         }
>> 
>>         opts = drive_def(optstr);
>>         if (!opts)
>>             return;
>> 
>> 
>> hmp_drive_add_node() uses error_report() and error_report_err().  Easy
>> enough to fix if you move the function here, as I suggested in my review
>> of PATCH 8.
> To be honest that involves exporting the monitor_bdrv_states variable and
> bds_tree_init, which were both static before, but I created a patch that does that,
> If that is all right, I'll squash it with some of my patches.
>
>
>> 
>> drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
>> error_report_err().  You can't change qemu_opts_parse_noisily() to use
>> hmp_handle_error().  You'd have to convert drive_def() to Error, which
>> involves switching it to qemu_opts_parse() + qemu_opts_print_help().
>> 
>> These are just the first two error paths in this file.  There's much
>> more.  Truly routing all HMP errors through hmp_handle_error() takes a
>> *massive* Error conversion effort, with a high risk of missing Error
>> conversions, followed by a never-ending risk of non-Error stuff creeping
>> in.
> Oops. Active can of worms is detected. Take cover!

:)

>> There must be an easier way.
>> 
>> Consider vreport():
>> 
>>     switch (type) {
>>     case REPORT_TYPE_ERROR:
>>         break;
>>     case REPORT_TYPE_WARNING:
>>         error_printf("warning: ");
>>         break;
>>     case REPORT_TYPE_INFO:
>>         error_printf("info: ");
>>         break;
>>     }
>> 
>> Adding the prefix here (either unconditionally, or if cur_mon) covers
>> all HMP errors reported with error_report() & friends in one blow.
>
> This is a very good idea.
> If feels like this should be done unconditionally, although that will
> break probably some scripts that depend on exact value of the error message (but to be honest,
> scripts shouldn't be doing that in first place).
>
> Doing that with cur_mon (took me some time to figure out what that is) will
> limit the damage but its a bit of a hack.
>
>
> I think that this is a very good change anyway though so if everyone agrees,
> I will be more that happy to do this change.
> Thoughts?

I think adding an "error: " tag has been proposed before.

I dislike overly decorated error messages, because decoration tends to
obscure information.

However, when there's significant non-error output, or even uncertainty
of what's an error and what's something else, decoration can help.

Perhaps you can give some examples where the proposed decoration helps.

>> That leaves the ones that are still reported with monitor_printf().
>> Converting those to error_report() looks far more tractable to me.
> Yep, in fact I grepped the tree for monitor_printf and there are not
> that much instances of this used for error reporting, so it might
> be possible to have 'error' prefix on all monitor errors that way
> and not only for the block layer.

I figure "all" would be more useful than "just for the block layer".

[...]

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
> > > Title is too long.  blockdev-hmp-cmds.c will become
> > > block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
> > > 
> > >     block/monitor: Prefer to use hmp_handle_error() to report HMP errors
> > > 
> > > Maxim Levitsky <mlevitsk@redhat.com> writes:
> > > 
> > > > This way they all will be prefixed with 'Error:' which some parsers
> > > > (e.g libvirt need)
> > > 
> > > Sadly, "all" is far from true.  Consider
> > > 
> > >     void hmp_drive_add(Monitor *mon, const QDict *qdict)
> > >     {
> > >         Error *err = NULL;
> > >         DriveInfo *dinfo = NULL;
> > >         QemuOpts *opts;
> > >         MachineClass *mc;
> > >         const char *optstr = qdict_get_str(qdict, "opts");
> > >         bool node = qdict_get_try_bool(qdict, "node", false);
> > > 
> > >         if (node) {
> > >             hmp_drive_add_node(mon, optstr);
> > >             return;
> > >         }
> > > 
> > >         opts = drive_def(optstr);
> > >         if (!opts)
> > >             return;
> > > 
> > > 
> > > hmp_drive_add_node() uses error_report() and error_report_err().  Easy
> > > enough to fix if you move the function here, as I suggested in my review
> > > of PATCH 8.
> > 
> > To be honest that involves exporting the monitor_bdrv_states variable and
> > bds_tree_init, which were both static before, but I created a patch that does that,
> > If that is all right, I'll squash it with some of my patches.
> > 
> > 
> > > 
> > > drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
> > > error_report_err().  You can't change qemu_opts_parse_noisily() to use
> > > hmp_handle_error().  You'd have to convert drive_def() to Error, which
> > > involves switching it to qemu_opts_parse() + qemu_opts_print_help().
> > > 
> > > These are just the first two error paths in this file.  There's much
> > > more.  Truly routing all HMP errors through hmp_handle_error() takes a
> > > *massive* Error conversion effort, with a high risk of missing Error
> > > conversions, followed by a never-ending risk of non-Error stuff creeping
> > > in.
> > 
> > Oops. Active can of worms is detected. Take cover!
> 
> :)
> 
> > > There must be an easier way.
> > > 
> > > Consider vreport():
> > > 
> > >     switch (type) {
> > >     case REPORT_TYPE_ERROR:
> > >         break;
> > >     case REPORT_TYPE_WARNING:
> > >         error_printf("warning: ");
> > >         break;
> > >     case REPORT_TYPE_INFO:
> > >         error_printf("info: ");
> > >         break;
> > >     }
> > > 
> > > Adding the prefix here (either unconditionally, or if cur_mon) covers
> > > all HMP errors reported with error_report() & friends in one blow.
> > 
> > This is a very good idea.
> > If feels like this should be done unconditionally, although that will
> > break probably some scripts that depend on exact value of the error message (but to be honest,
> > scripts shouldn't be doing that in first place).
> > 
> > Doing that with cur_mon (took me some time to figure out what that is) will
> > limit the damage but its a bit of a hack.
> > 
> > 
> > I think that this is a very good change anyway though so if everyone agrees,
> > I will be more that happy to do this change.
> > Thoughts?
> 
> I think adding an "error: " tag has been proposed before.
> 
> I dislike overly decorated error messages, because decoration tends to
> obscure information.
> 
> However, when there's significant non-error output, or even uncertainty
> of what's an error and what's something else, decoration can help.
Yes, also this way it is consistent

> 
> Perhaps you can give some examples where the proposed decoration helps.
It helps to tag most monitor messages with error prefix which was the root cause of
me starting to work on this refactoring.
You suggested this, and I kind of like that idea.

> 
> > > That leaves the ones that are still reported with monitor_printf().
> > > Converting those to error_report() looks far more tractable to me.
> > 
> > Yep, in fact I grepped the tree for monitor_printf and there are not
> > that much instances of this used for error reporting, so it might
> > be possible to have 'error' prefix on all monitor errors that way
> > and not only for the block layer.
> 
> I figure "all" would be more useful than "just for the block layer".
Yep, the cover letter is outdated, now this patch series touch way
more that the block layer.

Best regards,
	Maxim Levitsky

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Mon, 2020-01-27 at 14:33 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> > > I think it makes sense to collect *all* block HMP stuff here.
> > > 
> > > Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> > > 
> > > I guess hmp_change() has to stay there, because it's both block and ui.
> > > 
> > > Left in blockdev.c: hmp_drive_add_node().
> > 
> > Thank you very much. I added these and bunch more to my patchset.
> > 
> > > 
> > > Quick grep for possible files to check:
> > > 
> > > $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> > > MAINTAINERS
> > > blockdev-hmp-cmds.c
> > > 
> > > blockdev.c
> > 
> > hmp_drive_add_node is there and I moved it too.
> > 
> > 
> > > cpus.c
> > 
> > Nothing suspicious
> > 
> > > dump/dump.c
> > 
> > qmp_dump_guest_memory is only monitor reference there I think
> > 
> > > hw/display/qxl.c
> > 
> > No way that is related to the block layer
> > 
> > > hw/scsi/vhost-scsi.c
> > 
> > All right, the monitor_fd_param is an interesting thing.
> > Not related to block though.
> > 
> > > hw/usb/dev-storage.c
> > 
> > All right, this for no reason includes monitor/monitor.h,
> > added patch to remove this because why not.
> > 
> > > include/monitor/monitor.h
> > 
> > Nothing suspicious
> > 
> > > migration/migration.c
> > 
> > Nothing suspicious
> > 
> > > monitor/hmp-cmds.c
> > 
> > Added hmp_qemu_io
> > 
> > Maybe I need to add hmp_delvm too?
> > savevm/delvm do old style snapshots
> > which are stored to the first block device
> 
> One foot in the block subsystem, the other foot in the migration
> subsystem.  I'm not sure where this should go.  Kevin?
> 
> > > monitor/hmp.c
> > 
> > There are some block references in monitor_find_completion,
> > but I guess it is not worth it to move that
> > 
> > > monitor/misc.c
> > 
> > vm_completion for delvm/loadvm.
> 
> Having completion close to whatever it completes would be nice, I guess.
> 
> When in doubt, leave the savevm / delvm stuff alone.
Yep.

> 
> > > monitor/qmp-cmds.c
> > 
> > Nothing hmp related at first glance.
> > 
> > > qdev-monitor.c
> > 
> > blk_by_qdev_id - used by both hmp and qmp code
> > 
> > > vl.c
> > 
> > Hopefully nothing hmp+block related, I searched the file for
> > few things but I can't be fully sure.
> > Out of the curiosity do you know why this file is called like that,
> > since it hosts qemu main(), shouldn't it be called main.c ?
> 
> Its first commit 0824d6fc67 "for hard core developpers only: a new user
> mode linux project :-)" calls the executable "vl", and has
> 
>     void help(void)
>     {
>         printf("Virtual Linux version " QEMU_VERSION ", Copyright (c) 2003 Fabrice Bellard\n"
>                "usage: vl [-h] bzImage initrd [kernel parameters...]\n"
>                "\n"
>     [...]
>         exit(1);
>     }
> 
> The executable was renamed soon after.  I guess the source file name has
> made people wonder ever since.
Nice :-)

> 
> > 
> > Best regards and thanks for the detailed review!
> > 	Maxim Levitsky
> 
> You're welcome!

I hope we can move forward with this patch series as well.

Best regards,
	Maxim Levitsky

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Kevin Wolf]

Am 27.01.2020 um 14:33 hat Markus Armbruster geschrieben:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> >> I think it makes sense to collect *all* block HMP stuff here.
> >> 
> >> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> >> 
> >> I guess hmp_change() has to stay there, because it's both block and ui.
> >> 
> >> Left in blockdev.c: hmp_drive_add_node().
> >
> > Thank you very much. I added these and bunch more to my patchset.
> >
> >> 
> >> Quick grep for possible files to check:
> >> 
> >> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> >> MAINTAINERS
> >> blockdev-hmp-cmds.c
> >> 
> >
> >> blockdev.c
> > hmp_drive_add_node is there and I moved it too.
> >
> >
> >> cpus.c
> > Nothing suspicious
> >
> >> dump/dump.c
> > qmp_dump_guest_memory is only monitor reference there I think
> >
> >> hw/display/qxl.c
> > No way that is related to the block layer
> >
> >> hw/scsi/vhost-scsi.c
> > All right, the monitor_fd_param is an interesting thing.
> > Not related to block though.
> >
> >> hw/usb/dev-storage.c
> > All right, this for no reason includes monitor/monitor.h,
> > added patch to remove this because why not.
> >
> >> include/monitor/monitor.h
> > Nothing suspicious
> >
> >> migration/migration.c
> > Nothing suspicious
> >
> >> monitor/hmp-cmds.c
> > Added hmp_qemu_io
> >
> > Maybe I need to add hmp_delvm too?
> > savevm/delvm do old style snapshots
> > which are stored to the first block device
> 
> One foot in the block subsystem, the other foot in the migration
> subsystem.  I'm not sure where this should go.  Kevin?

Moving it to block is not an obvious improvement, so I think I'd leave
it alone.

Kevin

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Dr. David Alan Gilbert]

* Maxim Levitsky (mlevitsk@redhat.com) wrote:
> On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote:
> > Maxim Levitsky <mlevitsk@redhat.com> writes:
> > 
> > > On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
> > > > Title is too long.  blockdev-hmp-cmds.c will become
> > > > block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
> > > > 
> > > >     block/monitor: Prefer to use hmp_handle_error() to report HMP errors
> > > > 
> > > > Maxim Levitsky <mlevitsk@redhat.com> writes:
> > > > 
> > > > > This way they all will be prefixed with 'Error:' which some parsers
> > > > > (e.g libvirt need)
> > > > 
> > > > Sadly, "all" is far from true.  Consider
> > > > 
> > > >     void hmp_drive_add(Monitor *mon, const QDict *qdict)
> > > >     {
> > > >         Error *err = NULL;
> > > >         DriveInfo *dinfo = NULL;
> > > >         QemuOpts *opts;
> > > >         MachineClass *mc;
> > > >         const char *optstr = qdict_get_str(qdict, "opts");
> > > >         bool node = qdict_get_try_bool(qdict, "node", false);
> > > > 
> > > >         if (node) {
> > > >             hmp_drive_add_node(mon, optstr);
> > > >             return;
> > > >         }
> > > > 
> > > >         opts = drive_def(optstr);
> > > >         if (!opts)
> > > >             return;
> > > > 
> > > > 
> > > > hmp_drive_add_node() uses error_report() and error_report_err().  Easy
> > > > enough to fix if you move the function here, as I suggested in my review
> > > > of PATCH 8.
> > > 
> > > To be honest that involves exporting the monitor_bdrv_states variable and
> > > bds_tree_init, which were both static before, but I created a patch that does that,
> > > If that is all right, I'll squash it with some of my patches.
> > > 
> > > 
> > > > 
> > > > drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
> > > > error_report_err().  You can't change qemu_opts_parse_noisily() to use
> > > > hmp_handle_error().  You'd have to convert drive_def() to Error, which
> > > > involves switching it to qemu_opts_parse() + qemu_opts_print_help().
> > > > 
> > > > These are just the first two error paths in this file.  There's much
> > > > more.  Truly routing all HMP errors through hmp_handle_error() takes a
> > > > *massive* Error conversion effort, with a high risk of missing Error
> > > > conversions, followed by a never-ending risk of non-Error stuff creeping
> > > > in.
> > > 
> > > Oops. Active can of worms is detected. Take cover!
> > 
> > :)
> > 
> > > > There must be an easier way.
> > > > 
> > > > Consider vreport():
> > > > 
> > > >     switch (type) {
> > > >     case REPORT_TYPE_ERROR:
> > > >         break;
> > > >     case REPORT_TYPE_WARNING:
> > > >         error_printf("warning: ");
> > > >         break;
> > > >     case REPORT_TYPE_INFO:
> > > >         error_printf("info: ");
> > > >         break;
> > > >     }
> > > > 
> > > > Adding the prefix here (either unconditionally, or if cur_mon) covers
> > > > all HMP errors reported with error_report() & friends in one blow.
> > > 
> > > This is a very good idea.
> > > If feels like this should be done unconditionally, although that will
> > > break probably some scripts that depend on exact value of the error message (but to be honest,
> > > scripts shouldn't be doing that in first place).
> > > 
> > > Doing that with cur_mon (took me some time to figure out what that is) will
> > > limit the damage but its a bit of a hack.
> > > 
> > > 
> > > I think that this is a very good change anyway though so if everyone agrees,
> > > I will be more that happy to do this change.
> > > Thoughts?
> > 
> > I think adding an "error: " tag has been proposed before.
> > 
> > I dislike overly decorated error messages, because decoration tends to
> > obscure information.
> > 
> > However, when there's significant non-error output, or even uncertainty
> > of what's an error and what's something else, decoration can help.
> Yes, also this way it is consistent

Yes I also like it; I wouldn't worry too much about things parsing error
messages for the exact error message; if anything is doing that then the
corresponding case needs to have big red flags around it.

Dave

> > 
> > Perhaps you can give some examples where the proposed decoration helps.
> It helps to tag most monitor messages with error prefix which was the root cause of
> me starting to work on this refactoring.
> You suggested this, and I kind of like that idea.
> 
> > 
> > > > That leaves the ones that are still reported with monitor_printf().
> > > > Converting those to error_report() looks far more tractable to me.
> > > 
> > > Yep, in fact I grepped the tree for monitor_printf and there are not
> > > that much instances of this used for error reporting, so it might
> > > be possible to have 'error' prefix on all monitor errors that way
> > > and not only for the block layer.
> > 
> > I figure "all" would be more useful than "just for the block layer".
> Yep, the cover letter is outdated, now this patch series touch way
> more that the block layer.
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK