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. Michal
