Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Claudio Fontana <cfontana@suse.de>]

On 9/30/22 13:50, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 9/28/22 13:31, Markus Armbruster wrote:
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> improve error handling during module load, by changing:
>>>>
>>>> bool module_load(const char *prefix, const char *lib_name);
>>>> void module_load_qom(const char *type);
>>>>
>>>> to:
>>>>
>>>> int module_load(const char *prefix, const char *name, Error **errp);
>>>> int module_load_qom(const char *type, Error **errp);
>>>>
>>>> where the return value is:
>>>>
>>>>  -1 on module load error, and errp is set with the error
>>>>   0 on module or one of its dependencies are not installed
>>>>   1 on module load success
>>>>   2 on module load success (module already loaded or built-in)
>>>
>>> Two changes, if I understand things correctly:
>>>
>>> 1. Convert to Error API from fprintf(stderr, ...)
>>>
>>> 2. Return a more useful value
>>>
>>> Right?
>>
>> Yes.
>>
>>>
>>> Do you add any new errors here that weren't reported before?  Just
>>
>> Yes.
> 
> Thanks!
> 
>>> trying to calibrate my expectations before I dig into the actual patch.
>>>
>>>> module_load_qom_one has been introduced in:
>>>>
>>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>>> module_load_one, but discarded the bool return value. Restore it.
>>>>
>>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>>> as appropriate in each context.
>>>>
>>>> Some memory leaks also fixed as part of the module_load changes.
> 
> [...]
> 
>>>> audio: when attempting to load an audio module, report module load errors.
>>>> block: when attempting to load a block module, report module load errors.
>>>> console: when attempting to load a display module, report module load errors.
>>>>
>>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>>>       If a module cannot be loaded to create that device, now abort execution.
>>>>
>>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>>>               report module load errors.
>>>>
>>>> qtest: when processing the "module_load" qtest command, report errors
>>>>        in the load of the module.
>>>
>>> This looks like a list of behavioral changes.  Appreciated!  It's a bit
>>> terse, though.  I might come back to this and suggest improvement.  But
>>> first, I need to understand the patch.
>>>
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>>>  audio/audio.c         |  16 ++--
>>>>  block.c               |  20 +++-
>>>>  block/dmg.c           |  14 ++-
>>>>  hw/core/qdev.c        |  17 +++-
>>>>  include/qemu/module.h |  37 +++++++-
>>>>  qom/object.c          |  18 +++-
>>>>  softmmu/qtest.c       |   8 +-
>>>>  ui/console.c          |  18 +++-
>>>>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>>>>  9 files changed, 235 insertions(+), 124 deletions(-)
>>>>
>>>> diff --git a/audio/audio.c b/audio/audio.c
>>>> index 0a682336a0..ea51793843 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>>>>  audio_driver *audio_driver_lookup(const char *name)
>>>>  {
>>>>      struct audio_driver *d;
>>>> +    Error *local_err = NULL;
>>>> +    int rv;
>>>>  
>>>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>>>          if (strcmp(name, d->name) == 0) {
>>>>              return d;
>>>>          }
>>>>      }
>>>> -
>>>> -    audio_module_load(name);
>>>> -    QLIST_FOREACH(d, &audio_drivers, next) {
>>>> -        if (strcmp(name, d->name) == 0) {
>>>> -            return d;
>>>> +    rv = audio_module_load(name, &local_err);
>>>> +    if (rv > 0) {
>>>> +        QLIST_FOREACH(d, &audio_drivers, next) {
>>>> +            if (strcmp(name, d->name) == 0) {
>>>> +                return d;
>>>> +            }
>>>>          }
>>>> +    } else if (rv < 0) {
>>>> +        error_report_err(local_err);
>>>>      }
>>>> -
>>>>      return NULL;
>>>>  }
>>>>  
>>>
>>> Before: audio_module_load() reports to stderr, but the caller can't
>>
>> before: reports _some_ errors to stderr
> 
> Thanks for the reminder.
> 
>>> know.  So, error or no error, search the driver registry for the one we
>>> want.  Return it if found, else fail.
>>>
>>> After: if audio_module_load() fails, report to stderr or current
>>> monitor, and fail.  If it could find no module or loaded one, search the
>>> driver registry.  Return it if found, else fail.
>>>
>>> What if audio_module_load() fails, but a search for the driver succeeds?
>>> Before the patch, we succeed.  
>>
>> audio_module_load() is the only way that audio_drivers can be updated and the search would return a different result.
> 
> Not true.
> 
> @audio_driver gets built with audio_driver_register().  Audio drivers
> call it via type_init().  For instance:
> 
>     static void register_audio_none(void)
>     {
>         audio_driver_register(&no_audio_driver);
>     }
>     type_init(register_audio_none);
> 
> My build of qemu-system-x86_64 puts "none", "wav", "alsa", "oss", "pa",
> "sdl", and "spice" into @audio_driver without entering module_load().
> 
>> The previous code was just lazily running the same code twice to make it simpler for the programmer in those conditions.
>>
>> Afterwards, we fail.  Can this happen?
>>
>> No.
> 
> It can't, but not for the reason you claimed.  The error in my argument
> was a misreading of the patch: we *still* only call module_load() when
> the driver is not in @audio_driver.
> 
> I wish I was better at reading patches.  And, if I may be so frank, I
> wish you were better at identifying my errors.  Telling me I'm wrong
> when I actually am wrong is helpful (thanks!), but the way you told me
> this time made me waste time chasing phantoms.
> 
> [...]
> 

Hi Markus,

sorry I missed this email from you, I am just back from sick leave.

The latest is v9 at:

https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg05092.html

And it's fully reviewed from my perspective, anything else that needs to be done to make this series proceed?

thanks,

Claudio