Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]>
[...]
>>> 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.
