| Message ID | 20220908145308.30282-3-cfontana@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | improve error handling for module load | expand |
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~
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~
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~ >
Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > 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? Indeed. The code seems to consider that the modules may not be present. The behaviour in these cases is questionable (it seems to silently leave the buffers as they are and return success), but the modules are clearly optional. > 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...? Actually, I think during initialisation, we should just pass NULL as errp and ignore any errors. When a request would access a block that can't be uncompressed because of the missing module, that's where we can have a warn_report_once() and arguably should fail the I/O request. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> 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? > > Indeed. The code seems to consider that the modules may not be present. > The behaviour in these cases is questionable (it seems to silently leave > the buffers as they are and return success), but the modules are clearly > optional. > >> 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...? > > Actually, I think during initialisation, we should just pass NULL as > errp and ignore any errors. > > When a request would access a block that can't be uncompressed because > of the missing module, that's where we can have a warn_report_once() and > arguably should fail the I/O request. Seems like asking for data corruption. To avoid it, the complete stack needs to handle I/O errors correctly. Can we detect presence of compressed blocks on open?
On 9/20/22 18:50, Kevin Wolf wrote: > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> 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? > > Indeed. The code seems to consider that the modules may not be present. > The behaviour in these cases is questionable (it seems to silently leave > the buffers as they are and return success), but the modules are clearly > optional. > >> 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...? > > Actually, I think during initialisation, we should just pass NULL as > errp and ignore any errors. Hmm really? I'd think that if there is an actual error loading the module (module is installed, but the loading itself fails due to broken module, wrong permissions, I/O errors etc) we would want to report that fact as it happens? > > When a request would access a block that can't be uncompressed because > of the missing module, that's where we can have a warn_report_once() and > arguably should fail the I/O request. > > Kevin > That would mean, moving the warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") to the uncompression code and change it to a warn_report_once() right? Thanks, Claudio
Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >> 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? > > > > Indeed. The code seems to consider that the modules may not be present. > > The behaviour in these cases is questionable (it seems to silently leave > > the buffers as they are and return success), but the modules are clearly > > optional. > > > >> 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...? > > > > Actually, I think during initialisation, we should just pass NULL as > > errp and ignore any errors. > > > > When a request would access a block that can't be uncompressed because > > of the missing module, that's where we can have a warn_report_once() and > > arguably should fail the I/O request. > > Seems like asking for data corruption. To avoid it, the complete stack > needs to handle I/O errors correctly. If you have any component that doesn't handle I/O errors correctly, keep it far away from your data because it _will_ cause corruption eventually. The earlier it fails, the better for you. I don't think we should put great effort into making fundamentally broken software a little bit less broken in the corner case that you're least likely to hit. > Can we detect presence of compressed blocks on open? We seem to read in the full metadata of the image in dmg_open(). So I think it would be possible to detect it there. dmg_read_mish_block() is what fills in s->types. However, it never fills in types that it doesn't know (and it pretends it doesn't know the types of compressed blocks whose module is not loaded). So instead of checking it in dmg_open() after dmg_read_mish_block() has completed, you would have to catch the situation already in dmg_read_mish_block() while parsing the image file, which should be entirely doable if you want. This is a change in dmg's behaviour, though, which is not the goal of the proposed patch. So if we want to do that, it should be a separate patch. Kevin
Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > On 9/20/22 18:50, Kevin Wolf wrote: > > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >> 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? > > > > Indeed. The code seems to consider that the modules may not be present. > > The behaviour in these cases is questionable (it seems to silently leave > > the buffers as they are and return success) I think I misunderstood the code here actually. dmg_read_mish_block() skips chunks of unknown type, so later trying to find them fails and dmg_co_preadv() returns -EIO. Which is a reasonable return value for this. > > , but the modules are clearly > > optional. > > > >> 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...? > > > > Actually, I think during initialisation, we should just pass NULL as > > errp and ignore any errors. > > Hmm really? I'd think that if there is an actual error loading the > module (module is installed, but the loading itself fails due to > broken module, wrong permissions, I/O errors etc) we would want to > report that fact as it happens? Can we distinguish the two error cases? Oooh... Reading the code again carefully, are you returning false without setting errp if the module just couldn't be found? This is a surprising interface. Yes, I guess then your proposed code is fine (modulo moving warn_report() somewhere else so that it doesn't complain when the image doesn't even contain compressed chunks). > > When a request would access a block that can't be uncompressed because > > of the missing module, that's where we can have a warn_report_once() and > > arguably should fail the I/O request. > > > > Kevin > > > > That would mean, moving the > > warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > > to the uncompression code and change it to a warn_report_once() right? Yeah, though I think this doesn't actually work because we never even stored the metadata for chunks of unknown type (see above), so we never reach the uncompression code. What misled me initially is this code in dmg_read_chunk(): case UDBZ: /* bzip2 compressed */ if (!dmg_uncompress_bz2) { break; } I believe this is dead code, it could actually be an assertion. So if I'm not missing anything, adding the warning there would be useless. The other option is moving it into dmg_is_known_block_type() or its caller dmg_read_mish_block(), then we would detect it during open, which is probably nicer anyway. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> >> 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? >> > >> > Indeed. The code seems to consider that the modules may not be present. >> > The behaviour in these cases is questionable (it seems to silently leave >> > the buffers as they are and return success), but the modules are clearly >> > optional. >> > >> >> 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...? >> > >> > Actually, I think during initialisation, we should just pass NULL as >> > errp and ignore any errors. >> > >> > When a request would access a block that can't be uncompressed because >> > of the missing module, that's where we can have a warn_report_once() and >> > arguably should fail the I/O request. >> >> Seems like asking for data corruption. To avoid it, the complete stack >> needs to handle I/O errors correctly. > > If you have any component that doesn't handle I/O errors correctly, keep > it far away from your data because it _will_ cause corruption eventually. > The earlier it fails, the better for you. True for immortals. Since we're not, it actually depends on probabilities. > I don't think we should put great effort into making fundamentally > broken software a little bit less broken in the corner case that you're > least likely to hit. Fair point. >> Can we detect presence of compressed blocks on open? > > We seem to read in the full metadata of the image in dmg_open(). So I > think it would be possible to detect it there. > > dmg_read_mish_block() is what fills in s->types. However, it never fills > in types that it doesn't know (and it pretends it doesn't know the types > of compressed blocks whose module is not loaded). So instead of checking > it in dmg_open() after dmg_read_mish_block() has completed, you would > have to catch the situation already in dmg_read_mish_block() while > parsing the image file, which should be entirely doable if you want. In most cases, "open fails because some blocks are known to be unreadable" is much better UX than "everything goes swimmingly until you try to read one of the known-unreadable blocks". Even when your software manages not to eat your data, surprise bad blocks are still likely to result in a bad day. > This is a change in dmg's behaviour, though, which is not the goal of > the proposed patch. So if we want to do that, it should be a separate > patch. Makes sense.
Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: > >> Can we detect presence of compressed blocks on open? > > > > We seem to read in the full metadata of the image in dmg_open(). So I > > think it would be possible to detect it there. > > > > dmg_read_mish_block() is what fills in s->types. However, it never fills > > in types that it doesn't know (and it pretends it doesn't know the types > > of compressed blocks whose module is not loaded). So instead of checking > > it in dmg_open() after dmg_read_mish_block() has completed, you would > > have to catch the situation already in dmg_read_mish_block() while > > parsing the image file, which should be entirely doable if you want. > > In most cases, "open fails because some blocks are known to be > unreadable" is much better UX than "everything goes swimmingly until you > try to read one of the known-unreadable blocks". > > Even when your software manages not to eat your data, surprise bad > blocks are still likely to result in a bad day. That's fair. On the other hand, not allowing the user to read the part of data that is perfectly readable would be bad, too. Maybe the right solution would be to have a driver option like "unknown-block-types=io-error|fail-open" (probably with better names), and then having "fail-open" as the new default would be reasonable enough. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: >> >> Can we detect presence of compressed blocks on open? >> > >> > We seem to read in the full metadata of the image in dmg_open(). So I >> > think it would be possible to detect it there. >> > >> > dmg_read_mish_block() is what fills in s->types. However, it never fills >> > in types that it doesn't know (and it pretends it doesn't know the types >> > of compressed blocks whose module is not loaded). So instead of checking >> > it in dmg_open() after dmg_read_mish_block() has completed, you would >> > have to catch the situation already in dmg_read_mish_block() while >> > parsing the image file, which should be entirely doable if you want. >> >> In most cases, "open fails because some blocks are known to be >> unreadable" is much better UX than "everything goes swimmingly until you >> try to read one of the known-unreadable blocks". >> >> Even when your software manages not to eat your data, surprise bad >> blocks are still likely to result in a bad day. > > That's fair. On the other hand, not allowing the user to read the part > of data that is perfectly readable would be bad, too. > > Maybe the right solution would be to have a driver option like > "unknown-block-types=io-error|fail-open" (probably with better names), > and then having "fail-open" as the new default would be reasonable > enough. Makes sense.
On 9/21/22 13:56, Kevin Wolf wrote: > Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >> On 9/20/22 18:50, Kevin Wolf wrote: >>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>> 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? >>> >>> Indeed. The code seems to consider that the modules may not be present. >>> The behaviour in these cases is questionable (it seems to silently leave >>> the buffers as they are and return success) > > I think I misunderstood the code here actually. dmg_read_mish_block() > skips chunks of unknown type, so later trying to find them fails and > dmg_co_preadv() returns -EIO. Which is a reasonable return value for > this. > >>> , but the modules are clearly >>> optional. >>> >>>> 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...? >>> >>> Actually, I think during initialisation, we should just pass NULL as >>> errp and ignore any errors. >> >> Hmm really? I'd think that if there is an actual error loading the >> module (module is installed, but the loading itself fails due to >> broken module, wrong permissions, I/O errors etc) we would want to >> report that fact as it happens? > > Can we distinguish the two error cases? > > Oooh... Reading the code again carefully, are you returning false > without setting errp if the module just couldn't be found? This is a > surprising interface. > > Yes, I guess then your proposed code is fine (modulo moving > warn_report() somewhere else so that it doesn't complain when the image > doesn't even contain compressed chunks). > >>> When a request would access a block that can't be uncompressed because >>> of the missing module, that's where we can have a warn_report_once() and >>> arguably should fail the I/O request. >>> >>> Kevin >>> >> >> That would mean, moving the >> >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >> >> to the uncompression code and change it to a warn_report_once() right? > > Yeah, though I think this doesn't actually work because we never even > stored the metadata for chunks of unknown type (see above), so we never > reach the uncompression code. > > What misled me initially is this code in dmg_read_chunk(): > > case UDBZ: /* bzip2 compressed */ > if (!dmg_uncompress_bz2) { > break; > } > > I believe this is dead code, it could actually be an assertion. So > if I'm not missing anything, adding the warning there would be useless. > > The other option is moving it into dmg_is_known_block_type() or its > caller dmg_read_mish_block(), then we would detect it during open, which > is probably nicer anyway. > > Kevin > > Hi Kevin, I got a bit lost on whether we have some agreement on where if anywhere to move the check/warning about missing decompression submodules. If that's ok I'd post a V5, and we can rediscuss from the new starting point? Thanks, Claudio
Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: > On 9/21/22 13:56, Kevin Wolf wrote: > > Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > >> On 9/20/22 18:50, Kevin Wolf wrote: > >>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >>>> 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? > >>> > >>> Indeed. The code seems to consider that the modules may not be present. > >>> The behaviour in these cases is questionable (it seems to silently leave > >>> the buffers as they are and return success) > > > > I think I misunderstood the code here actually. dmg_read_mish_block() > > skips chunks of unknown type, so later trying to find them fails and > > dmg_co_preadv() returns -EIO. Which is a reasonable return value for > > this. > > > >>> , but the modules are clearly > >>> optional. > >>> > >>>> 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...? > >>> > >>> Actually, I think during initialisation, we should just pass NULL as > >>> errp and ignore any errors. > >> > >> Hmm really? I'd think that if there is an actual error loading the > >> module (module is installed, but the loading itself fails due to > >> broken module, wrong permissions, I/O errors etc) we would want to > >> report that fact as it happens? > > > > Can we distinguish the two error cases? > > > > Oooh... Reading the code again carefully, are you returning false > > without setting errp if the module just couldn't be found? This is a > > surprising interface. > > > > Yes, I guess then your proposed code is fine (modulo moving > > warn_report() somewhere else so that it doesn't complain when the image > > doesn't even contain compressed chunks). > > > >>> When a request would access a block that can't be uncompressed because > >>> of the missing module, that's where we can have a warn_report_once() and > >>> arguably should fail the I/O request. > >>> > >>> Kevin > >>> > >> > >> That would mean, moving the > >> > >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > >> > >> to the uncompression code and change it to a warn_report_once() right? > > > > Yeah, though I think this doesn't actually work because we never even > > stored the metadata for chunks of unknown type (see above), so we never > > reach the uncompression code. > > > > What misled me initially is this code in dmg_read_chunk(): > > > > case UDBZ: /* bzip2 compressed */ > > if (!dmg_uncompress_bz2) { > > break; > > } > > > > I believe this is dead code, it could actually be an assertion. So > > if I'm not missing anything, adding the warning there would be useless. > > > > The other option is moving it into dmg_is_known_block_type() or its > > caller dmg_read_mish_block(), then we would detect it during open, which > > is probably nicer anyway. > > > > Kevin > > > > > > Hi Kevin, I got a bit lost on whether we have some agreement on where > if anywhere to move the check/warning about missing decompression > submodules. > > If that's ok I'd post a V5, and we can rediscuss from the new starting > point? Sure, feel free, though I don't think the code will otherwise change for dmg, so we could as well continue here. My conclusion was that only dmg_read_mish_block() or something called by it can know whether compressed blocks exist in the image when the modules aren't present. So if we want to make the warning conditional on that (and my understanding is correct), this is where a warn_report_once() would have to live. Kevin
On 9/23/22 16:42, Kevin Wolf wrote: > Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: >> On 9/21/22 13:56, Kevin Wolf wrote: >>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >>>> On 9/20/22 18:50, Kevin Wolf wrote: >>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>>>> 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? >>>>> >>>>> Indeed. The code seems to consider that the modules may not be present. >>>>> The behaviour in these cases is questionable (it seems to silently leave >>>>> the buffers as they are and return success) >>> >>> I think I misunderstood the code here actually. dmg_read_mish_block() >>> skips chunks of unknown type, so later trying to find them fails and >>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for >>> this. >>> >>>>> , but the modules are clearly >>>>> optional. >>>>> >>>>>> 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...? >>>>> >>>>> Actually, I think during initialisation, we should just pass NULL as >>>>> errp and ignore any errors. >>>> >>>> Hmm really? I'd think that if there is an actual error loading the >>>> module (module is installed, but the loading itself fails due to >>>> broken module, wrong permissions, I/O errors etc) we would want to >>>> report that fact as it happens? >>> >>> Can we distinguish the two error cases? >>> >>> Oooh... Reading the code again carefully, are you returning false >>> without setting errp if the module just couldn't be found? This is a >>> surprising interface. >>> >>> Yes, I guess then your proposed code is fine (modulo moving >>> warn_report() somewhere else so that it doesn't complain when the image >>> doesn't even contain compressed chunks). >>> >>>>> When a request would access a block that can't be uncompressed because >>>>> of the missing module, that's where we can have a warn_report_once() and >>>>> arguably should fail the I/O request. >>>>> >>>>> Kevin >>>>> >>>> >>>> That would mean, moving the >>>> >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >>>> >>>> to the uncompression code and change it to a warn_report_once() right? >>> >>> Yeah, though I think this doesn't actually work because we never even >>> stored the metadata for chunks of unknown type (see above), so we never >>> reach the uncompression code. >>> >>> What misled me initially is this code in dmg_read_chunk(): >>> >>> case UDBZ: /* bzip2 compressed */ >>> if (!dmg_uncompress_bz2) { >>> break; >>> } >>> >>> I believe this is dead code, it could actually be an assertion. So >>> if I'm not missing anything, adding the warning there would be useless. >>> >>> The other option is moving it into dmg_is_known_block_type() or its >>> caller dmg_read_mish_block(), then we would detect it during open, which >>> is probably nicer anyway. >>> >>> Kevin >>> >>> >> >> Hi Kevin, I got a bit lost on whether we have some agreement on where >> if anywhere to move the check/warning about missing decompression >> submodules. >> >> If that's ok I'd post a V5, and we can rediscuss from the new starting >> point? > > Sure, feel free, though I don't think the code will otherwise change for > dmg, so we could as well continue here. > > My conclusion was that only dmg_read_mish_block() or something called by > it can know whether compressed blocks exist in the image when the > modules aren't present. So if we want to make the warning conditional on > that (and my understanding is correct), this is where a > warn_report_once() would have to live. > > Kevin > I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? Or give me the extra commit needed to move into the right place? Ciao, Claudio
Am 23.09.2022 um 16:46 hat Claudio Fontana geschrieben: > On 9/23/22 16:42, Kevin Wolf wrote: > > Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: > >> On 9/21/22 13:56, Kevin Wolf wrote: > >>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > >>>> On 9/20/22 18:50, Kevin Wolf wrote: > >>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >>>>>> 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? > >>>>> > >>>>> Indeed. The code seems to consider that the modules may not be present. > >>>>> The behaviour in these cases is questionable (it seems to silently leave > >>>>> the buffers as they are and return success) > >>> > >>> I think I misunderstood the code here actually. dmg_read_mish_block() > >>> skips chunks of unknown type, so later trying to find them fails and > >>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for > >>> this. > >>> > >>>>> , but the modules are clearly > >>>>> optional. > >>>>> > >>>>>> 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...? > >>>>> > >>>>> Actually, I think during initialisation, we should just pass NULL as > >>>>> errp and ignore any errors. > >>>> > >>>> Hmm really? I'd think that if there is an actual error loading the > >>>> module (module is installed, but the loading itself fails due to > >>>> broken module, wrong permissions, I/O errors etc) we would want to > >>>> report that fact as it happens? > >>> > >>> Can we distinguish the two error cases? > >>> > >>> Oooh... Reading the code again carefully, are you returning false > >>> without setting errp if the module just couldn't be found? This is a > >>> surprising interface. > >>> > >>> Yes, I guess then your proposed code is fine (modulo moving > >>> warn_report() somewhere else so that it doesn't complain when the image > >>> doesn't even contain compressed chunks). > >>> > >>>>> When a request would access a block that can't be uncompressed because > >>>>> of the missing module, that's where we can have a warn_report_once() and > >>>>> arguably should fail the I/O request. > >>>>> > >>>>> Kevin > >>>>> > >>>> > >>>> That would mean, moving the > >>>> > >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > >>>> > >>>> to the uncompression code and change it to a warn_report_once() right? > >>> > >>> Yeah, though I think this doesn't actually work because we never even > >>> stored the metadata for chunks of unknown type (see above), so we never > >>> reach the uncompression code. > >>> > >>> What misled me initially is this code in dmg_read_chunk(): > >>> > >>> case UDBZ: /* bzip2 compressed */ > >>> if (!dmg_uncompress_bz2) { > >>> break; > >>> } > >>> > >>> I believe this is dead code, it could actually be an assertion. So > >>> if I'm not missing anything, adding the warning there would be useless. > >>> > >>> The other option is moving it into dmg_is_known_block_type() or its > >>> caller dmg_read_mish_block(), then we would detect it during open, which > >>> is probably nicer anyway. > >>> > >>> Kevin > >>> > >>> > >> > >> Hi Kevin, I got a bit lost on whether we have some agreement on where > >> if anywhere to move the check/warning about missing decompression > >> submodules. > >> > >> If that's ok I'd post a V5, and we can rediscuss from the new starting > >> point? > > > > Sure, feel free, though I don't think the code will otherwise change for > > dmg, so we could as well continue here. > > > > My conclusion was that only dmg_read_mish_block() or something called by > > it can know whether compressed blocks exist in the image when the > > modules aren't present. So if we want to make the warning conditional on > > that (and my understanding is correct), this is where a > > warn_report_once() would have to live. > > > > Kevin > > > > I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? > > Or give me the extra commit needed to move into the right place? Only built and tested with an uncompressed image, so this could use testing with an actual compressed dmg image and the module present or missing, but something like the following should do the trick. Kevin diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..630cde3416 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) { s->types[i] = buff_read_uint32(buffer, offset); if (!dmg_is_known_block_type(s->types[i])) { + switch (s->types[i]) { + case UDBZ: + warn_report_once("dmg-bzip2 module is missing, accessing bzip2 " + "compressed blocks will result in I/O errors"); + break; + case ULFO: + warn_report_once("dmg-lzfse module is missing, accessing lzfse " + "compressed blocks will result in I/O errors"); + break; + case UDCM: + case UDLE: + /* Comments and last entry can be ignored without problems */ + break; + default: + warn_report_once("Image contains chunks of unknown type %x, " + "accessing them will result in I/O errors", + s->types[i]); + break; + } chunk_count--; i--; offset += 40;
On 9/23/22 18:29, Kevin Wolf wrote: > Am 23.09.2022 um 16:46 hat Claudio Fontana geschrieben: >> On 9/23/22 16:42, Kevin Wolf wrote: >>> Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: >>>> On 9/21/22 13:56, Kevin Wolf wrote: >>>>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >>>>>> On 9/20/22 18:50, Kevin Wolf wrote: >>>>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>>>>>> 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? >>>>>>> >>>>>>> Indeed. The code seems to consider that the modules may not be present. >>>>>>> The behaviour in these cases is questionable (it seems to silently leave >>>>>>> the buffers as they are and return success) >>>>> >>>>> I think I misunderstood the code here actually. dmg_read_mish_block() >>>>> skips chunks of unknown type, so later trying to find them fails and >>>>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for >>>>> this. >>>>> >>>>>>> , but the modules are clearly >>>>>>> optional. >>>>>>> >>>>>>>> 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...? >>>>>>> >>>>>>> Actually, I think during initialisation, we should just pass NULL as >>>>>>> errp and ignore any errors. >>>>>> >>>>>> Hmm really? I'd think that if there is an actual error loading the >>>>>> module (module is installed, but the loading itself fails due to >>>>>> broken module, wrong permissions, I/O errors etc) we would want to >>>>>> report that fact as it happens? >>>>> >>>>> Can we distinguish the two error cases? >>>>> >>>>> Oooh... Reading the code again carefully, are you returning false >>>>> without setting errp if the module just couldn't be found? This is a >>>>> surprising interface. >>>>> >>>>> Yes, I guess then your proposed code is fine (modulo moving >>>>> warn_report() somewhere else so that it doesn't complain when the image >>>>> doesn't even contain compressed chunks). >>>>> >>>>>>> When a request would access a block that can't be uncompressed because >>>>>>> of the missing module, that's where we can have a warn_report_once() and >>>>>>> arguably should fail the I/O request. >>>>>>> >>>>>>> Kevin >>>>>>> >>>>>> >>>>>> That would mean, moving the >>>>>> >>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >>>>>> >>>>>> to the uncompression code and change it to a warn_report_once() right? >>>>> >>>>> Yeah, though I think this doesn't actually work because we never even >>>>> stored the metadata for chunks of unknown type (see above), so we never >>>>> reach the uncompression code. >>>>> >>>>> What misled me initially is this code in dmg_read_chunk(): >>>>> >>>>> case UDBZ: /* bzip2 compressed */ >>>>> if (!dmg_uncompress_bz2) { >>>>> break; >>>>> } >>>>> >>>>> I believe this is dead code, it could actually be an assertion. So >>>>> if I'm not missing anything, adding the warning there would be useless. >>>>> >>>>> The other option is moving it into dmg_is_known_block_type() or its >>>>> caller dmg_read_mish_block(), then we would detect it during open, which >>>>> is probably nicer anyway. >>>>> >>>>> Kevin >>>>> >>>>> >>>> >>>> Hi Kevin, I got a bit lost on whether we have some agreement on where >>>> if anywhere to move the check/warning about missing decompression >>>> submodules. >>>> >>>> If that's ok I'd post a V5, and we can rediscuss from the new starting >>>> point? >>> >>> Sure, feel free, though I don't think the code will otherwise change for >>> dmg, so we could as well continue here. >>> >>> My conclusion was that only dmg_read_mish_block() or something called by >>> it can know whether compressed blocks exist in the image when the >>> modules aren't present. So if we want to make the warning conditional on >>> that (and my understanding is correct), this is where a >>> warn_report_once() would have to live. >>> >>> Kevin >>> >> >> I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? >> >> Or give me the extra commit needed to move into the right place? > > Only built and tested with an uncompressed image, so this could use > testing with an actual compressed dmg image and the module present or > missing, but something like the following should do the trick. > > Kevin > > > diff --git a/block/dmg.c b/block/dmg.c > index 98db18d82a..630cde3416 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, > for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) { > s->types[i] = buff_read_uint32(buffer, offset); > if (!dmg_is_known_block_type(s->types[i])) { > + switch (s->types[i]) { > + case UDBZ: > + warn_report_once("dmg-bzip2 module is missing, accessing bzip2 " > + "compressed blocks will result in I/O errors"); > + break; > + case ULFO: > + warn_report_once("dmg-lzfse module is missing, accessing lzfse " > + "compressed blocks will result in I/O errors"); > + break; > + case UDCM: > + case UDLE: > + /* Comments and last entry can be ignored without problems */ > + break; > + default: > + warn_report_once("Image contains chunks of unknown type %x, " > + "accessing them will result in I/O errors", > + s->types[i]); > + break; > + } > chunk_count--; > i--; > offset += 40; > Awesome thanks! Claudio
diff --git a/audio/audio.c b/audio/audio.c index 76b8735b44..4f4bb10cce 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -72,6 +72,7 @@ void audio_driver_register(audio_driver *drv) audio_driver *audio_driver_lookup(const char *name) { struct audio_driver *d; + Error *local_err = NULL; QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { @@ -79,7 +80,10 @@ audio_driver *audio_driver_lookup(const char *name) } } - audio_module_load_one(name); + if (!audio_module_load_one(name, &local_err) && local_err) { + error_report_err(local_err); + } + QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { return d; diff --git a/block.c b/block.c index bc85f46eed..85c3742d7a 100644 --- a/block.c +++ b/block.c @@ -464,7 +464,11 @@ BlockDriver *bdrv_find_format(const char *format_name) /* The driver isn't registered, maybe we need to load a module */ for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) { if (!strcmp(block_driver_modules[i].format_name, format_name)) { - block_module_load_one(block_driver_modules[i].library_name); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } @@ -976,7 +980,11 @@ BlockDriver *bdrv_find_protocol(const char *filename, for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) { if (block_driver_modules[i].protocol_name && !strcmp(block_driver_modules[i].protocol_name, protocol)) { - block_module_load_one(block_driver_modules[i].library_name); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..349b05d20b 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -434,6 +434,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, uint64_t plist_xml_offset, plist_xml_length; int64_t offset; int ret; + Error *local_err = NULL; ret = bdrv_apply_auto_read_only(bs, NULL, errp); if (ret < 0) { @@ -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; diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 0806d8fcaa..5902c59c94 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -148,7 +148,15 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp) DeviceState *qdev_new(const char *name) { if (!object_class_by_name(name)) { - module_load_qom_one(name); + Error *local_err = NULL; + if (!module_load_qom_one(name, &local_err)) { + if (local_err) { + error_report_err(local_err); + } else { + error_report("could not find a module for type '%s'", name); + } + abort(); + } } return DEVICE(object_new(name)); } diff --git a/include/qemu/module.h b/include/qemu/module.h index 8c012bbe03..e1fd4a6d7b 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -61,16 +61,33 @@ typedef enum { #define fuzz_target_init(function) module_init(function, \ MODULE_INIT_FUZZ_TARGET) #define migration_init(function) module_init(function, MODULE_INIT_MIGRATION) -#define block_module_load_one(lib) module_load_one("block-", lib) -#define ui_module_load_one(lib) module_load_one("ui-", lib) -#define audio_module_load_one(lib) module_load_one("audio-", lib) +#define block_module_load_one(lib, errp) module_load_one("block-", lib, errp) +#define ui_module_load_one(lib, errp) module_load_one("ui-", lib, errp) +#define audio_module_load_one(lib, errp) module_load_one("audio-", lib, errp) void register_module_init(void (*fn)(void), module_init_type type); void register_dso_module_init(void (*fn)(void), module_init_type type); void module_call_init(module_init_type type); -bool module_load_one(const char *prefix, const char *lib_name); -void module_load_qom_one(const char *type); + +/* + * module_load_one: attempt to load a module from a set of directories + * + * directories searched are: + * - getenv("QEMU_MODULE_DIR") + * - get_relocated_path(CONFIG_QEMU_MODDIR); + * - /var/run/qemu/${version_dir} + * + * prefix: a subsystem prefix, or the empty string ("audio-", ..., "") + * name: name of the module + * errp: error to set in case the module is found, but load failed. + * + * Return value: true on success (found and loaded); + * if module if found, but load failed, errp will be set. + * if module is not found, errp will not be set. + */ +bool module_load_one(const char *prefix, const char *name, Error **errp); +bool module_load_qom_one(const char *type, Error **errp); void module_load_qom_all(void); void module_allow_arch(const char *arch); diff --git a/qom/object.c b/qom/object.c index d34608558e..6a74e6a478 100644 --- a/qom/object.c +++ b/qom/object.c @@ -526,8 +526,14 @@ void object_initialize(void *data, size_t size, const char *typename) #ifdef CONFIG_MODULES if (!type) { - module_load_qom_one(typename); - type = type_get_by_name(typename); + Error *local_err = NULL; + if (!module_load_qom_one(typename, &local_err)) { + if (local_err) { + error_report_err(local_err); + } + } else { + type = type_get_by_name(typename); + } } #endif if (!type) { @@ -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); + } oc = object_class_by_name(typename); } #endif diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 76eb7bac56..bb83c7aae9 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -753,12 +753,16 @@ static void qtest_process_command(CharBackend *chr, gchar **words) qtest_sendf(chr, "OK %"PRIi64"\n", (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); } else if (strcmp(words[0], "module_load") == 0) { + Error *local_err = NULL; g_assert(words[1] && words[2]); qtest_send_prefix(chr); - if (module_load_one(words[1], words[2])) { + if (module_load_one(words[1], words[2], &local_err)) { qtest_sendf(chr, "OK\n"); } else { + if (local_err) { + error_report_err(local_err); + } qtest_sendf(chr, "FAIL\n"); } } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) { diff --git a/ui/console.c b/ui/console.c index 765892f84f..34dd206167 100644 --- a/ui/console.c +++ b/ui/console.c @@ -2632,7 +2632,11 @@ bool qemu_display_find_default(DisplayOptions *opts) for (i = 0; i < (int)ARRAY_SIZE(prio); i++) { if (dpys[prio[i]] == NULL) { - ui_module_load_one(DisplayType_str(prio[i])); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(prio[i]), &local_err) + && local_err) { + error_report_err(local_err); + } } if (dpys[prio[i]] == NULL) { continue; @@ -2650,7 +2654,11 @@ void qemu_display_early_init(DisplayOptions *opts) return; } if (dpys[opts->type] == NULL) { - ui_module_load_one(DisplayType_str(opts->type)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(opts->type), &local_err) + && local_err) { + error_report_err(local_err); + } } if (dpys[opts->type] == NULL) { error_report("Display '%s' is not available.", @@ -2680,7 +2688,11 @@ void qemu_display_help(void) printf("none\n"); for (idx = DISPLAY_TYPE_NONE; idx < DISPLAY_TYPE__MAX; idx++) { if (!dpys[idx]) { - ui_module_load_one(DisplayType_str(idx)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(idx), &local_err) + && local_err) { + error_report_err(local_err); + } } if (dpys[idx]) { printf("%s\n", DisplayType_str(dpys[idx]->type)); diff --git a/util/module.c b/util/module.c index 8563edd626..14e18e59c6 100644 --- a/util/module.c +++ b/util/module.c @@ -21,6 +21,7 @@ #include "qemu/module.h" #include "qemu/cutils.h" #include "qemu/config-file.h" +#include "qapi/error.h" #ifdef CONFIG_MODULE_UPGRADES #include "qemu-version.h" #endif @@ -144,25 +145,22 @@ static bool module_check_arch(const QemuModinfo *modinfo) return true; } -static int module_load_file(const char *fname, bool export_symbols) +/* + * module_load_dso: attempt to load an existing dso file + * + * fname: full pathname of the file to load + * export_symbols: if true, add the symbols to the global name space + * errp: error to set. + * + * Return value: true on success, false on error, and errp will be set. + */ +static bool module_load_dso(const char *fname, bool export_symbols, + Error **errp) { GModule *g_module; void (*sym)(void); - const char *dsosuf = CONFIG_HOST_DSOSUF; - int len = strlen(fname); - int suf_len = strlen(dsosuf); ModuleEntry *e, *next; - int ret, flags; - - if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) { - /* wrong suffix */ - ret = -EINVAL; - goto out; - } - if (access(fname, F_OK)) { - ret = -ENOENT; - goto out; - } + int flags; assert(QTAILQ_EMPTY(&dso_init_list)); @@ -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"); } g_module_close(g_module); - ret = -EINVAL; - } else { - QTAILQ_FOREACH(e, &dso_init_list, node) { - e->init(); - register_module_init(e->init, e->type); - } - ret = 0; + return false; } + QTAILQ_FOREACH(e, &dso_init_list, node) { + e->init(); + register_module_init(e->init, e->type); + } trace_module_load_module(fname); QTAILQ_FOREACH_SAFE(e, &dso_init_list, node, next) { QTAILQ_REMOVE(&dso_init_list, e, node); g_free(e); } -out: - return ret; + return true; } -#endif -bool module_load_one(const char *prefix, const char *lib_name) +bool module_load_one(const char *prefix, const char *name, Error **errp) { bool success = false; - -#ifdef CONFIG_MODULES - char *fname = NULL; #ifdef CONFIG_MODULE_UPGRADES char *version_dir; #endif @@ -219,14 +209,13 @@ bool module_load_one(const char *prefix, const char *lib_name) char *dirs[5]; char *module_name; int i = 0, n_dirs = 0; - int ret; bool export_symbols = false; static GHashTable *loaded_modules; const QemuModinfo *modinfo; const char **sl; if (!g_module_supported()) { - fprintf(stderr, "Module is not supported by system.\n"); + error_setg(errp, "%s", "this platform does not support GLib modules"); return false; } @@ -234,7 +223,7 @@ bool module_load_one(const char *prefix, const char *lib_name) loaded_modules = g_hash_table_new(g_str_hash, g_str_equal); } - module_name = g_strdup_printf("%s%s", prefix, lib_name); + module_name = g_strdup_printf("%s%s", prefix, name); if (g_hash_table_contains(loaded_modules, module_name)) { g_free(module_name); @@ -246,6 +235,8 @@ bool module_load_one(const char *prefix, const char *lib_name) if (modinfo->arch) { if (strcmp(modinfo->name, module_name) == 0) { if (!module_check_arch(modinfo)) { + error_setg(errp, "module arch does not match: " + "expected '%s', got '%s'", module_arch, modinfo->arch); return false; } } @@ -254,7 +245,9 @@ bool module_load_one(const char *prefix, const char *lib_name) if (strcmp(modinfo->name, module_name) == 0) { /* we depend on other module(s) */ for (sl = modinfo->deps; *sl != NULL; sl++) { - module_load_one("", *sl); + if (!(module_load_one("", *sl, errp))) { + return false; + } } } else { for (sl = modinfo->deps; *sl != NULL; sl++) { @@ -283,16 +276,26 @@ bool module_load_one(const char *prefix, const char *lib_name) assert(n_dirs <= ARRAY_SIZE(dirs)); for (i = 0; i < n_dirs; i++) { - fname = g_strdup_printf("%s/%s%s", - dirs[i], module_name, CONFIG_HOST_DSOSUF); - ret = module_load_file(fname, export_symbols); - g_free(fname); - fname = NULL; - /* Try loading until loaded a module file */ - if (!ret) { - success = true; + char *fname = g_strdup_printf("%s/%s%s", + dirs[i], module_name, CONFIG_HOST_DSOSUF); + int ret = access(fname, F_OK); + if (ret != 0) { + if (errno == ENOENT || errno == ENOTDIR) { + /* + * if we just don't find the module in this dir, try the next one. + * If we don't find it in any dir, that can be fine too, user or + * distro did not install the module. We will return false in this + * case, with no error set. + */ + continue; + } + /* most common is EACCES here */ + error_setg_errno(errp, errno, "error trying to access %s", fname); break; } + success = module_load_dso(fname, export_symbols, errp); + g_free(fname); + break; } if (!success) { @@ -304,21 +307,30 @@ bool module_load_one(const char *prefix, const char *lib_name) g_free(dirs[i]); } -#endif return success; } -#ifdef CONFIG_MODULES - static bool module_loaded_qom_all; -void module_load_qom_one(const char *type) +/* + * module_load_qom_one: attempt to load a module to provide a QOM type + * + * type: the type to be provided + * errp: error to set. + * + * Return value: true on success (found and loaded), false on failure. + * If a module is simply not found for the type, + * errp will not be set. + */ +bool module_load_qom_one(const char *type, Error **errp) { + bool found = false; const QemuModinfo *modinfo; const char **sl; if (!type) { - return; + error_setg(errp, "%s", "type is NULL"); + return false; } trace_module_lookup_object_type(type); @@ -331,15 +343,26 @@ void module_load_qom_one(const char *type) } for (sl = modinfo->objs; *sl != NULL; sl++) { if (strcmp(type, *sl) == 0) { - module_load_one("", modinfo->name); + if (found) { + error_setg(errp, "multiple modules providing '%s'", type); + found = false; + break; + } + found = module_load_one("", modinfo->name, errp); + if (!found) { + /* errp optionally set in module_load_one */ + break; + } } } } + return found; } void module_load_qom_all(void) { const QemuModinfo *modinfo; + Error *local_err = NULL; if (module_loaded_qom_all) { return; @@ -352,7 +375,9 @@ void module_load_qom_all(void) if (!module_check_arch(modinfo)) { continue; } - module_load_one("", modinfo->name); + if (!module_load_one("", modinfo->name, &local_err) && local_err) { + error_report_err(local_err); + } } module_loaded_qom_all = true; } @@ -368,7 +393,11 @@ void qemu_load_module_for_opts(const char *group) } for (sl = modinfo->opts; *sl != NULL; sl++) { if (strcmp(group, *sl) == 0) { - module_load_one("", modinfo->name); + Error *local_err = NULL; + if (!module_load_one("", modinfo->name, &local_err) + && local_err) { + error_report_err(local_err); + } } } } @@ -378,7 +407,8 @@ void qemu_load_module_for_opts(const char *group) void module_allow_arch(const char *arch) {} void qemu_load_module_for_opts(const char *group) {} -void module_load_qom_one(const char *type) {} +bool module_load_one(const char *prefix, const char *name, Error **errp) { return true; } +bool module_load_qom_one(const char *type, Error **errp) { return true; } void module_load_qom_all(void) {} #endif
improve error handling during module load, by changing: bool module_load_one(const char *prefix, const char *lib_name); void module_load_qom_one(const char *type); to: bool module_load_one(const char *prefix, const char *name, Error **errp); bool module_load_qom_one(const char *type, Error **errp); module_load_qom_one has been introduced in: commit 28457744c345 ("module: qom module support"), which built on top of module_load_one, but discarded the bool return value. Restore it. Adapt all callers to emit errors, or ignore them, or fail hard, as appropriate in each context. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- audio/audio.c | 6 +- block.c | 12 +++- block/dmg.c | 10 ++- hw/core/qdev.c | 10 ++- include/qemu/module.h | 27 ++++++-- qom/object.c | 15 ++++- softmmu/qtest.c | 6 +- ui/console.c | 18 ++++- util/module.c | 150 +++++++++++++++++++++++++----------------- 9 files changed, 176 insertions(+), 78 deletions(-)