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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6026 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]