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>
---
hw/loongarch/virt.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..d82676d316 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
error_setg(&err,
"Invalid thread-id %u specified, must be in range 1:%u",
cpu->thread_id, ms->smp.threads - 1);
- goto out;
+ error_propagate(errp, err);
+ return;
}
if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
error_setg(&err,
"Invalid core-id %u specified, must be in range 1:%u",
cpu->core_id, ms->smp.cores - 1);
- goto out;
+ error_propagate(errp, err);
+ return;
}
if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
error_setg(&err,
"Invalid socket-id %u specified, must be in range 1:%u",
cpu->socket_id, ms->smp.sockets - 1);
- goto out;
+ error_propagate(errp, err);
+ return;
}
topo.socket_id = cpu->socket_id;
@@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
"cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
cs->cpu_index, cpu->socket_id, cpu->core_id,
cpu->thread_id, cpu_slot->arch_id);
- goto out;
+ error_propagate(errp, err);
+ return;
}
} else {
/* For cold-add cpu, find empty cpu slot */
@@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
cpu->phy_id = cpu_slot->arch_id;
cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
numa_cpu_pre_plug(cpu_slot, dev, &err);
-out:
- if (err) {
- error_propagate(errp, err);
- }
}
static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
@@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
}
hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
- if (err) {
- error_propagate(errp, err);
- }
+ error_propagate(errp, err);
}
static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
@@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
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;
--
2.39.3Bibo 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>
> ---
> hw/loongarch/virt.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..d82676d316 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> error_setg(&err,
> "Invalid thread-id %u specified, must be in range 1:%u",
> cpu->thread_id, ms->smp.threads - 1);
> - goto out;
> + error_propagate(errp, err);
> + return;
Make this
error_setg(&err, ...);
return;
> }
>
> if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
> error_setg(&err,
> "Invalid core-id %u specified, must be in range 1:%u",
> cpu->core_id, ms->smp.cores - 1);
> - goto out;
> + error_propagate(errp, err);
> + return;
Likewise.
> }
>
> if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
> error_setg(&err,
> "Invalid socket-id %u specified, must be in range 1:%u",
> cpu->socket_id, ms->smp.sockets - 1);
> - goto out;
> + error_propagate(errp, err);
> + return;
Likewise.
> }
>
> topo.socket_id = cpu->socket_id;
> @@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
> cs->cpu_index, cpu->socket_id, cpu->core_id,
> cpu->thread_id, cpu_slot->arch_id);
> - goto out;
> + error_propagate(errp, err);
> + return;
Likewise.
> }
> } else {
> /* For cold-add cpu, find empty cpu slot */
> @@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> cpu->phy_id = cpu_slot->arch_id;
> cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
> numa_cpu_pre_plug(cpu_slot, dev, &err);
You need to pass errp instead of &err now.
> -out:
> - if (err) {
> - error_propagate(errp, err);
> - }
> }
>
> static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
> @@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
> }
>
> hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> - if (err) {
> - error_propagate(errp, err);
> - }
> + error_propagate(errp, err);
> }
Correct, but I'd recomment to go one step further:
static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
- Error *err = NULL;
LoongArchCPU *cpu = LOONGARCH_CPU(dev);
CPUState *cs = CPU(dev);
if (cs->cpu_index == 0) {
- error_setg(&err, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
+ error_setg(errp, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
cs->cpu_index, cpu->socket_id,
cpu->core_id, cpu->thread_id);
- error_propagate(errp, err);
return;
}
- hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
- if (err) {
- error_propagate(errp, err);
- }
+ hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, errp);
}
>
> static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
> @@ -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;
}
}
}
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>
>> ---
>> hw/loongarch/virt.c | 25 +++++++++++--------------
>> 1 file changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index a5840ff968..d82676d316 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> error_setg(&err,
>> "Invalid thread-id %u specified, must be in range 1:%u",
>> cpu->thread_id, ms->smp.threads - 1);
>> - goto out;
>> + error_propagate(errp, err);
>> + return;
>
> Make this
>
> error_setg(&err, ...);
> return;
My bad. I remember replacing error_propagate with error_setg(errp, ...)
already. Will do
>
>> }
>>
>> if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>> error_setg(&err,
>> "Invalid core-id %u specified, must be in range 1:%u",
>> cpu->core_id, ms->smp.cores - 1);
>> - goto out;
>> + error_propagate(errp, err);
>> + return;
>
> Likewise.
>
>> }
>>
>> if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>> error_setg(&err,
>> "Invalid socket-id %u specified, must be in range 1:%u",
>> cpu->socket_id, ms->smp.sockets - 1);
>> - goto out;
>> + error_propagate(errp, err);
>> + return;
>
> Likewise.
>
>> }
>>
>> topo.socket_id = cpu->socket_id;
>> @@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
>> cs->cpu_index, cpu->socket_id, cpu->core_id,
>> cpu->thread_id, cpu_slot->arch_id);
>> - goto out;
>> + error_propagate(errp, err);
>> + return;
>
> Likewise.
>
>> }
>> } else {
>> /* For cold-add cpu, find empty cpu slot */
>> @@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> cpu->phy_id = cpu_slot->arch_id;
>> cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
>> numa_cpu_pre_plug(cpu_slot, dev, &err);
>
> You need to pass errp instead of &err now.
>
>> -out:
>> - if (err) {
>> - error_propagate(errp, err);
>> - }
>> }
>>
>> static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>> @@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>> }
>>
>> hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>> - if (err) {
>> - error_propagate(errp, err);
>> - }
>> + error_propagate(errp, err);
>> }
>
> Correct, but I'd recomment to go one step further:
>
> static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
> - Error *err = NULL;
> LoongArchCPU *cpu = LOONGARCH_CPU(dev);
> CPUState *cs = CPU(dev);
>
> if (cs->cpu_index == 0) {
> - error_setg(&err, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
> + error_setg(errp, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
> cs->cpu_index, cpu->socket_id,
> cpu->core_id, cpu->thread_id);
> - error_propagate(errp, err);
> return;
> }
>
> - hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> - if (err) {
> - error_propagate(errp, err);
> - }
> + hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, errp);
> }
>
>>
>> static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
>> @@ -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);
}
Regards
Bibo Mao
> }
>
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.
On 2025/3/14 下午5:38, Markus Armbruster wrote:
> 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.
>
I see until checking file scripts/coccinelle/error_propagate_null.cocci
-if (L) {
error_propagate(E, L);
-}
you are error log expert, of source do with your suggestion -:)
And thanks for your earnest explanation.
Regards
Bibo Mao
© 2016 - 2025 Red Hat, Inc.