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