[PATCH v3 0/4] backends/hostmem: Report more errors on failures

[PATCH v3 0/4] backends/hostmem: Report more errors on failures

[Michal Privoznik]

v3 of:

diff to v2:
- In 1/4 I've reworked setting errno because of how posix_madvise()
  behaves on Darwin,
- In 4/4 I'm now using size_to_str() to print the page size, oh, and the
  error message now contains the backend name too.

Michal Privoznik (4):
  osdep: Make qemu_madvise() to set errno in all cases
  osdep: Make qemu_madvise() return ENOSYS on unsupported OSes
  backends/hostmem: Report error on qemu_madvise() failures
  backends/hostmem: Report error when memory size is unaligned

 backends/hostmem.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 util/osdep.c       | 16 ++++++++++++++--
 2 files changed, 52 insertions(+), 10 deletions(-)

-- 
2.44.1

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

[Michal Privoznik]

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

[PATCH v3 2/4] osdep: Make qemu_madvise() return ENOSYS on unsupported OSes

[Michal Privoznik]

Not every OS is capable of madvise() or posix_madvise() even. In
that case, errno should be set to ENOSYS as it reflects the cause
better.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 util/osdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/osdep.c b/util/osdep.c
index 1345238a5c..4a8920ba93 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -71,7 +71,7 @@ int qemu_madvise(void *addr, size_t len, int advice)
     }
     return 0;
 #else
-    errno = EINVAL;
+    errno = ENOSYS;
     return -1;
 #endif
 }
-- 
2.44.1

[PATCH v3 3/4] backends/hostmem: Report error on qemu_madvise() failures

[Michal Privoznik]

If user sets .merge or .dump attributes qemu_madvise() is called
with corresponding advice. But it is never checked for failure
which may mislead users into thinking the attribute is set
correctly. Report an appropriate error.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 backends/hostmem.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index eb9682b4a8..012a8c190f 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -178,8 +178,14 @@ static void host_memory_backend_set_merge(Object *obj, bool value, Error **errp)
         void *ptr = memory_region_get_ram_ptr(&backend->mr);
         uint64_t sz = memory_region_size(&backend->mr);
 
-        qemu_madvise(ptr, sz,
-                     value ? QEMU_MADV_MERGEABLE : QEMU_MADV_UNMERGEABLE);
+        if (qemu_madvise(ptr, sz,
+                         value ? QEMU_MADV_MERGEABLE : QEMU_MADV_UNMERGEABLE)) {
+            error_setg_errno(errp, errno,
+                             "Couldn't change property 'merge' on '%s'",
+                             object_get_typename(obj));
+            return;
+        }
+
         backend->merge = value;
     }
 }
@@ -204,8 +210,14 @@ static void host_memory_backend_set_dump(Object *obj, bool value, Error **errp)
         void *ptr = memory_region_get_ram_ptr(&backend->mr);
         uint64_t sz = memory_region_size(&backend->mr);
 
-        qemu_madvise(ptr, sz,
-                     value ? QEMU_MADV_DODUMP : QEMU_MADV_DONTDUMP);
+        if (qemu_madvise(ptr, sz,
+                     value ? QEMU_MADV_DODUMP : QEMU_MADV_DONTDUMP)) {
+            error_setg_errno(errp, errno,
+                             "Couldn't change property 'dump' on '%s'",
+                             object_get_typename(obj));
+            return;
+        }
+
         backend->dump = value;
     }
 }
@@ -337,11 +349,19 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
     ptr = memory_region_get_ram_ptr(&backend->mr);
     sz = memory_region_size(&backend->mr);
 
-    if (backend->merge) {
-        qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE);
+    if (backend->merge &&
+        qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE)) {
+        error_setg_errno(errp, errno,
+                         "Couldn't set property 'merge' on '%s'",
+                         object_get_typename(OBJECT(uc)));
+        return;
     }
-    if (!backend->dump) {
-        qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
+    if (!backend->dump &&
+        qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP)) {
+        error_setg_errno(errp, errno,
+                         "Couldn't set property 'dump' on '%s'",
+                         object_get_typename(OBJECT(uc)));
+        return;
     }
 #ifdef CONFIG_NUMA
     unsigned long lastbit = find_last_bit(backend->host_nodes, MAX_NODES);
-- 
2.44.1

[PATCH v3 4/4] backends/hostmem: Report error when memory size is unaligned

[Michal Privoznik]

If memory-backend-{file,ram} has a size that's not aligned to
underlying page size it is not only wasteful, but also may lead
to hard to debug behaviour. For instance, in case
memory-backend-file and hugepages, madvise() and mbind() fail.
Rightfully so, page is the smallest unit they can work with. And
even though an error is reported, the root cause it not very
clear:

  qemu-system-x86_64: Couldn't set property 'dump' on 'memory-backend-file': Invalid argument

After this commit:

  qemu-system-x86_64: backend 'memory-backend-file' memory size must be multiple of 2 MiB

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 backends/hostmem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 012a8c190f..4d6c69fe4d 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -20,6 +20,7 @@
 #include "qom/object_interfaces.h"
 #include "qemu/mmap-alloc.h"
 #include "qemu/madvise.h"
+#include "qemu/cutils.h"
 #include "hw/qdev-core.h"
 
 #ifdef CONFIG_NUMA
@@ -337,6 +338,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
     HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
     void *ptr;
     uint64_t sz;
+    size_t pagesize;
     bool async = !phase_check(PHASE_LATE_BACKENDS_CREATED);
 
     if (!bc->alloc) {
@@ -348,6 +350,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
 
     ptr = memory_region_get_ram_ptr(&backend->mr);
     sz = memory_region_size(&backend->mr);
+    pagesize = qemu_ram_pagesize(backend->mr.ram_block);
+
+    if (!QEMU_IS_ALIGNED(sz, pagesize)) {
+        g_autofree char *pagesize_str = size_to_str(pagesize);
+        error_setg(errp, "backend '%s' memory size must be multiple of %s",
+                   object_get_typename(OBJECT(uc)), pagesize_str);
+        return;
+    }
 
     if (backend->merge &&
         qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE)) {
-- 
2.44.1

Re: [PATCH v3 4/4] backends/hostmem: Report error when memory size is unaligned

[Philippe Mathieu-Daudé]

On 31/5/24 17:10, Michal Privoznik wrote:
> If memory-backend-{file,ram} has a size that's not aligned to
> underlying page size it is not only wasteful, but also may lead
> to hard to debug behaviour. For instance, in case
> memory-backend-file and hugepages, madvise() and mbind() fail.
> Rightfully so, page is the smallest unit they can work with. And
> even though an error is reported, the root cause it not very
> clear:
> 
>    qemu-system-x86_64: Couldn't set property 'dump' on 'memory-backend-file': Invalid argument
> 
> After this commit:
> 
>    qemu-system-x86_64: backend 'memory-backend-file' memory size must be multiple of 2 MiB

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   backends/hostmem.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)

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

[Philippe Mathieu-Daudé]

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;

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

[Akihiko Odaki]

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.

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

[Michal Prívozník]

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

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

[Michal Prívozník]

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

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

[Akihiko Odaki]

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

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

[Michal Prívozník]

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

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

[Akihiko Odaki]

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.