On 9/8/22 10:11, Richard Henderson wrote:
> On 9/6/22 12:55, Claudio Fontana wrote:
>> 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.
>>
>> Signed-off-by: Claudio Fontana <cfont...@suse.de>
>> ---
>> audio/audio.c | 6 +-
>> block.c | 12 +++-
>> block/dmg.c | 10 ++-
>> hw/core/qdev.c | 10 ++-
>> include/qemu/module.h | 10 +--
>> qom/object.c | 15 +++-
>> softmmu/qtest.c | 6 +-
>> ui/console.c | 19 +++++-
>> util/module.c | 155 ++++++++++++++++++++++++++++++------------
>> 9 files changed, 182 insertions(+), 61 deletions(-)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 76b8735b44..4f4bb10cce 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -72,6 +72,7 @@ void audio_driver_register(audio_driver *drv)
>> audio_driver *audio_driver_lookup(const char *name)
>> {
>> struct audio_driver *d;
>> + Error *local_err = NULL;
>>
>> QLIST_FOREACH(d, &audio_drivers, next) {
>> if (strcmp(name, d->name) == 0) {
>> @@ -79,7 +80,10 @@ audio_driver *audio_driver_lookup(const char *name)
>> }
>> }
>>
>> - audio_module_load_one(name);
>> + if (!audio_module_load_one(name, &local_err) && local_err) {
>> + error_report_err(local_err);
>> + }
>
> Why would local_err not be set on failure? Is this the NOTDIR thing? I
> guess this is
> sufficient to distinguish the two cases... The urge to bikeshed the return
> value is
> strong. :-)
>
>> +bool module_load_one(const char *prefix, const char *name, Error **errp);
>> +bool module_load_qom_one(const char *type, Error **errp);
>
> Doc comments go in the header file.
>
>> +/*
>> + * module_load_file: attempt to load a dso file
>> + *
>> + * fname: full pathname of the file to load
>> + * export_symbols: if true, add the symbols to the global name space
>> + * errp: error to set.
>> + *
>> + * Return value: 0 on success (found and loaded), < 0 on failure.
>> + * A return value of -ENOENT or -ENOTDIR means 'not found'.
>> + * -EINVAL is used as the generic error condition.
>> + *
>> + * Error: If fname is found, but could not be loaded, errp is set
>> + * with the error encountered during load.
>> + */
>> +static int module_load_file(const char *fname, bool export_symbols,
>> + Error **errp)
>> {
>> GModule *g_module;
>> void (*sym)(void);
>> @@ -152,16 +168,19 @@ static int module_load_file(const char *fname, bool
>> export_symbols)
>> int len = strlen(fname);
>> int suf_len = strlen(dsosuf);
>> ModuleEntry *e, *next;
>> - int ret, flags;
>> + int flags;
>>
>> if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
>> - /* wrong suffix */
>> - ret = -EINVAL;
>> - goto out;
>> + error_setg(errp, "wrong filename, missing suffix %s", dsosuf);
>> + return -EINVAL;
>> }
>> - if (access(fname, F_OK)) {
>> - ret = -ENOENT;
>> - goto out;
>> + if (access(fname, F_OK) != 0) {
>> + int ret = errno;
>> + if (ret != ENOENT && ret != ENOTDIR) {
>> + error_setg_errno(errp, ret, "error trying to access %s", fname);
>> + }
>> + /* most likely is EACCES here */
>> + return -ret;
>> }
>
> I looked at this a couple of days ago and came to the conclusion that the
> split between
> this function and its caller is wrong.
>
> The directory path probe loop is currently split between the two functions.
> I think the
> probe loop should be in the caller (i.e. the access call here moved out). I
> think
> module_load_file should only be called once the file is known to exist.
> Which then
> simplifies the set of errors to be returned from here.
>
> (I might even go so far as to say module_load_file should be moved into
> module_load_one,
> because there's not really much here, and it would reduce ifdefs.)
>
>
> r~
I missed that module_load_one contains basically an #ifdef CONFIG_MODULES
inside it. It's just a big #ifdef CONFIG_MDOULES in disguise,
it is really confusing. I'll try to make things more explicit.
