Re: [PATCH v8 16/19] hvf: arm: Implement PSCI handling

From: Alexander Graf <agraf@csgraf.de>
To: Richard Henderson <richard.henderson@linaro.org>,
	Peter Maydell <peter.maydell@linaro.org>
Cc: "Eduardo Habkost" <ehabkost@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Cameron Esfahani" <dirty@apple.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	qemu-arm <qemu-arm@nongnu.org>, "Frank Yang" <lfy@google.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Peter Collingbourne" <pcc@google.com>
Subject: Re: [PATCH v8 16/19] hvf: arm: Implement PSCI handling
Date: Mon, 13 Sep 2021 12:06:20 +0200	[thread overview]
Message-ID: <6fe284da-6989-f837-adda-1aa722f0d345@csgraf.de> (raw)
In-Reply-To: <8e0879b6-23c0-8f14-fd96-4d72f1d640c1@linaro.org>

On 12.09.21 23:40, Richard Henderson wrote:
> On 9/12/21 2:37 PM, Alexander Graf wrote:
>>
>> On 12.09.21 23:20, Richard Henderson wrote:
>>> On 9/12/21 1:36 PM, Alexander Graf wrote:
>>>>> I think the callsites would be clearer if you made the function
>>>>> return true for "PSCI call handled", false for "not recognised,
>>>>> give the guest an UNDEF". Code like
>>>>>            if (hvf_handle_psci_call(cpu)) {
>>>>>                stuff;
>>>>>            }
>>>>>
>>>>> looks like the 'stuff' is for the "psci call handled" case,
>>>>> which at the moment it isn't.
>>>>
>>>>
>>>> This function merely follows standard C semantics along the lines
>>>> of "0
>>>> means success, !0 is error". Isn't that what you would usually expect?
>>>
>>> No, not really.  I expect stuff that returns error codes to return
>>> negative integers on failure.  I expect stuff that returns a boolean
>>> success/failure to return true on success.
>>
>>
>> Fair, I'll change it to return -1 then. Thanks!
>
> Not quite the point I was making.  If the only two return values are
> -1/0, then bool false/true is in fact more appropriate.

If the whole code base adheres to it, maybe. The big problem with using
true/false as return values for functions that don't make it very
explicit (have an is in their name or use gerund for example). QEMU uses
a lot of 0 == success internally.

If you now add bools to the a code base that already uses int returns a
lot, you always need to look up what a function actually returns if the
caller treats it as bool (if, ?, assert, etc), making code review and
reasoning about code flows extremely hard. I've had one too many
occasions where I called an innocently looking API and put the assert()
the wrong way around for example.

I don't think we can solve this problem here, but IMHO the only sensible
way to get to good readability would be to have functions that return
success/failure return a libc defined struct that indicates the error.
Then check for success explicitly on every caller site. Overloading bool
or int for success/failure return is just bad :).

Alex