Introduce internal helper functions to remove duplicated code from
different memory_region_init_*ram functions.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
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;
}
+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,
- const char *name,
- uint64_t size,
- uint32_t ram_flags,
- int fd,
- ram_addr_t offset,
- Error **errp)
+bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ uint32_t ram_flags, int fd,
+ 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->ram_block = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
offset, false, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
- return false;
- }
- return true;
+ return memory_region_error_propagate(mr, err, errp);
}
#endif
-void memory_region_init_ram_ptr(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- void *ptr)
+void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size, void *ptr)
{
memory_region_init(mr, owner, name, size);
- mr->ram = true;
- mr->terminates = true;
- mr->destructor = memory_region_destructor_ram;
-
+ memory_region_setup_ram(mr);
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
assert(ptr != NULL);
mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
}
-void memory_region_init_ram_device_ptr(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
+void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
void *ptr)
{
memory_region_init(mr, owner, name, size);
- mr->ram = true;
- mr->ram_device = true;
memory_region_set_ops(mr, &ram_device_mem_ops, mr);
- mr->destructor = memory_region_destructor_ram;
-
+ memory_region_setup_ram(mr);
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
assert(ptr != NULL);
mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
+ mr->ram_device = true;
}
void memory_region_init_alias(MemoryRegion *mr,
@@ -3774,15 +3744,13 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
assert(ops);
memory_region_init(mr, owner, name, size);
memory_region_set_ops(mr, ops, opaque);
- mr->rom_device = true;
- mr->destructor = memory_region_destructor_ram;
+ memory_region_setup_ram(mr);
mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
+ if (!memory_region_error_propagate(mr, err, errp)) {
return false;
}
+ mr->ram = false;
+ mr->rom_device = true;
/* This will assert if owner is neither NULL nor a DeviceState.
* We only want the owner here for the purposes of defining a
* unique name for migration. TODO: Ideally we should implement
--
2.41.3On 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 <balaton@eik.bme.hu>
> ---
> 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,
> - const char *name,
> - uint64_t size,
> - uint32_t ram_flags,
> - int fd,
> - ram_addr_t offset,
> - Error **errp)
> +bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
> + const char *name, uint64_t size,
> + uint32_t ram_flags, int fd,
> + 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->ram_block = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
> offset, false, &err);
> - if (err) {
> - mr->size = int128_zero();
> - object_unparent(OBJECT(mr));
> - error_propagate(errp, err);
> - return false;
> - }
> - return true;
> + return memory_region_error_propagate(mr, err, errp);
> }
> #endif
>
> -void memory_region_init_ram_ptr(MemoryRegion *mr,
> - Object *owner,
> - const char *name,
> - uint64_t size,
> - void *ptr)
> +void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
> + const char *name, uint64_t size, void *ptr)
> {
> memory_region_init(mr, owner, name, size);
> - mr->ram = true;
> - mr->terminates = true;
> - mr->destructor = memory_region_destructor_ram;
> -
> + memory_region_setup_ram(mr);
> /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
> assert(ptr != NULL);
> mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
> }
>
> -void memory_region_init_ram_device_ptr(MemoryRegion *mr,
> - Object *owner,
> - const char *name,
> - uint64_t size,
> +void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
> + const char *name, uint64_t size,
> void *ptr)
> {
> memory_region_init(mr, owner, name, size);
> - mr->ram = true;
> - mr->ram_device = true;
> memory_region_set_ops(mr, &ram_device_mem_ops, mr);
> - mr->destructor = memory_region_destructor_ram;
> -
> + memory_region_setup_ram(mr);
> /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
> assert(ptr != NULL);
> mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
> + mr->ram_device = true;
> }
>
> void memory_region_init_alias(MemoryRegion *mr,
> @@ -3774,15 +3744,13 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
> assert(ops);
> memory_region_init(mr, owner, name, size);
> memory_region_set_ops(mr, ops, opaque);
> - mr->rom_device = true;
> - mr->destructor = memory_region_destructor_ram;
> + memory_region_setup_ram(mr);
> mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
> - if (err) {
> - mr->size = int128_zero();
> - object_unparent(OBJECT(mr));
> - error_propagate(errp, err);
> + if (!memory_region_error_propagate(mr, err, errp)) {
> return false;
> }
> + mr->ram = false;
> + mr->rom_device = true;
> /* This will assert if owner is neither NULL nor a DeviceState.
> * We only want the owner here for the purposes of defining a
> * unique name for migration. TODO: Ideally we should implement
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 <balaton@eik.bme.hu>
>> ---
>> 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
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 <balaton@eik.bme.hu>
>>> ---
>>> 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
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
© 2016 - 2026 Red Hat, Inc.