| Message ID | 393c7b26302cb445f1a086a2c80b1d718c31a071.1717168113.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backends/hostmem: Report more errors on failures | expand |
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> > + int rc = posix_madvise(addr, len, advice); > + if (rc) { > + if (rc > 0) { > + errno = rc; > + } > + return -1; > + } > + return 0; > #else > errno = EINVAL; > return -1;
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 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 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.
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;
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(-)