Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>> first call sets an error, it no longer points to NULL for the second
>>>>> call.
>>>>>
>>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>>> passes anything that needs adjusting.  Until the previous commit,
>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>
>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>
>>>>> Â Â  If everything's fine, return non-null and don't set an error.
>>>>>
>>>>> Â Â  Else, if memory size or type need adjusting, return non-null and
>>>>> Â Â  set an error describing the adjustment.
>>>>>
>>>>> Â Â  Else, return null and set an error reporting why no data can be
>>>>> Â Â  generated.
>>>>>
>>>>> Its callers treat the error as a warning even when null is returned.
>>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>>>
>>>>> Since the previous commit, only "everything's fine" can actually
>>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>>> of the error API violation.
>>>>
>>>> This leaves board code no chance to recover from values given by
>>>> user that won't fit without duplicating checks that this function
>>>> does. Also this will abort without giving meaningful errors if an
>>>> invalid value does get through and result in a crash which is not
>>>> used friendly. So I don't like this but if others think this is
>>>> acceptable maybe at least unit test should be adjusted to make
>>>> sure aborts cannot be triggered by user for values that are not
>>>> usually tested during development.
>>>
>>> Agreed. Do you have an example (or more) to better show Markus this
>>> code use? So we can add tests.
>>
>> After Markus's patches probably nothing uses it any more but this
>> comes with the result that previously giving some random value such
>> as -m 100 did produce a working sam460ex machine after some warnings
>> but now it just thows back some errors to the user which may or may
>> not be helpful to them.
>>
>>> Personally I'd use a script to generate a dumb static array of all
>>> possible sizes...
>>
>> Maybe testing with the biggest valid value such as -m 2048 (that's
>> commonly used probably) and an invalid value such as -m 100 might be
>> enough. Testing all possible values might take too long and would
>> not test what happens with invalid values. Ideally those invalud
>> values should also work like before a0258e4afa but should at least
>> give a meaningful warning so the user can fix the command line
>> without too much head scratching. Actually that commit was from Igor
>> not from Marcus so sorry for attributing that to Marcus too, I
>> remembered wrong.
>>
>> By the way you could argue that on real machine you cannot plug
>> certain combinations of memory modules so it's enough to model that
>> but I think QEMU does not have to be that strict and also support
>> configs that cannot happen on real hadware but would work. This
>> might be useful for example if you have some ammount of memory to
>> set aside for a VM on a host but that's not a size that exists in
>> memory modules on real hardware. This also works on pc machine in
>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>> create a machine with such unrealistic size. The sam460ex did the
>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>> to 96 MB which is now not possible due to change in QEMU internal
>> APIs. This probably isn't important enough to worth the extra effort
>> to support but would have been nice to preserve.
>
> Besides the above here's another use case of the fix ups that I wanted
> to keep:
>
> cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/">https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>
> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
> so works with whatever amount of RAM (also Linux booted with -kernel
> probably does not care) so any -memory value is valid. However some
> may want to also use original firmware ROM for compatibility which
> detects RAM reading SPD eeproms (the i2c emulation needed for that is
> not working yet but once that's fixed this will be the case). I want
> to add smbus_eeproms for this but do not want to just abort for cases
> where -memory given by user cannot be covered with SPD data. Instead a
> warning and covering as much RAM as possible should be enough (the ROM
> will detect less RAM than given with -m 
> but that's OK and better than just bailing out without a message
> tripping an assert). But I don't want to replicate in board code the
> calculation and checks the spd_data_generate() function does anyway
> (that would just puzzle reviewers for every use of this functions).
>
> Previously this was possible with my original spd_data_generate()
> implementation. What's your suggestion to bring that functionality
> back without breaking Error API? Maybe adding new parameters to tell
> the spd_data_generate() which fixups are allowed?

Quick reply without having thought through the issues at all: I'm not
opposed to you doing work to enable additional or even arbitrary memory
sizes where these actually work.  I'm first and foremost opposed to me
wasting time on "improving" code that is not used for anything.  That's
why I dumbed down spd_data_generate().

Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
QEMU do exactly what it's told, and fail with a clear error message when
that is not possible.  The error message may include hints for the user
on how to correct the configuration.