Re: [PATCH v3 1/4] osdep: Make qemu_madvise() to set errno in all cases

["Michal Prívozník" <mprivozn@redhat.com>]

On 6/2/24 08:26, Akihiko Odaki wrote:
> On 2024/06/01 0:46, Philippe Mathieu-Daudé wrote:
>> On 31/5/24 17:10, Michal Privoznik wrote:
>>> The unspoken premise of qemu_madvise() is that errno is set on
>>> error. And it is mostly the case except for posix_madvise() which
>>> is documented to return either zero (on success) or a positive
>>> error number. This means, we must set errno ourselves. And while
>>> at it, make the function return a negative value on error, just
>>> like other error paths do.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>   util/osdep.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/util/osdep.c b/util/osdep.c
>>> index e996c4744a..1345238a5c 100644
>>> --- a/util/osdep.c
>>> +++ b/util/osdep.c
>>> @@ -57,7 +57,19 @@ int qemu_madvise(void *addr, size_t len, int advice)
>>>   #if defined(CONFIG_MADVISE)
>>>       return madvise(addr, len, advice);
>>>   #elif defined(CONFIG_POSIX_MADVISE)
>>> -    return posix_madvise(addr, len, advice);
>>> +    /*
>>> +     * On Darwin posix_madvise() has the same return semantics as
>>> +     * plain madvise, i.e. errno is set and -1 is returned. Otherwise,
>>> +     * a positive error number is returned.
>>> +     */
>>
>> Alternative is to guard with #ifdef CONFIG_DARWIN ... #else ... #endif
>> which might be clearer.
>>
>> Although this approach seems reasonable, so:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> We should use plain madvise() if posix_madvise() is broken. In fact,
> QEMU detects the availability of plain madvise() and use it instead of
> posix_madvise() on my MacBook.
> 
> Perhaps it may be better to stop defining CONFIG_POSIX_MADVISE on Darwin
> to ensure we never use the broken implementation.
> 

Well, doesn't Darwin have madvise() in the first place?

https://opensource.apple.com/source/xnu/xnu-7195.81.3/bsd/man/man2/madvise.2.auto.html

I thought that's the reason for posix_madvise() to behave the same as madvise() there.

Michal