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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/06/03 19:07, Michal Prívozník wrote:
> On 6/3/24 10:50, Akihiko Odaki wrote:
>> On 2024/06/03 16:56, Michal Prívozník wrote:
>>> 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.
>>
>> It does have madvise() and QEMU on my MacBook uses it instead of
>> posix_madvise().
>>
> 
> I don't have a Mac myself, but I ran some tests on my colleague's Mac
> and yes, posix_madvise() is basically just an alias to madvise(). No
> dispute there.
> 
>> The behavior of posix_madvise() is probably just a bug (and perhaps it
>> is too late for them to fix).
>>
> 
> So what does this mean for this patch? Should I resend with the change
> you're suggesting or this is good as is? I mean, posix_madvise() is not
> going to be used on Mac anyways.

I'm for my suggestion. The current patch seems to imply that we will use 
posix_madvise() on macOS but in reality plain madivse() is used so it is 
a bit misleading. We can explicitly say we won't use posix_madvise() on 
macOS by not defining CONFIG_POSIX_MADVISE for that platform.