qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
Date: Fri, 12 Oct 2018 14:28:53 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

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 <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.

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]