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

[Claudio Fontana]

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?
> 
>> @@ -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~