[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 3/5] module: add Error arguments to module_load and module
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom |
Date: |
Fri, 30 Sep 2022 13:50:55 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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.
[...]