Re: [PATCH 3/4] hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()

[Markus Armbruster]

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 :)