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.
+ */
+ int rc = posix_madvise(addr, len, advice);
+ if (rc) {
+ if (rc > 0) {
+ errno = rc;
+ }
+ return -1;
+ }
+ return 0;
#else
errno = EINVAL;
return -1;
--
2.44.1On 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>
> + int rc = posix_madvise(addr, len, advice);
> + if (rc) {
> + if (rc > 0) {
> + errno = rc;
> + }
> + return -1;
> + }
> + return 0;
> #else
> errno = EINVAL;
> return -1;
On 5/31/24 17: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. That's how I had it written (locally) initially, but then thought: well, what if there's another OS that behaves the same? This way, we don't have to care and just do the right thing. > > Although this approach seems reasonable, so: > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Thanks! Michal
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.
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
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(). The behavior of posix_madvise() is probably just a bug (and perhaps it is too late for them to fix).
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. Michal
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.
© 2016 - 2024 Red Hat, Inc.