| Message ID | 20200127103647.17761-14-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: [for 5.0]: HMP monitor handlers cleanups | expand |
On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote: > This way they all will be prefixed with 'Error:' which some parsers > (e.g libvirt) need > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169 > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> And I probably will keep that patch since it is not invasive and still does a nice refactoring. Thoughts? Best regards, Maxim Levitsky > --- > block/monitor/block-hmp-cmds.c | 35 ++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c > index 7bbe4e3814..5b060d380d 100644 > --- a/block/monitor/block-hmp-cmds.c > +++ b/block/monitor/block-hmp-cmds.c > @@ -84,7 +84,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; > } > @@ -98,7 +97,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; > @@ -109,6 +108,7 @@ err: > monitor_remove_blk(blk); > blk_unref(blk); > } > + hmp_handle_error(mon, err); > } > > void hmp_drive_del(Monitor *mon, const QDict *qdict) > @@ -130,14 +130,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); > @@ -146,9 +146,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); > @@ -169,12 +168,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")) { > @@ -185,12 +187,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); > @@ -202,8 +204,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)
Maxim Levitsky <mlevitsk@redhat.com> writes: > On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote: >> This way they all will be prefixed with 'Error:' which some parsers >> (e.g libvirt) need >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169 >> >> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > And I probably will keep that patch since it is not invasive and > still does a nice refactoring. > Thoughts? The commit message needs updating: libvirt doesn't actually need this anymore. What remains is "nice refactoring". Let's look at the patch from that perspective. > > Best regards, > Maxim Levitsky > >> --- >> block/monitor/block-hmp-cmds.c | 35 ++++++++++++++++++++-------------- >> 1 file changed, 21 insertions(+), 14 deletions(-) >> >> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c >> index 7bbe4e3814..5b060d380d 100644 >> --- a/block/monitor/block-hmp-cmds.c >> +++ b/block/monitor/block-hmp-cmds.c >> @@ -84,7 +84,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; >> } >> @@ -98,7 +97,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; >> @@ -109,6 +108,7 @@ err: >> monitor_remove_blk(blk); >> blk_unref(blk); >> } >> + hmp_handle_error(mon, err); >> } Is this really nicer? >> >> void hmp_drive_del(Monitor *mon, const QDict *qdict) >> @@ -130,14 +130,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); >> @@ -146,9 +146,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); >> @@ -169,12 +168,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) >> } >> >> aio_context_release(aio_context); >> +err: >> + hmp_handle_error(mon, local_err); >> } Likewise. >> >> 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")) { >> @@ -185,12 +187,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); >> @@ -202,8 +204,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) Likewise. Up to the code's maintainers Kevin & Max.
On Wed, 2020-01-29 at 11:27 +0100, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote: > > > This way they all will be prefixed with 'Error:' which some parsers > > > (e.g libvirt) need > > > > > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169 > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > And I probably will keep that patch since it is not invasive and > > still does a nice refactoring. > > Thoughts? > > The commit message needs updating: libvirt doesn't actually need this > anymore. > > What remains is "nice refactoring". Let's look at the patch from that > perspective. > > > > > Best regards, > > Maxim Levitsky > > > > > --- > > > block/monitor/block-hmp-cmds.c | 35 ++++++++++++++++++++-------------- > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c > > > index 7bbe4e3814..5b060d380d 100644 > > > --- a/block/monitor/block-hmp-cmds.c > > > +++ b/block/monitor/block-hmp-cmds.c > > > @@ -84,7 +84,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; > > > } > > > @@ -98,7 +97,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; > > > @@ -109,6 +108,7 @@ err: > > > monitor_remove_blk(blk); > > > blk_unref(blk); > > > } > > > + hmp_handle_error(mon, err); > > > } > > Is this really nicer? > > > > > > > void hmp_drive_del(Monitor *mon, const QDict *qdict) > > > @@ -130,14 +130,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); > > > @@ -146,9 +146,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); > > > @@ -169,12 +168,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > > > } > > > > > > aio_context_release(aio_context); > > > +err: > > > + hmp_handle_error(mon, local_err); > > > } > > Likewise. > > > > > > > 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")) { > > > @@ -185,12 +187,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); > > > @@ -202,8 +204,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) > > Likewise. > > Up to the code's maintainers Kevin & Max. Yep. In theory, if I understand correctly we should reduce usage of monitor_printf, but yes, without libvirt justification I don't mind dropping this. I hope that the rest of the patches will make it, so that this work is not wasted. Thanks for the review! Best regards, Maxim Levitsky
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 7bbe4e3814..5b060d380d 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -84,7 +84,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; } @@ -98,7 +97,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; @@ -109,6 +108,7 @@ err: monitor_remove_blk(blk); blk_unref(blk); } + hmp_handle_error(mon, err); } void hmp_drive_del(Monitor *mon, const QDict *qdict) @@ -130,14 +130,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); @@ -146,9 +146,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); @@ -169,12 +168,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")) { @@ -185,12 +187,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); @@ -202,8 +204,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)
This way they all will be prefixed with 'Error:' which some parsers (e.g libvirt) need Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169 Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/monitor/block-hmp-cmds.c | 35 ++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)