qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Claudio Fontana
Subject: Re: [PATCH 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Date: Thu, 8 Sep 2022 10:28:55 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0

Hi Richard, thanks for looking at this,

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 

Right, we need to distinguish the case where the module does not exist from the 
case where
the module exists but an error has occured while trying to load it.

In the distros we build one time with all the modules enabled, but then may 
deliver those modules
or not deliver them in a specific distro, or the user may or may not have 
installed the packages
containing those DSOs.

So failing to find a module may be just fine, while we want to report errors if 
a load goes wrong.
We just encountered a bug where the actual cause was hidden by the fact that no 
error was produced on module load.


> sufficient to distinguish the two cases...  The urge to bikeshed the return 
> value is 
> strong.  :-)

If you find a better way let me know, this is just the first thing that came to 
mind to distinguish the cases, ie:

return value true -> loaded successfully

return value false -> not loaded:
                      local_err set   -> error during load
                      local_err unset -> no error, just not present

> 
>> +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.)

I'll attempt to integrate your suggestions.

> 
> 
> r~

Thanks!

Claudio



reply via email to

[Prev in Thread] Current Thread [Next in Thread]