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

[Philippe Mathieu-Daudé]

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