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

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> On 20/3/24 14:34, Peter Maydell wrote:
>> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daud=C3=A9 <philmd@linaro.=
> org> wrote:
>>>
>>> nmi_monitor_handle() is not related to the monitor,
>>> rename it as nmi_trigger(). Return boolean value
>>> indicating success / failure. The 'cpu_index' argument
>>> is not used, remove it.
>>>
>>> Signed-off-by: Philippe Mathieu-Daud=C3=A9 <philmd@linaro.org>

[...]

>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>>> index c70db941c9..32b27067f2 100644
>>> --- a/include/hw/nmi.h
>>> +++ b/include/hw/nmi.h
>>> @@ -49,6 +49,17 @@ struct NMIClass {
>>>       bool (*nmi_handler)(NMIState *n, Error **errp);
>>>   };
>>>
>>> -void nmi_monitor_handle(int cpu_index, Error **errp);
>>> +/**
>>> + * nmi_trigger: Trigger a NMI.
>>> + *
>>> + * @errp: pointer to error object
>>> + *
>>> + * Iterate over all objects implementing the TYPE_NMI interface
>>> + * and deliver NMI to them.

[...]

>>> + *
>>> + * On success, return %true.
>>> + * On failure, store an error through @errp and return %false.
>>> + */
>>> +bool nmi_trigger(Error **errp);
>>
>> Why return a bool here? None of the callsites looks at the
>> return value.
>
> Indeed, but again this is the style *recommended* by the Error API
> since commit e3fe3988d7 ("error: Document Error API usage rules").

Correct:

 * - 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.
>
> Callers providing a non-NULL errp should check the return value,
> but the QMP handlers generated by gen_command_decl() like
> qmp_inject_nmi() here don't return anything.

qmp_inject_nmi() passes the error to its caller.  That's fine.

ipmi_do_hw_op() and watchdog_perform_action() ignore the error.  Also
fine.

> Markus, what would be your preference?

I prefer sticking to the rule whenever practical.

Example for a defensible exception: a callback that's implemented many
more times than it's called.  There the effort of returning something
can exceed the benefit of making the calls more concise.

In a case like nmi_trigger(), what could possibly be gained by deviating
from the rule is dwarved many times over by having to consider the
tradeoffs and justifying them in review.  And if a caller shows up that
could use a return value, we'll waste some more time.  Not worth it.