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

[Richard Henderson]

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~