On 7/21/20 10:13 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4...@amsat.org> writes:
>
>> On 7/20/20 6:07 PM, Cédric Le Goater wrote:
>>> On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote:
>>>> The SDRAM Memory Controller has a 32-bit address bus, thus
>>>> supports up to 4 GiB of DRAM. There is a signed to unsigned
>>>> conversion error with the AST2600 maximum memory size:
>>>>
>>>> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648)
>>>> = 0xffffffff40000000
>>>> = 16 EiB - 2 GiB
>>>>
>>>> Fix by using the IEC suffixes which are usually safer, and add
>>>> a check to verify the memory is valid. This would have catched
>
> caught
>
>>>> this bug:
>>>>
>>>> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261:
>>>> qemu-system-arm: Invalid RAM size 16 EiB
>>>
>>> Indeed :/
>>>
>>>>
>>>> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support")
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>>> ---
>>>> hw/misc/aspeed_sdmc.c | 12 +++++++++---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>>>> index 0737d8de81..76dd7e6a20 100644
>>>> --- a/hw/misc/aspeed_sdmc.c
>>>> +++ b/hw/misc/aspeed_sdmc.c
>>>> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev,
>>>> Error **errp)
>>>> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>>>>
>>>> s->max_ram_size = asc->max_ram_size;
>>>> + if (s->max_ram_size >= 4 * GiB) {
>>>> + char *szstr = size_to_str(s->max_ram_size);
>>>> + error_setg(errp, "Invalid RAM size %s", szstr);
>>>> + g_free(szstr);
>>>> + return;
>>>> + }
>>>>
>>>
>>> I would put an assert() since the max RAM size is not user configurable.
>>
>> As you wish, at this point I'm completely lost with error reporting.
>
> :-/
>
>> Per the manual
>> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html):
>>
>> "Many, many devices neglect to clean up properly on error, and get away
>> with it only because all callers treat errors as fatal.
>>
>> If you decide to take cleanup shortcuts, say because the cleanup is
>> untestable, consider adding a comment at least."
>>
>> So I'll go for address + comment:
>>
>> assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */
>
> Makes sense.
>
> Note this is *not* a cleanup shortcut, at least not the kind I had in
> mind.
>
> What I had in mind is unclean failure, i.e. returning on error without
> proper cleanup: revert changes made so far, free resources. This is
> *wrong*. But the wrongness doesn't matter when all callers treat errors
> as fatal.
>
> Checking an impossible condition with assert() is better than treating
> it as an error and bungling its handling. If you treat it as an error,
> do it properly. Since I'm quite skeptical about the chances of pulling
> off "properly" for untestable things, I prefer assertions.
>
> There's another reason. User errors need to be handled gracefully.
> Programming errors should (in my opinion) trigger abort(), so they get
> fixed.
>
> When the spot that detects the error can't know which kind it is, you
> have to fail cleanly and let the caller decide how to handle the error.
>
> Example: object_property_find() errors out when the property doesn't
> exist. This may be a programming error, e.g. a well-known property
> isn't found, because a programmer mistyped the property name. Or it may
> be a user error, e.g. a user mistyped the property name argument of
> qom-get.
>
> When functions have multiple failure modes, and only some of them are
> programming errors, the caller typically can't tell them apart, and errs
> on the side of user error. Programming errors then get reported as
> (typically confusing!) user errors.
A big part of your reply is worth adding in a "How to correctly use the
Error* propagation API for dummies" in docs/devel document.
>
> The #1 reason for such awkward functions is lazy thinking + eager
> typing: by treating anything that can go wrong as an error for the
> caller to handle, I can replace thinking about what may go wrong and
> what must not go wrong by typing up a bunch of error paths. Great time
> saver as long as I stick to the time-honored strategy of not bothering
> to test my error paths.
Not all are easily testable :( Or do you have a recomendation? Like
forcing an error in the code while developing, so the path is checked?
