diff mbox series

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

Message ID 20250313091350.3770394-3-maobibo@loongson.cn (mailing list archive)
State New
Headers show
Series target/loongarch: Solve some issues reported from coccinelle | expand

Commit Message

bibo mao March 13, 2025, 9:13 a.m. UTC 
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 | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

Comments

Markus Armbruster March 13, 2025, 10:32 a.m. UTC | #1 
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 | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..ab951fc642 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -913,9 +913,7 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
>      numa_cpu_pre_plug(cpu_slot, dev, &err);
>  out:
> -    if (err) {
> -        error_propagate(errp, err);
> -    }
> +    error_propagate(errp, err);
>  }
>  
>  static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
> @@ -935,9 +933,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);
>  }

This looks correct.  But I believe we can eliminate error propagation
here.  Untested patch appended.

>  
>  static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
> @@ -1001,9 +997,7 @@ 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;

Here, I'd recommend

       if (lvms->acpi_ged) {
           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
           if (err) {
               error_propagate(errp, err);
  +            return;
           }

because it makes future screwups harder.


diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..4674bd9163 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,
bibo mao March 13, 2025, 11:22 a.m. UTC | #2 
On 2025/3/13 下午6:32, 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 | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index a5840ff968..ab951fc642 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -913,9 +913,7 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>       cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
>>       numa_cpu_pre_plug(cpu_slot, dev, &err);
>>   out:
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -    }
>> +    error_propagate(errp, err);
>>   }
>>   
>>   static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>> @@ -935,9 +933,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);
>>   }
> 
> This looks correct.  But I believe we can eliminate error propagation
> here.  Untested patch appended.
Sure, will eliminate error propagation and return earlier.
Previously I always consider correctness and neglect these coding style 
details, that is actually another gap -:)

Regards
Bibo Mao

> 
>>   
>>   static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
>> @@ -1001,9 +997,7 @@ 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;
> 
> Here, I'd recommend
> 
>         if (lvms->acpi_ged) {
>             hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>             if (err) {
>                 error_propagate(errp, err);
>    +            return;
>             }
> 
> because it makes future screwups harder.
> 
> 
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..4674bd9163 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,
>
diff mbox series

Patch

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..ab951fc642 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -913,9 +913,7 @@  static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
     cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
     numa_cpu_pre_plug(cpu_slot, dev, &err);
 out:
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
@@ -935,9 +933,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 +997,7 @@  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;