On Mon, Mar 17, 2025 at 06:21:13PM +0800, Chenyi Qiang wrote:
>
>
> On 3/17/2025 5:45 PM, Tony Lindgren wrote:
> > 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 putting the whole thing separately, I think the rest part to do error
> handling still needs to add the same operation. Or I misunderstand
> something?
So maybe you suggestion of just a separate clean-up function would work:
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)) {
memory_attribute_manager_cleanup(...);
goto out_free;
}
> >> 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.
>
> I'm not sure if we should add such change directly, or we can wait for
> the real error check introduced in future.
Right, not sure either.
Regards,
Tony
