[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and mo
From: |
Claudio Fontana |
Subject: |
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one |
Date: |
Fri, 23 Sep 2022 16:46:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 |
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
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, (continued)
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/20
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/21
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/21
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/21
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/22
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/22
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/21
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/21
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/23
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/23
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one,
Claudio Fontana <=
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Kevin Wolf, 2022/09/23
- Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/23