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?
>
>> 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.
>
> Regards,
>
> Tony
