Re: [PATCH v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[Markus Armbruster <armbru@redhat.com>]

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.

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.