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

[Markus Armbruster]

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.

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.

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.

That leaves the ones that are still reported with monitor_printf().
Converting those to error_report() looks far more tractable to me.

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

>      }
>  
>      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)