Re: [PATCH v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[Markus Armbruster <armbru@redhat.com>]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/19 下午2:50, Markus Armbruster wrote:
>> Bibo Mao <maobibo@loongson.cn> writes:
>> 
>>> There is NULL pointer checking function error_propagate() already,
>>> it is not necessary to add checking for function parameter. Here remove
>>> NULL pointer checking with function parameter.
>> 
>> I believe the title "Remove unnecessary NULL pointer" and this paragraph
>> are remnants of your initial version, which transformed
>> 
>>      if (err) {
>>          error_propagate(errp, err);
>>      }
>> 
>> to just
>> 
>>      error_propagate(errp, err);
>> 
>> However, the patch doesn't do that anymore.
>> 
>> I think you should drop the paragraph, and replace the title.
>
> yes, the title is misleading. Originally the problem is found with 
> script scripts/coccinelle/remove_local_err.cocci, so here is the title.
>
> How about "Remove local error object" or something else. Could you 
> please provide some suggestions since English is your mother language?

My first language is German, but I've practiced writing English commit
messages for a while :)

Here's what I've used for similar patches before, adapted to this one:

    hw/loongarch/virt: Eliminate error_propagate()
    
    When all we do with an Error we receive into a local variable is
    propagating to somewhere else, we can just as well receive it there
    right away.


>> I apologize for not noticing this earlier.
>
> It is not necessary for the apologize. I appreciate your review 
> comments. With effective communication, the quality of code is better.

Thank you!

>>> Since function will return directly when there is error report, this
>>> patch removes combination about error_setg() and error_propagate(),
>>> error_setg() with dest error object is used directly such as:
>>>
>>>    error_setg(err);                 -------->      error_setg(errp);
>>>    error_propagate(errp, err);                     return;
>>>    return;
>> 
>> Yes, much of the patch does this or equivalent transformations.
>> However, there's more; see [*] below.
>> 
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>   hw/loongarch/virt.c | 33 ++++++++++++---------------------
>>>   1 file changed, 12 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>> index a5840ff968..a9fab39dd8 100644
>>> --- a/hw/loongarch/virt.c
>>> +++ b/hw/loongarch/virt.c

[...]

>>> @@ -1003,6 +993,7 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>> +            return;
>>>          }
>>>      }
>> 
>>       cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>>       cpu_slot->cpu = NULL;
>>       return;
>>   }
>> 
>> [*] This is something else.  Before the patch, we clear cpu_slot->cpu
>> evem when the last hotplug_handler() fails.  Afterwards, we don't.
>> Looks like a bug fix to me.  Either mention the fix in the commit
>> message, or split it off into a separate patch.  I'd do the latter.
>> 
> yes, will split it into two patches. The latter is bugfix, when cpu 
> fails to unplug and it should return immediately, so that system can 
> continue to run , and cpu_slot->cpu should not be cleared.

Thanks again!