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

[Markus Armbruster]

Maxim Levitsky <address@hidden> 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 <address@hidden> 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".

[...]