Maxim Levitsky <mlevi...@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 <mlevi...@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".
[...]
