| Message ID | e2250aa1d69faffcfd12b6d809d98b0c8157ce36.1717140354.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | backends/hostmem: Report more errors on failures | expand |
Hi Michal, On 31/5/24 09:28, 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. Watch out, Linux: RETURN VALUE On success, posix_madvise() returns 0. On failure, it returns a positive error number. but on Darwin: RETURN VALUES Upon successful completion, a value of 0 is returned. Otherwise, a value of -1 is returned and errno is set to indicate the error. (Haven't checked other POSIX OSes). So we likely need more #ifdef'ry here. > 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> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > util/osdep.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/util/osdep.c b/util/osdep.c > index e996c4744a..e42f4e8121 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -57,7 +57,12 @@ 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); > + int rc = posix_madvise(addr, len, advice); > + if (rc) { > + errno = rc; > + return -1; > + } > + return 0; > #else > errno = EINVAL; > return -1;
On 31.05.24 09:57, Philippe Mathieu-Daudé wrote: > Hi Michal, > > On 31/5/24 09:28, 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. > > Watch out, Linux: > > RETURN VALUE > > On success, posix_madvise() returns 0. On failure, > it returns a positive error number. > > but on Darwin: > > RETURN VALUES > > Upon successful completion, a value of 0 is returned. > Otherwise, a value of -1 is returned and errno is set > to indicate the error. > > (Haven't checked other POSIX OSes). > ... but it's supposed to follow the "posix standard" :D Maybe an issue in the docs? FreeBSD seems to follow the standard: "The posix_madvise() interface is identical, except it returns an error number on error and does not modify errno, and is provided for standards conformance." Same with OpenBSD: "The posix_madvise() interface has the same effect, but returns the error value instead of only setting errno."
On 31/5/24 10:01, David Hildenbrand wrote: > On 31.05.24 09:57, Philippe Mathieu-Daudé wrote: >> Hi Michal, >> >> On 31/5/24 09:28, 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. >> >> Watch out, Linux: >> >> RETURN VALUE >> >> On success, posix_madvise() returns 0. On failure, >> it returns a positive error number. >> >> but on Darwin: >> >> RETURN VALUES >> >> Upon successful completion, a value of 0 is returned. >> Otherwise, a value of -1 is returned and errno is set >> to indicate the error. >> >> (Haven't checked other POSIX OSes). >> > > ... but it's supposed to follow the "posix standard" :D Maybe an issue > in the docs? > > FreeBSD seems to follow the standard: "The posix_madvise() interface is > identical, except it returns an error number on error and does not > modify errno, and is provided for standards conformance." > > Same with OpenBSD: "The posix_madvise() interface has the same effect, > but returns the error value instead of only setting errno." On Darwin, MADVISE(2): The posix_madvise() behaves same as madvise() except that it uses values with POSIX_ prefix for the advice system call argument. The posix_madvise function is part of IEEE 1003.1-2001 and was first implemented in Mac OS X 10.2. Per IEEE 1003.1-2001 (https://pubs.opengroup.org/onlinepubs/009695399/functions/posix_madvise.html): RETURN VALUE Upon successful completion, posix_madvise() shall return zero; otherwise, an error number shall be returned to indicate the error. Note the use of "shall" which is described in RFC2119 as: This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. Regards, Phil.
On 31.05.24 10:12, Philippe Mathieu-Daudé wrote: > On 31/5/24 10:01, David Hildenbrand wrote: >> On 31.05.24 09:57, Philippe Mathieu-Daudé wrote: >>> Hi Michal, >>> >>> On 31/5/24 09:28, 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. >>> >>> Watch out, Linux: >>> >>> RETURN VALUE >>> >>> On success, posix_madvise() returns 0. On failure, >>> it returns a positive error number. >>> >>> but on Darwin: >>> >>> RETURN VALUES >>> >>> Upon successful completion, a value of 0 is returned. >>> Otherwise, a value of -1 is returned and errno is set >>> to indicate the error. >>> >>> (Haven't checked other POSIX OSes). >>> >> >> ... but it's supposed to follow the "posix standard" :D Maybe an issue >> in the docs? >> >> FreeBSD seems to follow the standard: "The posix_madvise() interface is >> identical, except it returns an error number on error and does not >> modify errno, and is provided for standards conformance." >> >> Same with OpenBSD: "The posix_madvise() interface has the same effect, >> but returns the error value instead of only setting errno." > > On Darwin, MADVISE(2): > > The posix_madvise() behaves same as madvise() except that it uses > values with POSIX_ prefix for the advice system call argument. > > The posix_madvise function is part of IEEE 1003.1-2001 and was first > implemented in Mac OS X 10.2. > > Per IEEE 1003.1-2001 > (https://pubs.opengroup.org/onlinepubs/009695399/functions/posix_madvise.html): > > RETURN VALUE > > Upon successful completion, posix_madvise() shall return zero; > otherwise, an error number shall be returned to indicate the error. > > Note the use of "shall" which is described in RFC2119 as: > > This word, or the adjective "RECOMMENDED", mean that there > may exist valid reasons in particular circumstances to ignore a > particular item, but the full implications must be understood and > carefully weighed before choosing a different course. Agreed, so we have to be careful. I do wonder if there would be the option for an automatic approach: for example, detect if the errno was/was not changed. Hm.
On 5/31/24 11:08, David Hildenbrand wrote: > On 31.05.24 10:12, Philippe Mathieu-Daudé wrote: >> On 31/5/24 10:01, David Hildenbrand wrote: >>> On 31.05.24 09:57, Philippe Mathieu-Daudé wrote: >>>> Hi Michal, >>>> >>>> On 31/5/24 09:28, 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. >>>> >>>> Watch out, Linux: >>>> >>>> RETURN VALUE >>>> >>>> On success, posix_madvise() returns 0. On failure, >>>> it returns a positive error number. >>>> >>>> but on Darwin: >>>> >>>> RETURN VALUES >>>> >>>> Upon successful completion, a value of 0 is returned. >>>> Otherwise, a value of -1 is returned and errno is set >>>> to indicate the error. >>>> >>>> (Haven't checked other POSIX OSes). >>>> >>> >>> ... but it's supposed to follow the "posix standard" :D Maybe an issue >>> in the docs? >>> >>> FreeBSD seems to follow the standard: "The posix_madvise() interface is >>> identical, except it returns an error number on error and does not >>> modify errno, and is provided for standards conformance." >>> >>> Same with OpenBSD: "The posix_madvise() interface has the same effect, >>> but returns the error value instead of only setting errno." >> >> On Darwin, MADVISE(2): >> >> The posix_madvise() behaves same as madvise() except that it uses >> values with POSIX_ prefix for the advice system call argument. >> >> The posix_madvise function is part of IEEE 1003.1-2001 and was first >> implemented in Mac OS X 10.2. >> >> Per IEEE 1003.1-2001 >> (https://pubs.opengroup.org/onlinepubs/009695399/functions/posix_madvise.html): >> >> RETURN VALUE >> >> Upon successful completion, posix_madvise() shall return zero; >> otherwise, an error number shall be returned to indicate the error. >> >> Note the use of "shall" which is described in RFC2119 as: >> >> This word, or the adjective "RECOMMENDED", mean that there >> may exist valid reasons in particular circumstances to ignore a >> particular item, but the full implications must be understood and >> carefully weighed before choosing a different course. > > Agreed, so we have to be careful. > > I do wonder if there would be the option for an automatic approach: for > example, detect if the errno was/was not changed. Hm. > Firstly, thanks Philippe for this great catch! I did think that "posix_" prefix might mean POSIX is followed. Anyway, looks like the common denominator is: on success 0 returned. And then, on Darwin, errno is set and -1 is returned. On everything(?) else, a positive value is returned and errno is left untouched. So I think we can get away with something like the following: int rc = posix_madvise(); if (rc) { if (rc > 0) { errno = rc; } return -1; } return 0; Plus a comment explaining the difference on Darwin. Michal
diff --git a/util/osdep.c b/util/osdep.c index e996c4744a..e42f4e8121 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -57,7 +57,12 @@ 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); + int rc = posix_madvise(addr, len, advice); + if (rc) { + errno = rc; + return -1; + } + return 0; #else errno = EINVAL; return -1;