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 <mpriv...@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é <phi...@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. Michal
