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

[Richard Henderson <richard.henderson@linaro.org>]

On 9/8/22 15:53, Claudio Fontana wrote:
> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>           return -EINVAL;
>       }
>   
> -    block_module_load_one("dmg-bz2");
> -    block_module_load_one("dmg-lzfse");
> +    if (!block_module_load_one("dmg-bz2", &local_err) && local_err) {
> +        error_report_err(local_err);
> +    }
> +    local_err = NULL;
> +    if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) {
> +        error_report_err(local_err);
> +    }
>   
>       s->n_chunks = 0;
>       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;

I wonder if these shouldn't fail hard if the modules don't exist?
Or at least pass back the error.

Kevin?

> @@ -1033,7 +1039,10 @@ ObjectClass *module_object_class_by_name(const char *typename)
>       oc = object_class_by_name(typename);
>   #ifdef CONFIG_MODULES
>       if (!oc) {
> -        module_load_qom_one(typename);
> +        Error *local_err = NULL;
> +        if (!module_load_qom_one(typename, &local_err) && local_err) {
> +            error_report_err(local_err);
> +        }

You can return NULL from this path, we know it failed.

> @@ -172,46 +170,38 @@ static int module_load_file(const char *fname, bool export_symbols)
>       }
>       g_module = g_module_open(fname, flags);
>       if (!g_module) {
> -        fprintf(stderr, "Failed to open module: %s\n",
> -                g_module_error());
> -        ret = -EINVAL;
> -        goto out;
> +        error_setg(errp, "failed to open module: %s", g_module_error());
> +        return false;
>       }
>       if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
> -        fprintf(stderr, "Failed to initialize module: %s\n",
> -                fname);
> -        /* Print some info if this is a QEMU module (but from different build),
> -         * this will make debugging user problems easier. */
> +        error_setg(errp, "failed to initialize module: %s", fname);
> +        /*
> +         * Print some info if this is a QEMU module (but from different build),
> +         * this will make debugging user problems easier.
> +         */
>           if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
> -            fprintf(stderr,
> -                    "Note: only modules from the same build can be loaded.\n");
> +            error_append_hint(errp,
> +                              "Only modules from the same build can be loaded");

With error_append_hint, you add the newline.

The rest of the util/module.c reorg looks good.


r~