Calling error_report() from within a a function that takes an Error **
argument is suspicious.  drive_new() does that, and its caller
drive_init_func() then exit()s.  Its caller main(), via
qemu_opts_foreach(), is fine with it, but clean it up anyway:

* Convert drive_new() to Error

* Update add_init_drive() to report the error received from
  drive_new().

* Make main() pass &error_fatal through qemu_opts_foreach(),
  drive_init_func() to drive_new()

* Make default_drive() pass &error_abort through qemu_opts_foreach(),
  drive_init_func() to drive_new()

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c                | 27 ++++++++++++++-------------
 device-hotplug.c          |  5 ++++-
 include/sysemu/blockdev.h |  3 ++-
 vl.c                      | 11 ++++-------
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a8755bd908..574adbcb7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
     },
 };
 
-DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
+                     Error **errp)
 {
     const char *value;
     BlockBackend *blk;
@@ -808,7 +809,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to,
                         &local_err);
         if (local_err) {
-            error_report_err(local_err);
+            error_propagate(errp, local_err);
             return NULL;
         }
     }
@@ -819,7 +820,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         bool writethrough;
 
         if (bdrv_parse_cache_mode(value, &flags, &writethrough) != 0) {
-            error_report("invalid cache option");
+            error_setg(errp, "invalid cache option");
             return NULL;
         }
 
@@ -847,7 +848,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
                                    &error_abort);
     qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
     if (local_err) {
-        error_report_err(local_err);
+        error_propagate(errp, local_err);
         goto fail;
     }
 
@@ -860,7 +861,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
             media = MEDIA_CDROM;
             read_only = true;
         } else {
-            error_report("'%s' invalid media", value);
+            error_setg(errp, "'%s' invalid media", value);
             goto fail;
         }
     }
@@ -885,7 +886,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
              type++) {
         }
         if (type == IF_COUNT) {
-            error_report("unsupported bus type '%s'", value);
+            error_setg(errp, "unsupported bus type '%s'", value);
             goto fail;
         }
     } else {
@@ -902,7 +903,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     if (index != -1) {
         if (bus_id != 0 || unit_id != -1) {
-            error_report("index cannot be used with bus and unit");
+            error_setg(errp, "index cannot be used with bus and unit");
             goto fail;
         }
         bus_id = drive_index_to_bus_id(type, index);
@@ -921,13 +922,13 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     }
 
     if (max_devs && unit_id >= max_devs) {
-        error_report("unit %d too big (max is %d)", unit_id, max_devs - 1);
+        error_setg(errp, "unit %d too big (max is %d)", unit_id, max_devs - 1);
         goto fail;
     }
 
     if (drive_get(type, bus_id, unit_id) != NULL) {
-        error_report("drive with bus=%d, unit=%d (index=%d) exists",
-                     bus_id, unit_id, index);
+        error_setg(errp, "drive with bus=%d, unit=%d (index=%d) exists",
+                   bus_id, unit_id, index);
         goto fail;
     }
 
@@ -970,7 +971,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     if (werror != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO &&
             type != IF_NONE) {
-            error_report("werror is not supported by this bus type");
+            error_setg(errp, "werror is not supported by this bus type");
             goto fail;
         }
         qdict_put_str(bs_opts, "werror", werror);
@@ -980,7 +981,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     if (rerror != NULL) {
         if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI &&
             type != IF_NONE) {
-            error_report("rerror is not supported by this bus type");
+            error_setg(errp, "rerror is not supported by this bus type");
             goto fail;
         }
         qdict_put_str(bs_opts, "rerror", rerror);
@@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     bs_opts = NULL;
     if (!blk) {
         if (local_err) {
-            error_report_err(local_err);
+            error_propagate(errp, local_err);
         }
         goto fail;
     } else {
diff --git a/device-hotplug.c b/device-hotplug.c
index cd427e2c76..6090d5f1e9 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -28,6 +28,7 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "sysemu/sysemu.h"
@@ -36,6 +37,7 @@
 
 static DriveInfo *add_init_drive(const char *optstr)
 {
+    Error *err = NULL;
     DriveInfo *dinfo;
     QemuOpts *opts;
     MachineClass *mc;
@@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
         return NULL;
 
     mc = MACHINE_GET_CLASS(current_machine);
-    dinfo = drive_new(opts, mc->block_default_type);
+    dinfo = drive_new(opts, mc->block_default_type, &err);
     if (!dinfo) {
+        error_report_err(err);
         qemu_opts_del(opts);
         return NULL;
     }
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 24954b94e0..d34c4920dc 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
-DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
+DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
+                     Error **errp);
 
 /* device-hotplug */
 
diff --git a/vl.c b/vl.c
index 0d25956b2f..101e0123d9 100644
--- a/vl.c
+++ b/vl.c
@@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
     BlockInterfaceType *block_default_type = opaque;
 
-    return drive_new(opts, *block_default_type) == NULL;
+    return drive_new(opts, *block_default_type, errp) == NULL;
 }
 
 static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
@@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
         drive_enable_snapshot(NULL, opts, NULL);
     }
 
