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

[Claudio Fontana]

On 9/8/22 19:10, Claudio Fontana wrote:
> On 9/8/22 18:03, Richard Henderson wrote:
>> 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?

is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg image is not 
compressed,
"dmg" can function even if the extra dmg-bz module is not loaded right?

I'd suspect we should then do:

if (!block_module_load_one("dmg-bz2", &local_err)) {
  if (local_err) {
     error_report_err(local_err);
     return -EINVAL;
  }
  warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */
}

and same for dmg-lzfse...?

Ciao,

C
     
>>> +        error_report_err(local_err);
>>> +    }
>>
>>> @@ -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.
> 
> 
> I am tempted then to change also other similar instances in the code.
> 
>>
>>> @@ -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~
>