[v2] backends/hostmem: Report more errors on failures

[PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by Michal Privoznik 1 year, 8 months ago

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>
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;
-- 
2.44.1

Re: [PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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;

Re: [PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by David Hildenbrand 1 year, 8 months ago

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

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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.

Re: [PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by David Hildenbrand 1 year, 8 months ago

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.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/4] osdep: Make qemu_madvise() to set errno in all cases

Posted by Michal Prívozník 1 year, 8 months ago

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