-    dinfo = drive_new(opts, type);
-    assert(dinfo);
+    dinfo = drive_new(opts, type, &error_abort);
     dinfo->is_default = true;
 
 }
@@ -4348,10 +4347,8 @@ int main(int argc, char **argv, char **envp)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }
-    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
-                          &machine_class->block_default_type, NULL)) {
-        exit(1);
-    }
+    qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
+                      &machine_class->block_default_type, &error_fatal);
 
     default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
                   CDROM_OPTS);
-- 
2.17.1

On 10/8/18 12:31 PM, Markus Armbruster wrote:
> Calling error_report() from within a a function that takes an Error **

s/a a/a/

> argument is suspicious.  drive_new() does that, and its caller
> drive_init_func() then exit()s.  Its caller main(), via
> qemu_opts_foreach(), is fine with it, but clean it up anyway:
> 
> * Convert drive_new() to Error
> 
> * Update add_init_drive() to report the error received from
>    drive_new().
> 
> * Make main() pass &error_fatal through qemu_opts_foreach(),
>    drive_init_func() to drive_new()
> 
> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>    drive_init_func() to drive_new()
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   blockdev.c                | 27 ++++++++++++++-------------
>   device-hotplug.c          |  5 ++++-
>   include/sysemu/blockdev.h |  3 ++-
>   vl.c                      | 11 ++++-------
>   4 files changed, 24 insertions(+), 22 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 08.10.18 19:31, Markus Armbruster wrote:
> Calling error_report() from within a a function that takes an Error **
> argument is suspicious.  drive_new() does that, and its caller
> drive_init_func() then exit()s.

I'm afraid I don't quite follow you here.  There is no function here
that takes an Error ** already and then calls error_report().  There is
however drive_new() that does not take an Error **, consequentially
calls error_report(), and there is its caller drive_init_func() which
does take an Error ** but does not set it.

So while I fully agree with you to make drive_new() take an Error **
(and thus effectively fix drive_init_func()), I don't quite understand
this explanation.

(Furthermore, drive_init_func() does not exit().  It's main() that
exit()s after calling drive_init_func().)

