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.