[PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

Philippe Mathieu-Daudé posted 25 patches 1 year ago
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Jean-Christophe Dubois <jcd@tribudubois.net>, "Hervé Poussineau" <hpoussin@reactos.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Stefan Weil <sw@weilnetz.de>
[PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Philippe Mathieu-Daudé 1 year ago
Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4140eb0c95..8e6fb55f59 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1498,8 +1498,10 @@ void memory_region_init_alias(MemoryRegion *mr,
  *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 337b12a674..bfe0b62d59 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
     mr->alias_offset = offset;
 }
 
-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
                                       Error **errp)
 {
-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
+    bool rv;
+
+    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
     mr->readonly = true;
+
+    return rv;
 }
 
 void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
-- 
2.41.0


Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Gavin Shan 11 months, 4 weeks ago
On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 8 ++++++--
>   2 files changed, 9 insertions(+), 3 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_rom_nomigrate() in the
commit message, mentioned by Peter Xu. With this:

Reviewed-by: Gavin Shan <gshan@redhat.com>


Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Manos Pitsidianakis 1 year ago
On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/exec/memory.h | 4 +++-
> system/memory.c       | 8 ++++++--
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/include/exec/memory.h b/include/exec/memory.h
>index 4140eb0c95..8e6fb55f59 100644
>--- a/include/exec/memory.h
>+++ b/include/exec/memory.h
>@@ -1498,8 +1498,10 @@ void memory_region_init_alias(MemoryRegion *mr,
>  *        must be unique within any device
>  * @size: size of the region.
>  * @errp: pointer to Error*, to store an error if it happens.
>+ *
>+ * Return: true on success, else false setting @errp with error.
>  */
>-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>diff --git a/system/memory.c b/system/memory.c
>index 337b12a674..bfe0b62d59 100644
>--- a/system/memory.c
>+++ b/system/memory.c
>@@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>     mr->alias_offset = offset;
> }
> 
>-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>                                       Error **errp)
> {
>-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
>+    bool rv;
>+
>+    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
>     mr->readonly = true;
>+

By the way, do we want to set mr->readonly on failure? Should there be 
modifications if an error is propagated upwards?




Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Philippe Mathieu-Daudé 10 months, 3 weeks ago
On 21/11/23 13:10, Manos Pitsidianakis wrote:
> On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> Following the example documented since commit e3fe3988d7 ("error:
>> Document Error API usage rules"), have cpu_exec_realizefn()
>> return a boolean indicating whether an error is set or not.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/exec/memory.h | 4 +++-
>> system/memory.c       | 8 ++++++--
>> 2 files changed, 9 insertions(+), 3 deletions(-)


>> diff --git a/system/memory.c b/system/memory.c
>> index 337b12a674..bfe0b62d59 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>>     mr->alias_offset = offset;
>> }
>>
>> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>                                       Object *owner,
>>                                       const char *name,
>>                                       uint64_t size,
>>                                       Error **errp)
>> {
>> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, 
>> errp);
>> +    bool rv;
>> +
>> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, 
>> size, 0, errp);
>>     mr->readonly = true;
>> +
> 
> By the way, do we want to set mr->readonly on failure? Should there be 
> modifications if an error is propagated upwards?

Good point, I'm squashing:

-- >8 --
diff --git a/system/memory.c b/system/memory.c
index a748de3694..72c6441e20 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1707,12 +1707,13 @@ bool 
memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                        uint64_t size,
                                        Error **errp)
  {
-    bool rv;
-
-    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 
0, errp);
+    if (!memory_region_init_ram_flags_nomigrate(mr, owner, name,
+                                                size, 0, errp)) {
+         return false;
+    }
      mr->readonly = true;

-    return rv;
+    return true;
  }
