bibo mao <[email protected]> writes:
On 2025/3/19 下午2:50, Markus Armbruster wrote:
>> Bibo Mao <[email protected]> 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 <[email protected]>
>>> ---
>>> 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!
