Philippe Mathieu-Daudé <f4...@amsat.org> writes: > On 16/9/22 11:27, Markus Armbruster wrote: >> Claudio Fontana <cfont...@suse.de> writes: >> >>> improve error handling during module load, by changing: >>> >>> bool module_load_one(const char *prefix, const char *lib_name); >>> void module_load_qom_one(const char *type); >>> >>> to: >>> >>> bool module_load_one(const char *prefix, const char *name, Error **errp); >>> bool module_load_qom_one(const char *type, Error **errp); >>> >>> 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. >> >> How exactly does behavior change? The commit message is mum on the >> behavior before the patch, and vague on the behavior afterwards. >> >>> Signed-off-by: Claudio Fontana <cfont...@suse.de> >>> --- >>> audio/audio.c | 9 ++- >>> block.c | 15 ++++- >>> block/dmg.c | 18 +++++- >>> hw/core/qdev.c | 10 ++- >>> include/qemu/module.h | 38 ++++++++++-- >>> qom/object.c | 18 +++++- >>> softmmu/qtest.c | 6 +- >>> ui/console.c | 18 +++++- >>> util/module.c | 140 ++++++++++++++++++++++++------------------ >>> 9 files changed, 194 insertions(+), 78 deletions(-) > >>> diff --git a/include/qemu/module.h b/include/qemu/module.h >>> index 8c012bbe03..78d4c4de96 100644 >>> --- a/include/qemu/module.h >>> +++ b/include/qemu/module.h >>> @@ -61,16 +61,44 @@ typedef enum { > >>> >>> void module_call_init(module_init_type type); >>> -bool module_load_one(const char *prefix, const char *lib_name); >>> -void module_load_qom_one(const char *type); >>> + >>> +/* >>> + * module_load_one: attempt to load a module from a set of directories >>> + * >>> + * directories searched are: >>> + * - getenv("QEMU_MODULE_DIR") >>> + * - get_relocated_path(CONFIG_QEMU_MODDIR); >>> + * - /var/run/qemu/${version_dir} >>> + * >>> + * prefix: a subsystem prefix, or the empty string ("audio-", ..., >>> "") >>> + * name: name of the module >>> + * errp: error to set in case the module is found, but load >>> failed. >>> + * >>> + * Return value: true on success (found and loaded); >>> + * if module if found, but load failed, errp will be set. >>> + * if module is not found, errp will not be set. >> >> I understand you need to distingush two failure modes "found, but load >> failed" and "not found". >> >> Functions that set an error on some failures only tend to be awkward: in >> addition to checking the return value for failure, you have to check >> @errp for special failures. This is particularly cumbersome when it >> requires a @local_err and an error_propagate() just for that. I >> generally prefer to return an error code and always set an error. > > I notice the same issue, therefore would suggest this alternative > prototype: > > bool module_load_one(const char *prefix, const char *name, > bool ignore_if_missing, Error **errp); > which always sets *errp when returning false: > > * Return value: if ignore_if_missing is true: > * true on success (found or missing), false on > * load failure. > * if ignore_if_missing is false: > * true on success (found and loaded); false if > * not found or load failed. > * errp will be set if the returned value is false. > */
I think this interface is less surprising. If having to pass a flag turns out to to be a legibility issue, we can have wrapper functions.