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