> Its caller main(), via
> qemu_opts_foreach(), is fine with it, but clean it up anyway:
> 
> * Convert drive_new() to Error
> 
> * Update add_init_drive() to report the error received from
>   drive_new().
> 
> * Make main() pass &error_fatal through qemu_opts_foreach(),
>   drive_init_func() to drive_new()
> 
> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>   drive_init_func() to drive_new()
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c                | 27 ++++++++++++++-------------
>  device-hotplug.c          |  5 ++++-
>  include/sysemu/blockdev.h |  3 ++-
>  vl.c                      | 11 ++++-------
>  4 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a8755bd908..574adbcb7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {

[...]

> @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      bs_opts = NULL;
>      if (!blk) {
>          if (local_err) {
> -            error_report_err(local_err);
> +            error_propagate(errp, local_err);
>          }

Wait, what would be the case where blockdev_init() returns NULL but
*errp remains unse——— oh no.

There is only one case which is someone specified "format=help".  Hm.  I
suppose you are as unhappy as me about the fact that because of this
drive_new() cannot guarantee that *errp is set in case of an error.

I think it's ""fine"" (*gnashing teeth*) to keep it this way, but it
means that callers need to continue to check the return value and not
*errp alone.

>          goto fail;
>      } else {
> diff --git a/device-hotplug.c b/device-hotplug.c
> index cd427e2c76..6090d5f1e9 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/error.h"
>  #include "qemu/config-file.h"
>  #include "qemu/option.h"
>  #include "sysemu/sysemu.h"
> @@ -36,6 +37,7 @@
>  
>  static DriveInfo *add_init_drive(const char *optstr)
>  {
> +    Error *err = NULL;
>      DriveInfo *dinfo;
>      QemuOpts *opts;
>      MachineClass *mc;
> @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
>          return NULL;
>  
>      mc = MACHINE_GET_CLASS(current_machine);
> -    dinfo = drive_new(opts, mc->block_default_type);
> +    dinfo = drive_new(opts, mc->block_default_type, &err);
>      if (!dinfo) {
> +        error_report_err(err);
>          qemu_opts_del(opts);
>          return NULL;
>      }
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 24954b94e0..d34c4920dc 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
>  QemuOpts *drive_def(const char *optstr);
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
> +                     Error **errp);
>  
>  /* device-hotplug */
>  
> diff --git a/vl.c b/vl.c
> index 0d25956b2f..101e0123d9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      BlockInterfaceType *block_default_type = opaque;
>  
> -    return drive_new(opts, *block_default_type) == NULL;
> +    return drive_new(opts, *block_default_type, errp) == NULL;
>  }
>  
>  static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
> @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>          drive_enable_snapshot(NULL, opts, NULL);
>      }
>  
> -    dinfo = drive_new(opts, type);
> -    assert(dinfo);
> +    dinfo = drive_new(opts, type, &error_abort);

Which means the assertion is still necessary here.

>      dinfo->is_default = true;
>  
>  }
> @@ -4348,10 +4347,8 @@ int main(int argc, char **argv, char **envp)
>          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>                            NULL, NULL);
>      }
> -    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> -                          &machine_class->block_default_type, NULL)) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> +                      &machine_class->block_default_type, &error_fatal);

And we still have to keep an exit() here.

Alternative: You transform blockdev_init()'s format=help into an error
(or construct a new error in drive_new() if blockdev_init() returned
NULL but no error).

Max

>  
>      default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
>                    CDROM_OPTS);
> 

Copying Marc-André for a possible connection to his recent work on
improving help.  Marc-André, search for "format=help".  Just in case you
have further observations to offer.

Max Reitz <mreitz@redhat.com> writes:

> On 08.10.18 19:31, Markus Armbruster wrote:
>> Calling error_report() from within a a function that takes an Error **
>> argument is suspicious.  drive_new() does that, and its caller
>> drive_init_func() then exit()s.
>
> I'm afraid I don't quite follow you here.  There is no function here
> that takes an Error ** already and then calls error_report().  There is
> however drive_new() that does not take an Error **, consequentially
> calls error_report(), and there is its caller drive_init_func() which
> does take an Error ** but does not set it.
>
> So while I fully agree with you to make drive_new() take an Error **
> (and thus effectively fix drive_init_func()), I don't quite understand
> this explanation.
>
> (Furthermore, drive_init_func() does not exit().  It's main() that
> exit()s after calling drive_init_func().)
  
You're right.

There's about a dozen patches cleaning up pretty much the same thing,
and I reused the same commit message with the appropriate variations.  I
failed to vary this instance appropriately.  Sorry!

I'll fix this for v2.

