Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 7/21/20 10:13 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@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é <f4bug@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?