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.
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;
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
@@ -859,30 +859,29 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
LoongArchCPU *cpu = LOONGARCH_CPU(dev);
CPUState *cs = CPU(dev);
CPUArchId *cpu_slot;
- Error *err = NULL;
LoongArchCPUTopo topo;
int arch_id;
if (lvms->acpi_ged) {
if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
- error_setg(&err,
+ error_setg(errp,
"Invalid thread-id %u specified, must be in range 1:%u",
cpu->thread_id, ms->smp.threads - 1);
- goto out;
+ return;
}
if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
- error_setg(&err,
+ error_setg(errp,
"Invalid core-id %u specified, must be in range 1:%u",
cpu->core_id, ms->smp.cores - 1);
- goto out;
+ return;
}
if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
- error_setg(&err,
+ error_setg(errp,
"Invalid socket-id %u specified, must be in range 1:%u",
cpu->socket_id, ms->smp.sockets - 1);
- goto out;
+ return;
}
topo.socket_id = cpu->socket_id;
@@ -891,11 +890,11 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
arch_id = virt_get_arch_id_from_topo(ms, &topo);
cpu_slot = virt_find_cpu_slot(ms, arch_id);
if (CPU(cpu_slot->cpu)) {
- error_setg(&err,
+ error_setg(errp,
"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;
+ return;
}
} else {
/* For cold-add cpu, find empty cpu slot */
@@ -911,33 +910,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
cpu->env.address_space_iocsr = &lvms->as_iocsr;
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);
- }
+ numa_cpu_pre_plug(cpu_slot, dev, errp);
}
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,
@@ -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;
}
}
--
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.
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.
I apologize for not noticing this earlier.
> 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
> @@ -859,30 +859,29 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> LoongArchCPU *cpu = LOONGARCH_CPU(dev);
> CPUState *cs = CPU(dev);
> CPUArchId *cpu_slot;
> - Error *err = NULL;
> LoongArchCPUTopo topo;
> int arch_id;
>
> if (lvms->acpi_ged) {
> if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
> - error_setg(&err,
> + error_setg(errp,
> "Invalid thread-id %u specified, must be in range 1:%u",
> cpu->thread_id, ms->smp.threads - 1);
> - goto out;
> + return;
> }
>
> if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
> - error_setg(&err,
> + error_setg(errp,
> "Invalid core-id %u specified, must be in range 1:%u",
> cpu->core_id, ms->smp.cores - 1);
> - goto out;
> + return;
> }
>
> if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
> - error_setg(&err,
> + error_setg(errp,
> "Invalid socket-id %u specified, must be in range 1:%u",
> cpu->socket_id, ms->smp.sockets - 1);
> - goto out;
> + return;
> }
>
> topo.socket_id = cpu->socket_id;
> @@ -891,11 +890,11 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> arch_id = virt_get_arch_id_from_topo(ms, &topo);
> cpu_slot = virt_find_cpu_slot(ms, arch_id);
> if (CPU(cpu_slot->cpu)) {
> - error_setg(&err,
> + error_setg(errp,
> "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;
> + return;
> }
> } else {
> /* For cold-add cpu, find empty cpu slot */
> @@ -911,33 +910,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
> cpu->env.address_space_iocsr = &lvms->as_iocsr;
> 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);
> - }
> + numa_cpu_pre_plug(cpu_slot, dev, errp);
> }
>
> 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,
> @@ -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.
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?
>
> 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.
>
>> 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
>> @@ -859,30 +859,29 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> LoongArchCPU *cpu = LOONGARCH_CPU(dev);
>> CPUState *cs = CPU(dev);
>> CPUArchId *cpu_slot;
>> - Error *err = NULL;
>> LoongArchCPUTopo topo;
>> int arch_id;
>>
>> if (lvms->acpi_ged) {
>> if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
>> - error_setg(&err,
>> + error_setg(errp,
>> "Invalid thread-id %u specified, must be in range 1:%u",
>> cpu->thread_id, ms->smp.threads - 1);
>> - goto out;
>> + return;
>> }
>>
>> if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>> - error_setg(&err,
>> + error_setg(errp,
>> "Invalid core-id %u specified, must be in range 1:%u",
>> cpu->core_id, ms->smp.cores - 1);
>> - goto out;
>> + return;
>> }
>>
>> if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>> - error_setg(&err,
>> + error_setg(errp,
>> "Invalid socket-id %u specified, must be in range 1:%u",
>> cpu->socket_id, ms->smp.sockets - 1);
>> - goto out;
>> + return;
>> }
>>
>> topo.socket_id = cpu->socket_id;
>> @@ -891,11 +890,11 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> arch_id = virt_get_arch_id_from_topo(ms, &topo);
>> cpu_slot = virt_find_cpu_slot(ms, arch_id);
>> if (CPU(cpu_slot->cpu)) {
>> - error_setg(&err,
>> + error_setg(errp,
>> "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;
>> + return;
>> }
>> } else {
>> /* For cold-add cpu, find empty cpu slot */
>> @@ -911,33 +910,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> cpu->env.address_space_iocsr = &lvms->as_iocsr;
>> 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);
>> - }
>> + numa_cpu_pre_plug(cpu_slot, dev, errp);
>> }
>>
>> 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,
>> @@ -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.
Regards
Bibo Mao
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!
© 2016 - 2025 Red Hat, Inc.