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. 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 <mlevi...@redhat.com> > --- > 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)