Bibo Mao <maob...@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 <maob...@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.