>> Its caller main(), via
>> qemu_opts_foreach(), is fine with it, but clean it up anyway:
>> 
>> * Convert drive_new() to Error
>> 
>> * Update add_init_drive() to report the error received from
>>   drive_new().
>> 
>> * Make main() pass &error_fatal through qemu_opts_foreach(),
>>   drive_init_func() to drive_new()
>> 
>> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>>   drive_init_func() to drive_new()
>> 
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  blockdev.c                | 27 ++++++++++++++-------------
>>  device-hotplug.c          |  5 ++++-
>>  include/sysemu/blockdev.h |  3 ++-
>>  vl.c                      | 11 ++++-------
>>  4 files changed, 24 insertions(+), 22 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index a8755bd908..574adbcb7f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
>
> [...]
>
>> @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>>      bs_opts = NULL;
>>      if (!blk) {
>>          if (local_err) {
>> -            error_report_err(local_err);
>> +            error_propagate(errp, local_err);
>>          }
>
> Wait, what would be the case where blockdev_init() returns NULL but
> *errp remains unse——— oh no.
>
> There is only one case which is someone specified "format=help".  Hm.  I
> suppose you are as unhappy as me about the fact that because of this
> drive_new() cannot guarantee that *errp is set in case of an error.

That's an ugly interface wart we have in a few places.  In a nutshell, either

* succeed and return a value indicating success

* fail, set an error, and return a value indicating failure

* print help, leave error alone, and return a value indicating failure

> I think it's ""fine"" (*gnashing teeth*) to keep it this way, but it
> means that callers need to continue to check the return value and not
> *errp alone.

Yes, they need to check both.

Note that -device and -machine use a technique I consider cleaner:
there's a separate function FOO_help_func() to deal with providing help
before we do the actual work.  If the user asked for help,
FOO_help_func() prints some, and returns 1.  Else it returns 0.  This
lets main() call exit(0) after help.

>>          goto fail;
>>      } else {
>> diff --git a/device-hotplug.c b/device-hotplug.c
>> index cd427e2c76..6090d5f1e9 100644
>> --- a/device-hotplug.c
>> +++ b/device-hotplug.c
>> @@ -28,6 +28,7 @@
>>  #include "sysemu/block-backend.h"
>>  #include "sysemu/blockdev.h"
>>  #include "qapi/qmp/qdict.h"
>> +#include "qapi/error.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/option.h"
>>  #include "sysemu/sysemu.h"
>> @@ -36,6 +37,7 @@
>>  
>>  static DriveInfo *add_init_drive(const char *optstr)
>>  {
>> +    Error *err = NULL;
>>      DriveInfo *dinfo;
>>      QemuOpts *opts;
>>      MachineClass *mc;
>> @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
>>          return NULL;
>>  
>>      mc = MACHINE_GET_CLASS(current_machine);
>> -    dinfo = drive_new(opts, mc->block_default_type);
>> +    dinfo = drive_new(opts, mc->block_default_type, &err);
>>      if (!dinfo) {
>> +        error_report_err(err);
>>          qemu_opts_del(opts);
>>          return NULL;
>>      }
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index 24954b94e0..d34c4920dc 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
>>  QemuOpts *drive_def(const char *optstr);
>>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>>                      const char *optstr);
>> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
>> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
>> +                     Error **errp);
>>  
>>  /* device-hotplug */
>>  
>> diff --git a/vl.c b/vl.c
>> index 0d25956b2f..101e0123d9 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>  {
>>      BlockInterfaceType *block_default_type = opaque;
>>  
>> -    return drive_new(opts, *block_default_type) == NULL;
>> +    return drive_new(opts, *block_default_type, errp) == NULL;
>>  }
>>  
>>  static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
>> @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>>          drive_enable_snapshot(NULL, opts, NULL);
>>      }
>>  
>> -    dinfo = drive_new(opts, type);
>> -    assert(dinfo);
>> +    dinfo = drive_new(opts, type, &error_abort);
>
> Which means the assertion is still necessary here.

I see very little value in assert(p) right before *p.  Matter of taste,
I guess.  Do you want me to keep it?

>>      dinfo->is_default = true;
>>  
>>  }
>> @@ -4348,10 +4347,8 @@ int main(int argc, char **argv, char **envp)
>>          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>>                            NULL, NULL);
>>      }
>> -    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>> -                          &machine_class->block_default_type, NULL)) {
>> -        exit(1);
>> -    }
>> +    qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>> +                      &machine_class->block_default_type, &error_fatal);
>
> And we still have to keep an exit() here.

You're right.  But it should become exit(0).

> Alternative: You transform blockdev_init()'s format=help into an error
> (or construct a new error in drive_new() if blockdev_init() returned
> NULL but no error).

