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 <mlevitsk@redhat.com>
---
 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);
 }
 
 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);
 }
 
 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)
-- 
2.17.2

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

And I probably will keep that patch since it is not invasive and
still does a nice refactoring.
Thoughts?

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

Maxim Levitsky <mlevitsk@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 <mlevitsk@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.

On Wed, 2020-01-29 at 11:27 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@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 <mlevitsk@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.
Yep.
In theory, if I understand correctly we should reduce usage of monitor_printf,
but yes, without libvirt justification I don't mind dropping this.

I hope that the rest of the patches will make it, so that this work is not
wasted.

Thanks for the review!

Best regards,
	Maxim Levitsky