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

[Peter Maydell]

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