| Message ID | 20191120185850.18986-10-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: [for 5.0]: HMP monitor handlers cleanups | expand |
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 <mlevitsk@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 <mlevitsk@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)
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 <mlevitsk@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? > > 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 <mlevitsk@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. 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
Maxim Levitsky <mlevitsk@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 <mlevitsk@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". [...]
On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@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 <mlevitsk@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. 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
* Maxim Levitsky (mlevitsk@redhat.com) wrote: > On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote: > > Maxim Levitsky <mlevitsk@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 <mlevitsk@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. > Yes, also this way it is consistent Yes I also like it; I wouldn't worry too much about things parsing error messages for the exact error message; if anything is doing that then the corresponding case needs to have big red flags around it. Dave > > > > 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 > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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; } 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)
This way they all will be prefixed with 'Error:' which some parsers (e.g libvirt need) Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- blockdev-hmp-cmds.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)