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