Re: [PATCH 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Claudio Fontana]

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 <cfontana@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.