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

[Markus Armbruster]

BALATON Zoltan <address@hidden> writes:

> On Mon, 20 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.
>
> The idea here again is to make it work if there's a way it could work
> rather than throw back an error to user and bail. This is for user
> convenience in the likely case the user knows nothing about the board
> or SPD data restrictions.

We're in perfect agreement that the user of QEMU should not need to know
anything about memory type and number of banks.  QEMU should pick a
sensible configuration for any memory size it supports.

>                           You seem to disagree with this

Here's what I actually disagree with:

1. QEMU warning the user about its choice of memory type, but only
sometimes.  Why warn?  There is nothing wrong, and there is nothing the
user can do to avoid the condition that triggers the warning.

2. QEMU changing the user's memory size.  Yes, I understand that's an
attempt to be helpful, but I prefer my tools not to second-guess my
intent.

>                                                          and want to
> remove all such logic from everywhere. Despite the title of this patch
> it's not just fixing error API usage but removing those fix ups.

I'm removing unused code that is also broken.  If you want to keep it,
please fix it, and provide a user and/or a unit test.  Without that, it
is a trap for future callers.

> If Error cannot be used for this could you change error_setg to
> warn_report and keep the fix ups untouched? That fixes the problem
> with error (although leaves no chance to board code to handle
> errors). Maybe fix Error to be usable for what it's intended for
> rather than removing cases it can't handle.

Error is designed for errors, not warnings.

A function either succeeds (no error) or fails (one error).

It can be pressed into service for warnings (you did, although in a
buggy way).  You still can return at most one Error per Error **
parameter.  If you need multiple warnings, use multiple parameters
(ugh!).  You could also try to squash multiple warnings into one the
hints mechanism.

I'd advise against combining warn_report() and Error ** in one function.
A function should either take care of talking to the user completely, or
leave it to its caller completely.