Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) &&
>>> local_err) {
>>> +error_report_err(local_err);
>>> +}
>>> +local_err = NULL;
>>> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) &&
> > local_err) {
> > +error_report_err(local_err);
> > +}
> > +local_err = NULL;
> > +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
> +error_report_err(local_err);
> +}
> +local_err = NULL;
> +if (!block_module_load_one("dmg-lzfse", _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", _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()
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
> >>> +error_report_err(local_err);
> >>> +}
> >>> +local_err = NULL;
> >>> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
>>> +error_report_err(local_err);
>>> +}
>>> +local_err = NULL;
>>> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Kevin Wolf writes: > Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: >> Kevin Wolf 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.
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: > Kevin Wolf 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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
> > +error_report_err(local_err);
> > +}
> > +local_err = NULL;
> > +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Kevin Wolf writes:
> Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben:
>> Kevin Wolf 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", _err) && local_err) {
>> >> >>> +error_report_err(local_err);
>> >> >>> +}
>> >> >>> +local_err = NULL;
>> >> >>> +if (!block_module_load_one("dmg-lzfse", _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", _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.
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben:
> Kevin Wolf 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", _err) && local_err) {
> >> >>> +error_report_err(local_err);
> >> >>> +}
> >> >>> +local_err = NULL;
> >> >>> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
> +error_report_err(local_err);
> +}
> +local_err = NULL;
> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Kevin Wolf 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", _err) && local_err) {
>> >>> +error_report_err(local_err);
>> >>> +}
>> >>> +local_err = NULL;
>> >>> +if (!block_module_load_one("dmg-lzfse", _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", _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?
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
> >>> +error_report_err(local_err);
> >>> +}
> >>> +local_err = NULL;
> >>> +if (!block_module_load_one("dmg-lzfse", _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", _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
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
>>> +error_report_err(local_err);
>>> +}
>>> +local_err = NULL;
>>> +if (!block_module_load_one("dmg-lzfse", _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", _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, _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 *))) {
>>> -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
>>> *))) {
>>> -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~
>
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
>> +error_report_err(local_err);
>> +}
>> +local_err = NULL;
>> +if (!block_module_load_one("dmg-lzfse", _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, _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 *))) {
>> -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
>> *))) {
>> -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~
Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one
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", _err) && local_err) {
+error_report_err(local_err);
+}
+local_err = NULL;
+if (!block_module_load_one("dmg-lzfse", _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, _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 *))) {
-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 *)))
{
-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~