Another alternative would be separating out help.  Since I'd prefer to
keep this patch mostly mechanical, I'd rather do that on top if it's
desired.

>
> Max
>
>>  
>>      default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
>>                    CDROM_OPTS);
>> 

On 12.10.18 07:44, Markus Armbruster wrote:
> Copying Marc-André for a possible connection to his recent work on
> improving help.  Marc-André, search for "format=help".  Just in case you
> have further observations to offer.
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 08.10.18 19:31, Markus Armbruster wrote:
>>> Calling error_report() from within a a function that takes an Error **
>>> argument is suspicious.  drive_new() does that, and its caller
>>> drive_init_func() then exit()s.
>>
>> I'm afraid I don't quite follow you here.  There is no function here
>> that takes an Error ** already and then calls error_report().  There is
>> however drive_new() that does not take an Error **, consequentially
>> calls error_report(), and there is its caller drive_init_func() which
>> does take an Error ** but does not set it.
>>
>> So while I fully agree with you to make drive_new() take an Error **
>> (and thus effectively fix drive_init_func()), I don't quite understand
>> this explanation.
>>
>> (Furthermore, drive_init_func() does not exit().  It's main() that
>> exit()s after calling drive_init_func().)
>   
> You're right.
> 
> There's about a dozen patches cleaning up pretty much the same thing,
> and I reused the same commit message with the appropriate variations.  I
> failed to vary this instance appropriately.  Sorry!
> 
> I'll fix this for v2.
> 
>>> Its caller main(), via
>>> qemu_opts_foreach(), is fine with it, but clean it up anyway:
>>>
>>> * Convert drive_new() to Error
>>>
>>> * Update add_init_drive() to report the error received from
>>>   drive_new().
>>>
>>> * Make main() pass &error_fatal through qemu_opts_foreach(),
>>>   drive_init_func() to drive_new()
>>>
>>> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>>>   drive_init_func() to drive_new()
>>>
>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>> Cc: Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  blockdev.c                | 27 ++++++++++++++-------------
>>>  device-hotplug.c          |  5 ++++-
>>>  include/sysemu/blockdev.h |  3 ++-
>>>  vl.c                      | 11 ++++-------
>>>  4 files changed, 24 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index a8755bd908..574adbcb7f 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
>>
>> [...]
>>
>>> @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>>>      bs_opts = NULL;
>>>      if (!blk) {
>>>          if (local_err) {
>>> -            error_report_err(local_err);
>>> +            error_propagate(errp, local_err);
>>>          }
>>
>> Wait, what would be the case where blockdev_init() returns NULL but
>> *errp remains unse——— oh no.
>>
>> There is only one case which is someone specified "format=help".  Hm.  I
>> suppose you are as unhappy as me about the fact that because of this
>> drive_new() cannot guarantee that *errp is set in case of an error.
> 
> That's an ugly interface wart we have in a few places.  In a nutshell, either
> 
> * succeed and return a value indicating success
> 
> * fail, set an error, and return a value indicating failure
> 
> * print help, leave error alone, and return a value indicating failure
> 
>> I think it's ""fine"" (*gnashing teeth*) to keep it this way, but it
>> means that callers need to continue to check the return value and not
>> *errp alone.
> 
> Yes, they need to check both.
> 
> Note that -device and -machine use a technique I consider cleaner:
> there's a separate function FOO_help_func() to deal with providing help
> before we do the actual work.  If the user asked for help,
> FOO_help_func() prints some, and returns 1.  Else it returns 0.  This
> lets main() call exit(0) after help.

It is indeed cleaner.  I don't mind the current way too much, though, I
mostly just don't like effectively failing without setting the Error object.

>>>          goto fail;
>>>      } else {
>>> diff --git a/device-hotplug.c b/device-hotplug.c
>>> index cd427e2c76..6090d5f1e9 100644
>>> --- a/device-hotplug.c
>>> +++ b/device-hotplug.c
>>> @@ -28,6 +28,7 @@
>>>  #include "sysemu/block-backend.h"
>>>  #include "sysemu/blockdev.h"
>>>  #include "qapi/qmp/qdict.h"
>>> +#include "qapi/error.h"
>>>  #include "qemu/config-file.h"
>>>  #include "qemu/option.h"
>>>  #include "sysemu/sysemu.h"
>>> @@ -36,6 +37,7 @@
>>>  
>>>  static DriveInfo *add_init_drive(const char *optstr)
>>>  {
>>> +    Error *err = NULL;
>>>      DriveInfo *dinfo;
>>>      QemuOpts *opts;
>>>      MachineClass *mc;
>>> @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
>>>          return NULL;
>>>  
>>>      mc = MACHINE_GET_CLASS(current_machine);
>>> -    dinfo = drive_new(opts, mc->block_default_type);
>>> +    dinfo = drive_new(opts, mc->block_default_type, &err);
>>>      if (!dinfo) {
>>> +        error_report_err(err);
>>>          qemu_opts_del(opts);
>>>          return NULL;
>>>      }
>>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>>> index 24954b94e0..d34c4920dc 100644
>>> --- a/include/sysemu/blockdev.h
>>> +++ b/include/sysemu/blockdev.h
>>> @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
>>>  QemuOpts *drive_def(const char *optstr);
>>>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>>>                      const char *optstr);
>>> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
>>> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
>>> +                     Error **errp);
>>>  
>>>  /* device-hotplug */
>>>  
>>> diff --git a/vl.c b/vl.c
>>> index 0d25956b2f..101e0123d9 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>>  {
>>>      BlockInterfaceType *block_default_type = opaque;
>>>  
>>> -    return drive_new(opts, *block_default_type) == NULL;
>>> +    return drive_new(opts, *block_default_type, errp) == NULL;
>>>  }
>>>  
>>>  static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
>>> @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>>>          drive_enable_snapshot(NULL, opts, NULL);
>>>      }
>>>  
>>> -    dinfo = drive_new(opts, type);
>>> -    assert(dinfo);
>>> +    dinfo = drive_new(opts, type, &error_abort);
>>
>> Which means the assertion is still necessary here.
> 
> I see very little value in assert(p) right before *p.  Matter of taste,
> I guess.  Do you want me to keep it?

