On 9/30/22 13:50, Markus Armbruster wrote: > Claudio Fontana <[email protected]> writes: > >> On 9/28/22 13:31, Markus Armbruster wrote: >>> Claudio Fontana <[email protected]> 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 <[email protected]> >>>> --- >>>> 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
