Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking

[Markus Armbruster <armbru@redhat.com>]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/14 下午5:11, 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.
>>>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

[...]

>>> @@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>         cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>>         cpu_slot->cpu = CPU(dev);
>>         if (lvms->ipi) {
>>             hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
>>             if (err) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
>> 
>>         if (lvms->extioi) {
>>             hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>>             if (err) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
>>>   
>>>       if (lvms->acpi_ged) {
>>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>> -        if (err) {
>>> -            error_propagate(errp, err);
>>> -        }
>>> +        error_propagate(errp, err);
>>> +        return;
>>>       }
>>>   
>>>       return;
>> 
>> Better make this work exactly like the other checks above, and drop the
>> final return, which serves no purpose:
>> 
>>             if (err) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
> Here is report from commandline, it say err NULL check is not necessary
> spatch --sp-file scripts/coccinelle/error_propagate_null.cocci --dir 
> hw/loongarch/
>
> @@ -1001,9 +997,7 @@ static void virt_cpu_plug(HotplugHandler
>
>       if (lvms->acpi_ged) {
>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -        }
> +        error_propagate(errp, err);
>       }

This change is correct.  The change I suggests is also correct.  I
prefer mine because it's less easy to screw up.  If you add another
check at the end, my version keeps working, while yours needs an update,
which is easy to miss.