On Wed, 2020-01-29 at 11:27 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevi...@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 <mlevi...@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
