| Message ID | 20231120213301.24349-4-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | memory: Propagate Error* when possible | expand |
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?
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>
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; } ---
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
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.
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,
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(-)