* [PATCH 1/9] monitor: uninline add_init_drive
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 7:13 ` Markus Armbruster
2019-11-20 18:58 ` [PATCH 2/9] monitor: rename device-hotplug.c to blockdev-hmp-cmds.c Maxim Levitsky
` (8 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 1/9] monitor: uninline add_init_drive
2019-11-20 18:58 ` [PATCH 1/9] monitor: uninline add_init_drive Maxim Levitsky
@ 2019-11-27 7:13 ` Markus Armbruster
2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 7:13 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 1/9] monitor: uninline add_init_drive
2019-11-27 7:13 ` Markus Armbruster
@ 2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/9] monitor: rename device-hotplug.c to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
2019-11-20 18:58 ` [PATCH 1/9] monitor: uninline add_init_drive Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-20 18:58 ` [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit " Maxim Levitsky
` (7 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
2019-11-20 18:58 ` [PATCH 1/9] monitor: uninline add_init_drive Maxim Levitsky
2019-11-20 18:58 ` [PATCH 2/9] monitor: rename device-hotplug.c to blockdev-hmp-cmds.c Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 7:29 ` Markus Armbruster
2019-11-27 7:50 ` Markus Armbruster
2019-11-20 18:58 ` [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup " Maxim Levitsky
` (6 subsequent siblings)
9 siblings, 2 replies; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
2019-11-20 18:58 ` [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit " Maxim Levitsky
@ 2019-11-27 7:29 ` Markus Armbruster
2020-01-27 11:03 ` Maxim Levitsky
2019-11-27 7:50 ` Markus Armbruster
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 7:29 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Dr. David Alan Gilbert, Max Reitz
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".
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
2019-11-27 7:29 ` Markus Armbruster
@ 2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
2019-11-20 18:58 ` [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit " Maxim Levitsky
2019-11-27 7:29 ` Markus Armbruster
@ 2019-11-27 7:50 ` Markus Armbruster
2020-01-27 11:03 ` Maxim Levitsky
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 7:50 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
2019-11-27 7:50 ` Markus Armbruster
@ 2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (2 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit " Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 7:22 ` Markus Armbruster
2019-11-20 18:58 ` [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c Maxim Levitsky
` (5 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c
2019-11-20 18:58 ` [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup " Maxim Levitsky
@ 2019-11-27 7:22 ` Markus Armbruster
2020-01-27 11:04 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 7:22 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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+.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c
2019-11-27 7:22 ` Markus Armbruster
@ 2020-01-27 11:04 ` Maxim Levitsky
0 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (3 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup " Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 7:24 ` Markus Armbruster
2019-11-20 18:58 ` [PATCH 6/9] monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c Maxim Levitsky
` (4 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c
2019-11-20 18:58 ` [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c Maxim Levitsky
@ 2019-11-27 7:24 ` Markus Armbruster
2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 7:24 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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);
> +}
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c
2019-11-27 7:24 ` Markus Armbruster
@ 2020-01-27 11:03 ` Maxim Levitsky
0 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 6/9] monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (4 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-20 18:58 ` [PATCH 7/9] monitor: move remaining hmp_block* functions " Maxim Levitsky
` (3 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 7/9] monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (5 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 6/9] monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-20 18:58 ` [PATCH 8/9] monitor: move hmp_info_block* " Maxim Levitsky
` (2 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (6 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 7/9] monitor: move remaining hmp_block* functions " Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 8:08 ` Markus Armbruster
2019-11-20 18:58 ` [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands Maxim Levitsky
2019-11-22 10:15 ` [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Dr. David Alan Gilbert
9 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2019-11-20 18:58 ` [PATCH 8/9] monitor: move hmp_info_block* " Maxim Levitsky
@ 2019-11-27 8:08 ` Markus Armbruster
2020-01-27 11:05 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 8:08 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2019-11-27 8:08 ` Markus Armbruster
@ 2020-01-27 11:05 ` Maxim Levitsky
2020-01-27 13:33 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2020-01-27 11:05 ` Maxim Levitsky
@ 2020-01-27 13:33 ` Markus Armbruster
2020-01-27 13:54 ` Maxim Levitsky
2020-01-27 14:07 ` Kevin Wolf
0 siblings, 2 replies; 32+ messages in thread
From: Markus Armbruster @ 2020-01-27 13:33 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, Dr. David Alan Gilbert, qemu-block,
qemu-devel
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!
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2020-01-27 13:33 ` Markus Armbruster
@ 2020-01-27 13:54 ` Maxim Levitsky
2020-01-27 14:07 ` Kevin Wolf
1 sibling, 0 replies; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 13:54 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Max Reitz, Dr. David Alan Gilbert, qemu-block,
qemu-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
2020-01-27 13:33 ` Markus Armbruster
2020-01-27 13:54 ` Maxim Levitsky
@ 2020-01-27 14:07 ` Kevin Wolf
1 sibling, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-01-27 14:07 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Max Reitz, Dr. David Alan Gilbert, qemu-block,
Maxim Levitsky
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (7 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 8/9] monitor: move hmp_info_block* " Maxim Levitsky
@ 2019-11-20 18:58 ` Maxim Levitsky
2019-11-27 8:38 ` Markus Armbruster
2019-11-22 10:15 ` [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Dr. David Alan Gilbert
9 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2019-11-20 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Dr. David Alan Gilbert,
Maxim Levitsky, Max Reitz
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2019-11-20 18:58 ` [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands Maxim Levitsky
@ 2019-11-27 8:38 ` Markus Armbruster
2020-01-27 11:04 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2019-11-27 8:38 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block,
Dr. David Alan Gilbert
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)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2019-11-27 8:38 ` Markus Armbruster
@ 2020-01-27 11:04 ` Maxim Levitsky
2020-01-27 13:44 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 11:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Dr. David Alan Gilbert, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2020-01-27 11:04 ` Maxim Levitsky
@ 2020-01-27 13:44 ` Markus Armbruster
2020-01-27 13:53 ` Maxim Levitsky
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2020-01-27 13:44 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, Dr. David Alan Gilbert, qemu-block,
qemu-devel
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".
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2020-01-27 13:44 ` Markus Armbruster
@ 2020-01-27 13:53 ` Maxim Levitsky
2020-01-28 19:35 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 32+ messages in thread
From: Maxim Levitsky @ 2020-01-27 13:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Max Reitz, Dr. David Alan Gilbert, qemu-block,
qemu-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
2020-01-27 13:53 ` Maxim Levitsky
@ 2020-01-28 19:35 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 32+ messages in thread
From: Dr. David Alan Gilbert @ 2020-01-28 19:35 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, Markus Armbruster, qemu-block, qemu-devel
* 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups
2019-11-20 18:58 [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Maxim Levitsky
` (8 preceding siblings ...)
2019-11-20 18:58 ` [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands Maxim Levitsky
@ 2019-11-22 10:15 ` Dr. David Alan Gilbert
2019-11-22 10:27 ` Kevin Wolf
9 siblings, 1 reply; 32+ messages in thread
From: Dr. David Alan Gilbert @ 2019-11-22 10:15 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Max Reitz, qemu-devel, qemu-block, Markus Armbruster
* 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups
2019-11-22 10:15 ` [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups Dr. David Alan Gilbert
@ 2019-11-22 10:27 ` Kevin Wolf
0 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2019-11-22 10:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Markus Armbruster, Max Reitz, qemu-devel, qemu-block,
Maxim Levitsky
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
^ permalink raw reply [flat|nested] 32+ messages in thread