True.  "An assertion looks better to the user" isn't an argument,
considering the user shouldn't ever see assertions either.

So feel free to drop it indeed.

>>>      dinfo->is_default = true;
>>>  
>>>  }
>>> @@ -4348,10 +4347,8 @@ int main(int argc, char **argv, char **envp)
>>>          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>>>                            NULL, NULL);
>>>      }
>>> -    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>>> -                          &machine_class->block_default_type, NULL)) {
>>> -        exit(1);
>>> -    }
>>> +    qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>>> +                      &machine_class->block_default_type, &error_fatal);
>>
>> And we still have to keep an exit() here.
> 
> You're right.  But it should become exit(0).

That makes sense, yes.

>> Alternative: You transform blockdev_init()'s format=help into an error
>> (or construct a new error in drive_new() if blockdev_init() returned
>> NULL but no error).
> 
> Another alternative would be separating out help.  Since I'd prefer to
> keep this patch mostly mechanical, I'd rather do that on top if it's
> desired.

I'm not sure how much I desire it, so I won't push you to do so.  But
it's definitely fine to keep it out of this patch.

Max

Am 12.10.2018 um 14:28 hat Max Reitz geschrieben:
> >>> @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
> >>>          drive_enable_snapshot(NULL, opts, NULL);
> >>>      }
> >>>  
> >>> -    dinfo = drive_new(opts, type);
> >>> -    assert(dinfo);
> >>> +    dinfo = drive_new(opts, type, &error_abort);
> >>
> >> Which means the assertion is still necessary here.
> > 
> > I see very little value in assert(p) right before *p.  Matter of taste,
> > I guess.  Do you want me to keep it?
> 
> True.  "An assertion looks better to the user" isn't an argument,
> considering the user shouldn't ever see assertions either.

The point that could be made is that it documents that we're aware that
drive_new() can return NULL generally, but we've made sure that with the
specific options passed it doesn't happen here, so not having any error
handling is not a bug, but intended.

> So feel free to drop it indeed.

I don't mind either way.

Kevin