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 <mre...@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 <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.
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);
>>
