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

[Peter Maydell]

On Wed, 20 Mar 2024 at 19:05, Markus Armbruster <armbru@redhat.com> wrote:
>
> 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.

Fair enough. (When I made the comment I was vaguely wondering
if we wanted to keep the return value available to distinguish
"this hook has handled the NMI, don't keep iterating" from
"no error, but you should keep iterating through other handlers".
But I think in the end my feeling is we should always stop
after the first NMI handler we find regardless.

-- PMM