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

[Markus Armbruster <armbru@redhat.com>]

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.

[...]