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 <kw...@redhat.com>
> Cc: Max Reitz <mre...@redhat.com>
> Signed-off-by: Markus Armbruster <arm...@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);
>
signature.asc
Description: OpenPGP digital signature
