On Mon, Mar 17, 2025 at 03:32:16PM +0800, Chenyi Qiang wrote:
>
>
> On 3/17/2025 2:18 PM, Tony Lindgren wrote:
> > Hi,
> >
> > On Mon, Mar 10, 2025 at 04:18:34PM +0800, Chenyi Qiang wrote:
> >> --- a/system/physmem.c
> >> +++ b/system/physmem.c
> >> @@ -1885,6 +1886,16 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> >> qemu_mutex_unlock_ramlist();
> >> goto out_free;
> >> }
> >> +
> >> + new_block->memory_attribute_manager = MEMORY_ATTRIBUTE_MANAGER(object_new(TYPE_MEMORY_ATTRIBUTE_MANAGER));
> >> + if (memory_attribute_manager_realize(new_block->memory_attribute_manager, new_block->mr)) {
> >> + error_setg(errp, "Failed to realize memory attribute manager");
> >> + object_unref(OBJECT(new_block->memory_attribute_manager));
> >> + close(new_block->guest_memfd);
> >> + ram_block_discard_require(false);
> >> + qemu_mutex_unlock_ramlist();
> >> + goto out_free;
> >> + }
> >> }
> >>
> >> ram_size = (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS;
> >
> > Might as well put the above into a separate memory manager init function
> > to start with. It keeps the goto out_free error path unified, and makes
> > things more future proof if the rest of ram_block_add() ever develops a
> > need to check for errors.
>
> Which part to be defined in a separate function? The init function of
> object_new() + realize(), or the error handling operation
> (object_unref() + close() + ram_block_discard_require(false))?
I was thinking the whole thing, including freeing :) But maybe there's
something more to consider to keep calls paired.
> If need to check for errors in the rest of ram_block_add() in future,
> how about adding a new label before out_free and move the error handling
> there?
Yeah that would work too.
Regards,
Tony
