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~
