[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] hw/nmi: Remove @cpu_index argument from NMIClass::nmi_ha
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 3/4] hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler() |
Date: |
Wed, 20 Mar 2024 20:05:06 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 20/3/24 14:23, Peter Maydell wrote:
>> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org>
>> wrote:
>>>
>>> Only s390x was using the 'cpu_index' argument, but since the
>>> previous commit it isn't anymore (it use the first cpu).
>>> Since this argument is now completely unused, remove it. Have
>>> the callback return a boolean indicating failure.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/nmi.h | 11 ++++++++++-
>>> hw/core/nmi.c | 3 +--
>>> hw/hppa/machine.c | 8 +++++---
>>> hw/i386/x86.c | 7 ++++---
>>> hw/intc/m68k_irqc.c | 6 ++++--
>>> hw/m68k/q800-glue.c | 6 ++++--
>>> hw/misc/macio/gpio.c | 6 ++++--
>>> hw/ppc/pnv.c | 6 ++++--
>>> hw/ppc/spapr.c | 6 ++++--
>>> hw/s390x/s390-virtio-ccw.c | 6 ++++--
>>> 10 files changed, 44 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>>> index fff41bebc6..c70db941c9 100644
>>> --- a/include/hw/nmi.h
>>> +++ b/include/hw/nmi.h
>>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
>>> struct NMIClass {
>>> InterfaceClass parent_class;
>>>
>>> - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>> + /**
>>> + * nmi_handler: Callback to handle NMI notifications.
>>> + *
>>> + * @n: Class #NMIState state
>>> + * @errp: pointer to error object
>>> + *
>>> + * On success, return %true.
>>> + * On failure, store an error through @errp and return %false.
>>> + */
>>> + bool (*nmi_handler)(NMIState *n, Error **errp);
>> Any particular reason to change the method name here?
>> Do we really need to indicate failure both through the bool return
>> and the Error** ?
>
> No, but this is the style *recommended* by the Error API since
> commit e3fe3988d7 ("error: Document Error API usage rules"):
>
> error: Document Error API usage rules
>
> This merely codifies existing practice, with one exception: the rule
> advising against returning void, where existing practice is mixed.
>
> When the Error API was created, we adopted the (unwritten) rule to
> return void when the function returns no useful value on success,
> unlike GError, which recommends to return true on success and false
> on error then.
>
> [...]
>
> Make the rule advising against returning void official by putting it
> in writing. This will hopefully reduce confusion.
>
> * - Whenever practical, also return a value that indicates success /
> * failure. This can make the error checking more concise, and can
> * avoid useless error object creation and destruction. Note that
It's the difference between
if (!frobnicate(arg, errp)) {
return;
}
and
frobnicate(arg, &err);
if (err) {
error_propagate(errp, err);
return;
}
Readabilty dies by a thousand cuts.
GError got this right. We deviated from it for Error, until we
understood why it's right.
Another win: &error_abort gives you a backtrace into frobnicate() with
the former, and into error_propagate() with the latter.
> * we still have many functions returning void. We recommend
> * • bool-valued functions return true on success / false on failure,
> * • pointer-valued functions return non-null / null pointer, and
> * • integer-valued functions return non-negative / negative.
>
> Anyway I'll respin removing @cpu_index as a single change :)