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

[Maxim Levitsky]

On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote:
> 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.
Yes, also this way it is consistent

> 
> Perhaps you can give some examples where the proposed decoration helps.
It helps to tag most monitor messages with error prefix which was the root 
cause of
me starting to work on this refactoring.
You suggested this, and I kind of like that idea.

> 
> > > 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".
Yep, the cover letter is outdated, now this patch series touch way
more that the block layer.

Best regards,
        Maxim Levitsky