Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Claudio Fontana <cfontana@suse.de>]

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