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