---

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Peter Maydell 10 months, 3 weeks ago
On Fri, 5 Jan 2024 at 14:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 21/11/23 13:10, Manos Pitsidianakis wrote:
> > On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org>
> > wrote:
> >> Following the example documented since commit e3fe3988d7 ("error:
> >> Document Error API usage rules"), have cpu_exec_realizefn()
> >> return a boolean indicating whether an error is set or not.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >> include/exec/memory.h | 4 +++-
> >> system/memory.c       | 8 ++++++--
> >> 2 files changed, 9 insertions(+), 3 deletions(-)
>
>
> >> diff --git a/system/memory.c b/system/memory.c
> >> index 337b12a674..bfe0b62d59 100644
> >> --- a/system/memory.c
> >> +++ b/system/memory.c
> >> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
> >>     mr->alias_offset = offset;
> >> }
> >>
> >> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
> >> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
> >>                                       Object *owner,
> >>                                       const char *name,
> >>                                       uint64_t size,
> >>                                       Error **errp)
> >> {
> >> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0,
> >> errp);
> >> +    bool rv;
> >> +
> >> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name,
> >> size, 0, errp);
> >>     mr->readonly = true;
> >> +
> >
> > By the way, do we want to set mr->readonly on failure? Should there be
> > modifications if an error is propagated upwards?
>
> Good point

I don't think it matters much. If the init function fails,
then the MemoryRegion is not initialized, and there's
nothing you can do with the struct except free it (if it
was in allocated memory). Whether the readonly field is
true or false doesn't matter, because conceptually it's
all undefined-values. And memory_region_init_ram_flags_nomigrate()
has already written to some fields, so avoiding changing
mr->readonly specifically doesn't seem worthwhile.

-- PMM
Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean
Posted by Philippe Mathieu-Daudé 10 months, 3 weeks ago
Hi Peter,

On 5/1/24 15:57, Peter Maydell wrote:
> On Fri, 5 Jan 2024 at 14:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 21/11/23 13:10, Manos Pitsidianakis wrote:
>>> On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org>
>>> wrote:
>>>> Following the example documented since commit e3fe3988d7 ("error:
>>>> Document Error API usage rules"), have cpu_exec_realizefn()
>>>> return a boolean indicating whether an error is set or not.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> include/exec/memory.h | 4 +++-
>>>> system/memory.c       | 8 ++++++--
>>>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>
>>
>>>> diff --git a/system/memory.c b/system/memory.c
>>>> index 337b12a674..bfe0b62d59 100644
>>>> --- a/system/memory.c
>>>> +++ b/system/memory.c
>>>> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>>>>      mr->alias_offset = offset;
>>>> }
>>>>
>>>> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>>> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>>>                                        Object *owner,
>>>>                                        const char *name,
>>>>                                        uint64_t size,
>>>>                                        Error **errp)
>>>> {
>>>> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0,
>>>> errp);
>>>> +    bool rv;
>>>> +
>>>> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name,
>>>> size, 0, errp);
>>>>      mr->readonly = true;
>>>> +
>>>
>>> By the way, do we want to set mr->readonly on failure? Should there be
>>> modifications if an error is propagated upwards?
>>
>> Good point
> 
> I don't think it matters much. If the init function fails,
> then the MemoryRegion is not initialized, and there's
> nothing you can do with the struct except free it (if it
> was in allocated memory). Whether the readonly field is
> true or false doesn't matter, because conceptually it's
> all undefined-values. And memory_region_init_ram_flags_nomigrate()
> has already written to some fields, so avoiding changing
> mr->readonly specifically doesn't seem worthwhile.

I concur with your analysis. QEMU Error* type is helpful to propagate
errors, but the cleanup path is rarely well implemented. See for
example the many returns in DeviceRealize handlers without releasing
previously allocated mem.

In this particular patch case, I find Manos suggestion useful, the
code now uses a simpler pattern and avoid having to look at the
callee implementation.

The updated patch is already in a PR I posted before reading your
comment. The changes seem innocuous to me, so not worthwhile to
restore the previous patch content. But if you object, I don't mind
reposting the PR.

Regards,

Phil.