Re: [PATCH v6 5/8] memory: Factor out common ram region initialization
On Thu, Mar 05, 2026 at 10:57:19AM +0900, Akihiko Odaki wrote: > But looking at the code, the functions generating errors (e.g., > qemu_ram_alloc()) return values that indicate failures (NULL), so I now > think we should use the first pattern I cited (i.e., check the returned > value instead of err) and remove the err variable and error_propagate() > altogether instead of factoring them out with > memory_region_error_propagate() or ERRP_GUARD(). I agree, this looks better. -- Peter Xu
Re: [PATCH v6 5/8] memory: Factor out common ram region initialization
On 2026/03/04 20:38, BALATON Zoltan wrote:
On Wed, 4 Mar 2026, Akihiko Odaki wrote:
On 2026/03/04 6:47, BALATON Zoltan wrote:
Introduce internal helper functions to remove duplicated code from
different memory_region_init_*ram functions.
Signed-off-by: BALATON Zoltan
---
system/memory.c | 132 ++--
1 file changed, 50 insertions(+), 82 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index e8c4912a60..1b26c6b5a5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1589,19 +1589,16 @@ bool
memory_region_init_ram_nomigrate(MemoryRegion *mr,
size, 0, errp);
}
-bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- uint32_t ram_flags,
- Error **errp)
+static void memory_region_setup_ram(MemoryRegion *mr)
{
- Error *err = NULL;
- memory_region_init(mr, owner, name, size);
mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
- mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+}
+
+static bool memory_region_error_propagate(MemoryRegion *mr,
+ Error *err, Error **errp)
+{
if (err) {
mr->size = int128_zero();
object_unparent(OBJECT(mr));
@@ -1611,6 +1608,18 @@ bool
memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
return true;
}
I think the optimal way to factor out error propagation is to use
ERRP_GUARD().
I don't think ERRP_GUARD applies here as we do not dereference errp and
this does more than just propagate the error so I don't see how
ERRP_GUARD could help here.
include/qapi/error.h has an extensive documentation describing how
errors should be passed around:
> Call a function, receive an error from it, and pass it to the caller
> - when the function returns a value that indicates failure, say
> false:
> if (!foo(arg, errp)) {
> handle the error...
> }
> - when it does not, say because it is a void function:
> ERRP_GUARD();
> foo(arg, errp);
> if (*errp) {
> handle the error...
> }
> More on ERRP_GUARD() below.
>
> Code predating ERRP_GUARD() still exists, and looks like this:
> Error *err = NULL;
> foo(arg, &err);
> if (err) {
> handle the error...
> error_propagate(errp, err); // deprecated
> }
> Avoid in new code. Do *not* "optimize" it to
> foo(arg, errp);
> if (*errp) { // WRONG!
> handle the error...
> }
> because errp may be NULL without the ERRP_GUARD() guard.
>
> But when all you do with the error is pass it on, please use
> foo(arg, errp);
> for readability.
>
> Receive an error, and handle it locally
> - when the function returns a value that indicates failure, say
> false:
> Error *err = NULL;
> if (!foo(arg, &err)) {
> handle the error...
> }
> - when it does not, say because it is a void function:
> Error *err = NULL;
> foo(arg, &err);
> if (err) {
> handle the error...
> }
>
> Pass an existing error to the caller:
> error_propagate(errp, err);
> This is rarely needed. When @err is a local variable, use of
> ERRP_GUARD() commonly results in more readable code.
But looking at the code, the functions generating errors (e.g.,
qemu_ram_alloc()) return values that indicate failures (NULL), so I now
think we should use the first pattern I cited (i.e., check the returned
value instead of err) and remove the err variable and error_propagate()
altogether instead of factoring them out with
memory_region_error_propagate() or ERRP_GUARD().
Regards,
Akihiko Odaki
Re: [PATCH v6 5/8] memory: Factor out common ram region initialization
On Wed, 4 Mar 2026, Akihiko Odaki wrote:
On 2026/03/04 6:47, BALATON Zoltan wrote:
Introduce internal helper functions to remove duplicated code from
different memory_region_init_*ram functions.
Signed-off-by: BALATON Zoltan
---
system/memory.c | 132 ++--
1 file changed, 50 insertions(+), 82 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index e8c4912a60..1b26c6b5a5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1589,19 +1589,16 @@ bool memory_region_init_ram_nomigrate(MemoryRegion
*mr,
size, 0, errp);
}
-bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
-Object *owner,
-const char *name,
-uint64_t size,
-uint32_t ram_flags,
-Error **errp)
+static void memory_region_setup_ram(MemoryRegion *mr)
{
-Error *err = NULL;
-memory_region_init(mr, owner, name, size);
mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
-mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+}
+
+static bool memory_region_error_propagate(MemoryRegion *mr,
+ Error *err, Error **errp)
+{
if (err) {
mr->size = int128_zero();
object_unparent(OBJECT(mr));
@@ -1611,6 +1608,18 @@ bool
memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
return true;
}
I think the optimal way to factor out error propagation is to use
ERRP_GUARD().
I don't think ERRP_GUARD applies here as we do not dereference errp and
this does more than just propagate the error so I don't see how ERRP_GUARD
could help here.
Regards,
BALATON Zoltan
Re: [PATCH v6 5/8] memory: Factor out common ram region initialization
On 2026/03/04 6:47, BALATON Zoltan wrote:
Introduce internal helper functions to remove duplicated code from
different memory_region_init_*ram functions.
Signed-off-by: BALATON Zoltan
---
system/memory.c | 132 ++--
1 file changed, 50 insertions(+), 82 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index e8c4912a60..1b26c6b5a5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1589,19 +1589,16 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
size, 0, errp);
}
-bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
-Object *owner,
-const char *name,
-uint64_t size,
-uint32_t ram_flags,
-Error **errp)
+static void memory_region_setup_ram(MemoryRegion *mr)
{
-Error *err = NULL;
-memory_region_init(mr, owner, name, size);
mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
-mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+}
+
+static bool memory_region_error_propagate(MemoryRegion *mr,
+ Error *err, Error **errp)
+{
if (err) {
mr->size = int128_zero();
object_unparent(OBJECT(mr));
@@ -1611,6 +1608,18 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion
*mr,
return true;
}
I think the optimal way to factor out error propagation is to use
ERRP_GUARD().
Regards,
Akihiko Odaki
+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
+const char *name, uint64_t size,
+uint32_t ram_flags, Error **errp)
+{
+Error *err = NULL;
+
+memory_region_init(mr, owner, name, size);
+memory_region_setup_ram(mr);
+mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+return memory_region_error_propagate(mr, err, errp);
+}
+
bool memory_region_init_resizeable_ram(MemoryRegion *mr,
Object *owner,
const char *name,
@@ -1622,108 +1631,69 @@ bool memory_region_init_resizeable_ram(MemoryRegion
*mr,
Error **errp)
{
Error *err = NULL;
+
memory_region_init(mr, owner, name, size);
-mr->ram = true;
-mr->terminates = true;
-mr->destructor = memory_region_destructor_ram;
-mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
- mr, &err);
-if (err) {
-mr->size = int128_zero();
-object_unparent(OBJECT(mr));
-error_propagate(errp, err);
-return false;
-}
-return true;
+memory_region_setup_ram(mr);
+mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
+ &err);
+return memory_region_error_propagate(mr, err, errp);
}
#if defined(CONFIG_POSIX) && !defined(EMSCRIPTEN)
-bool memory_region_init_ram_from_file(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- uint64_t align,
- uint32_t ram_flags,
- const char *path,
- ram_addr_t offset,
+bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ uint64_t align, uint32_t ram_flags,
+ const char *path, ram_addr_t offset,
Error **errp)
{
Error *err = NULL;
+
memory_region_init(mr, owner, name, size);
-mr->ram = true;
+memory_region_setup_ram(mr);
mr->readonly = !!(ram_flags & RAM_READONLY);
-mr->terminates = true;
-mr->destructor = memory_region_destructor_ram;
mr->align = align;
-mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
- offset, &err);
-if (err) {
-mr->size = int128_zero();
-object_unparent(OBJECT(mr));
-error_propagate(errp, err);
-return false;
-}
-return true;
+mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset,
+ &err);
+return memory_region_error_propagate(mr, err, errp);
}
-bool memory_region_init_ram_from_fd(MemoryRegion *mr,
-Object *owner,
-
