Maxim Levitsky <mlevi...@redhat.com> writes:
> On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote:
>> This way they all will be prefixed with 'Error:' which some parsers
>> (e.g libvirt) need
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>>
>> Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
>
> And I probably will keep that patch since it is not invasive and
> still does a nice refactoring.
> Thoughts?
The commit message needs updating: libvirt doesn't actually need this
anymore.
What remains is "nice refactoring". Let's look at the patch from that
perspective.
>
> Best regards,
> Maxim Levitsky
>
>> ---
>> block/monitor/block-hmp-cmds.c | 35 ++++++++++++++++++++--------------
>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
>> index 7bbe4e3814..5b060d380d 100644
>> --- a/block/monitor/block-hmp-cmds.c
>> +++ b/block/monitor/block-hmp-cmds.c
>> @@ -84,7 +84,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;
>> }
>> @@ -98,7 +97,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;
>> @@ -109,6 +108,7 @@ err:
>> monitor_remove_blk(blk);
>> blk_unref(blk);
>> }
>> + hmp_handle_error(mon, err);
>> }
Is this really nicer?
>>
>> void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> @@ -130,14 +130,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);
>> @@ -146,9 +146,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);
>> @@ -169,12 +168,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> }
>>
>> aio_context_release(aio_context);
>> +err:
>> + hmp_handle_error(mon, local_err);
>> }
Likewise.
>>
>> 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")) {
>> @@ -185,12 +187,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);
>> @@ -202,8 +204,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)
Likewise.
Up to the code's maintainers Kevin & Max.
