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

[Maxim Levitsky]

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?

> 
> 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.

> 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  blockdev-hmp-cmds.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index c943dccd03..197994716f 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -59,7 +59,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;
> >      }
> > @@ -73,7 +72,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;
> > @@ -84,6 +83,7 @@ err:
> >          monitor_remove_blk(blk);
> >          blk_unref(blk);
> >      }
> > +    hmp_handle_error(mon, &err);
> >  }
> >  
> >  void hmp_drive_del(Monitor *mon, const QDict *qdict)
> > @@ -105,14 +105,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;
> 
> Having to create Error objects just so we can use hmp_handle_error() is
> awkward.  Tolerable if using hmp_handle_error() improves matters.  I'm
> not sure it does.

Well, if we go with your suggestion of adding the 'error' prefix to vreport
that will not be needed. In fact then the only use of hmp_handle_error
would be to skip reporting if err is NULL, thus this function can be either
dropped or moved to common monitor code.

> 
> >      }
> >  
> >      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);
> > @@ -121,9 +121,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);
> > @@ -144,12 +143,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      aio_context_release(aio_context);
> > +err:
> > +    hmp_handle_error(mon, &local_err);
> >  }
> >  
> >  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")) {
> > @@ -160,12 +162,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);
> > @@ -177,8 +179,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)
> 
> 

Best regards,
        Maxim Levitsky