Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:

[...]

>>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>>> be nice to have.
>>>
>>
>> I'll look into it.
>>
>
> I see that hmp_handle_error() takes an hmp argument that's unused and

Unused since commit 193227f9e5 (error: Use error_report_err() instead of
monitor_printf(), 2015-12-18).

I doubt it's worth removing.

> also that callers often do the redundant:
>
>    if (err) {
>       hmp_handle_error(err);
>    }

Would be nice to clean this up.

> But aside from those I don't see other improvements to be made, many
> callsites of hmp_handle_error() re-use the Error for other calls,
> e.g. block/monitor/block-hmp-cmds.c:
>
>     Error *err = NULL;
>     ...
>     if (!qdict_get_try_str(qdict, "node-name")) {
>         qobject_unref(qdict);
>         error_setg(&err, "'node-name' needs to be specified");
>         goto out;
>     }
>
>     BlockDriverState *bs = bds_tree_init(qdict, &err);
>     if (!bs) {
>         goto out;
>     }
>
>     bdrv_set_monitor_owned(bs);
>  out:
>     qemu_opts_del(opts);
>     hmp_handle_error(err);
>
> So having another helper I think would just create confusion as
> error_setg() is already a standard way of creating an error.

We have both error_report() and error_report_err(), and it doesn't lead
to confusion as far as I can tell.

Whether doing the same for HMP error reporting would be an improvement
is not obvious.  Let's move on.

[...]