[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 2/4] smbus: Fix spd_data_generate() error API violation
Date: Wed, 22 Apr 2020 15:43:38 +0200 (CEST)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

On Tue, 21 Apr 2020, Markus Armbruster wrote:
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

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

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.

I though it could be useful to warn the users when QEMU had to fix up some values to notify them that what they get may not be what they expect and can then know why. If the message really annoys you you can remove it but I think it can be useful. I just think your real problem with it is that Error can't support it so it's easier to remove the warning than fixing Error or use warn_report instead.

                          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.

The memory size that triggers the warning is specified by the user so the user can do someting about it.

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

I agree with that and also hate Windows's habit of trying to be more intelligent than the user and prefer the Unix way however the average users of QEMU (from my perpective, who wants to run Amiga like OSes typically on Windows and for the most part not knowing what they are doing) are already intimidated by the messy QEMU command line interface and will specify random values and complain or go away if it does not work so making their life a little easier is not useless. These users (or any CLI users) are apparently not relevant from your point of view but they do exist and I think should be supported better.

                                                         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.

What was broken in this patch? Isn't freeing the previous warning when adding a new one or combining them as you say below (although I don't really get what you mean) would fix it without removing fix ups? It's only unused now because in previous patches you've removed all usages: first broke memory fixup because of some internal QEMU API did not support fixing up memory size so instead of fixing that API removed what did not fit, then now removing type fix ups because it's not fitting Error API.

Originally it did work well and produced usable values for whatever number the user specified and it was convenient for users. (Especially the sam460ex is a problematic case because of SoC and firmware limits that the user should not need to know about.)

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.

I've tried to use error so the board code can decide what's an error and what's a warning but if this cannot be done with this QEMU facility I don't know a better way than using warn_report for warnings.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]