Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error

[Markus Armbruster]

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 <address@hidden> 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 <address@hidden>
>> Cc: Max Reitz <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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);
>>