[PATCH 0/3] target/loongarch: Solve some issues reported from coccinelle

[PATCH 0/3] target/loongarch: Solve some issues reported from coccinelle

[Bibo Mao]

This patch set solves errors reported by coccinelle tool with commands:
  spatch --sp-file scripts/coccinelle/*.cocci --dir target/loongarch/
  spatch --sp-file scripts/coccinelle/*.cocci --dir hw/loongarch/

The main problem is that qemu should fail to run when feature is forced
to enabled however KVM does not support it, rather than report error and
continue to run.

Bibo Mao (3):
  target/loongarch: Return directly when detect KVM disabled features
  hw/loongarch/virt: Remove unnecessary NULL pointer checking
  target/loongarch: Remove unnecessary temporary variable assignment

 hw/loongarch/virt.c               | 12 +++---------
 target/loongarch/kvm/kvm.c        |  4 ++++
 target/loongarch/tcg/tlb_helper.c |  5 ++---
 3 files changed, 9 insertions(+), 12 deletions(-)


base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
-- 
2.39.3

[PATCH 1/3] target/loongarch: Return directly when detect KVM disabled features

[Bibo Mao]

For some paravirt KVM features, if user forces to enable it however
KVM does not support, qemu should fail to run. Here set error message
and return directly in function kvm_arch_init_vcpu().

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 target/loongarch/kvm/kvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
index 28735c80be..b7f370ba97 100644
--- a/target/loongarch/kvm/kvm.c
+++ b/target/loongarch/kvm/kvm.c
@@ -1091,21 +1091,25 @@ int kvm_arch_init_vcpu(CPUState *cs)
     ret = kvm_cpu_check_lsx(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_lasx(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_lbt(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_pmu(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_pv_features(cs, &local_err);
-- 
2.39.3

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

[Bibo Mao]

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);
 }
 
 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;
-- 
2.39.3

[PATCH 3/3] target/loongarch: Remove unnecessary temporary variable assignment

[Bibo Mao]

Temporary variable ret is assigned at last line and return, it can
be removed and return directly.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 target/loongarch/tcg/tlb_helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/loongarch/tcg/tlb_helper.c b/target/loongarch/tcg/tlb_helper.c
index 646dbf59de..182881a237 100644
--- a/target/loongarch/tcg/tlb_helper.c
+++ b/target/loongarch/tcg/tlb_helper.c
@@ -543,7 +543,7 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
                           target_ulong level, uint32_t mem_idx)
 {
     CPUState *cs = env_cpu(env);
-    target_ulong badvaddr, index, phys, ret;
+    target_ulong badvaddr, index, phys;
     uint64_t dir_base, dir_width;
 
     if (unlikely((level == 0) || (level > 4))) {
@@ -571,8 +571,7 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
     get_dir_base_width(env, &dir_base, &dir_width, level);
     index = (badvaddr >> dir_base) & ((1 << dir_width) - 1);
     phys = base | index << 3;
-    ret = ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
-    return ret;
+    return ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
 }
 
 void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
-- 
2.39.3

Re: [PATCH 1/3] target/loongarch: Return directly when detect KVM disabled features

[Markus Armbruster]

Suggest something like

  arget/loongarch: Fix error handling of KVM feature checks

That way, the nature of the patch (it's an error handling bug fix) is
obvious at a glance.

Bibo Mao <maobibo@loongson.cn> writes:

> For some paravirt KVM features, if user forces to enable it however
> KVM does not support, qemu should fail to run. Here set error message
> and return directly in function kvm_arch_init_vcpu().
>

Please add

  Fixes: 6edd2a9bec90 (target/loongarch/kvm: Implement LoongArch PMU extension)
  Fixes: 936c3f4d7916 (target/loongarch: Use auto method with LSX feature)
  Fixes: 5e360dabedb1 (target/loongarch: Use auto method with LASX feature)
  Fixes: 620d9bd0022e (target/loongarch: Add paravirt ipi feature detection) 

> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  target/loongarch/kvm/kvm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
> index 28735c80be..b7f370ba97 100644
> --- a/target/loongarch/kvm/kvm.c
> +++ b/target/loongarch/kvm/kvm.c
   int kvm_arch_init_vcpu(CPUState *cs)
   {
       uint64_t val;
       int ret;
       Error *local_err = NULL;

       ret = 0;

Please drop this assignment, it's useless.

       qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs);

       if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) {
           brk_insn = val;
       }

> @@ -1091,21 +1091,25 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      ret = kvm_cpu_check_lsx(cs, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
> +        return ret;
>      }
>  
>      ret = kvm_cpu_check_lasx(cs, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
> +        return ret;
>      }
>  
>      ret = kvm_cpu_check_lbt(cs, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
> +        return ret;
>      }
>  
>      ret = kvm_cpu_check_pmu(cs, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
> +        return ret;
>      }
>  

Recommend to

>      ret = kvm_cpu_check_pv_features(cs, &local_err);
       if (ret < 0) {
           error_report_err(local_err);
  +        return ret;
       }

  -    return ret;
       return 0;
   }

Why?  Have a look at commit 6edd2a9bec90:

@@ -793,6 +828,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (ret < 0) {
         error_report_err(local_err);
     }
+
+    ret = kvm_cpu_check_pmu(cs, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+
     return ret;
 }

The new call broke the previous call's error handling.  Easy mistake to
make.  Less easy with my version.

With that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

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

[Markus Armbruster]

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,

Re: [PATCH 3/3] target/loongarch: Remove unnecessary temporary variable assignment

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> Temporary variable ret is assigned at last line and return, it can
> be removed and return directly.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 3/3] target/loongarch: Remove unnecessary temporary variable assignment

[Philippe Mathieu-Daudé]

On 13/3/25 10:13, Bibo Mao wrote:
> Temporary variable ret is assigned at last line and return, it can
> be removed and return directly.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   target/loongarch/tcg/tlb_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] target/loongarch: Return directly when detect KVM disabled features

[bibo mao]

On 2025/3/13 下午6:26, Markus Armbruster wrote:
> Suggest something like
> 
>    target/loongarch: Fix error handling of KVM feature checks
> 
> That way, the nature of the patch (it's an error handling bug fix) is
> obvious at a glance.
yeap, will modify title like this.

> 
> Bibo Mao <maobibo@loongson.cn> writes:
> 
>> For some paravirt KVM features, if user forces to enable it however
>> KVM does not support, qemu should fail to run. Here set error message
>> and return directly in function kvm_arch_init_vcpu().
>>
> 
> Please add
> 
>    Fixes: 6edd2a9bec90 (target/loongarch/kvm: Implement LoongArch PMU extension)
>    Fixes: 936c3f4d7916 (target/loongarch: Use auto method with LSX feature)
>    Fixes: 5e360dabedb1 (target/loongarch: Use auto method with LASX feature)
>    Fixes: 620d9bd0022e (target/loongarch: Add paravirt ipi feature detection)
OK, will add these fixes tag.
> 
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   target/loongarch/kvm/kvm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
>> index 28735c80be..b7f370ba97 100644
>> --- a/target/loongarch/kvm/kvm.c
>> +++ b/target/loongarch/kvm/kvm.c
>     int kvm_arch_init_vcpu(CPUState *cs)
>     {
>         uint64_t val;
>         int ret;
>         Error *local_err = NULL;
> 
>         ret = 0;
> 
> Please drop this assignment, it's useless.
Sure, will do.
> 
>         qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs);
> 
>         if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) {
>             brk_insn = val;
>         }
> 
>> @@ -1091,21 +1091,25 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>       ret = kvm_cpu_check_lsx(cs, &local_err);
>>       if (ret < 0) {
>>           error_report_err(local_err);
>> +        return ret;
>>       }
>>   
>>       ret = kvm_cpu_check_lasx(cs, &local_err);
>>       if (ret < 0) {
>>           error_report_err(local_err);
>> +        return ret;
>>       }
>>   
>>       ret = kvm_cpu_check_lbt(cs, &local_err);
>>       if (ret < 0) {
>>           error_report_err(local_err);
>> +        return ret;
>>       }
>>   
>>       ret = kvm_cpu_check_pmu(cs, &local_err);
>>       if (ret < 0) {
>>           error_report_err(local_err);
>> +        return ret;
>>       }
>>   
> 
> Recommend to
> 
>>       ret = kvm_cpu_check_pv_features(cs, &local_err);
>         if (ret < 0) {
>             error_report_err(local_err);
>    +        return ret;
>         }
> 
>    -    return ret;
>         return 0;
>     }
> 
> Why?  Have a look at commit 6edd2a9bec90:
I agree. That is simple and easy to understand, will do like this.

Regards
Bibo Mao
> 
> @@ -793,6 +828,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>       if (ret < 0) {
>           error_report_err(local_err);
>       }
> +
> +    ret = kvm_cpu_check_pmu(cs, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +
>       return ret;
>   }
> 
> The new call broke the previous call's error handling.  Easy mistake to
> make.  Less easy with my version.
> 
> With that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Thanks!
> 

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

[bibo mao]

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,
> 

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

[bibo mao]

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.
> 
>>   
>>   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;
Hi Markus,

 From APIs, it seems that function error_propagate() do much more than 
error appending, such as comparing dest_err with error_abort etc. Though 
caller function is local variable rather than error_abort/fatal/warn, 
error_propagate() seems useful. How about do propagate error and return 
directly as following:

@@ -868,7 +868,8 @@ 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;
          }

Regards
Bibo Mao
>           }
>   
>           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,
> 

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

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/13 下午6:32, Markus Armbruster wrote:

[...]

>> 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;
>
> Hi Markus,
>
>  From APIs, it seems that function error_propagate() do much more than 
> error appending, such as comparing dest_err with error_abort etc. Though 
> caller function is local variable rather than error_abort/fatal/warn, 
> error_propagate() seems useful. How about do propagate error and return 
> directly as following:
>
> @@ -868,7 +868,8 @@ 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;
>          }

This is strictly worse.  One, it's more verbose.  Two, the stack
backtrace on failure is less useful, which matters when @errp is
&error_abort, and when you set a breakpoint on error_handle(), abort(),
or exit().  Three, it doesn't actually add useful functionality.

To help you see the latter, let's compare the two versions, i.e.

   direct: error_setg(errp, ...)

and

   propagate: two steps, first error_setg(&err, ...), and then
   error_propagate(errp, err);

Cases: @errp can be NULL, &error_abort, &error_fatal, &error_warn, or a
non-null pointer to variable containing NULL.

1. @errp is NULL

   Direct does nothing.

   Propagate step 1 creates an error object, and stores it in @err.
   Step 2 frees the error object.  Roundabout way to do nothing.

2. @errp is &error_abort

   Direct creates an error object, reports it to stderr, and abort()s.
   Note that the stack backtrace shows where the error is created.

   Propagate step 1 creates an error object, and stores it in @err.
   Step 2 reports it to stderr, and abort()s.  No difference, except the
   stack backtrace shows where the error is propagated, which is less
   useful.

3. @errp is &error_fatal

   Direct creates an error object, reports it to stderr, frees it, and
   exit(1)s.

   Propagate step 1 creates an error object, and stores it in @err.
   Step 2 reports it to stderr, frees it, and exit(1)s.  No difference.

4. @errp is &error_warn

   Direct creates an error object, reports it to stderr, and frees it.

   Propagate step 1 creates an error object, and stores it in @err.
   Step 2 reports it to stderr, and frees it.  No difference.

5. @errp points to variable containing NULL

   Direct creates an error object, and stores it in the variable.

   Propagate step 1 creates an error object, and stores it in @err.
   Step 2 copies it to the variable.  No difference.

Questions?

[...]

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

[bibo mao]

On 2025/3/14 下午1:38, Markus Armbruster wrote:
> bibo mao <maobibo@loongson.cn> writes:
> 
> On 2025/3/13 下午6:32, Markus Armbruster wrote:
> 
> [...]
> 
>>> 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;
>>
>> Hi Markus,
>>
>>   From APIs, it seems that function error_propagate() do much more than
>> error appending, such as comparing dest_err with error_abort etc. Though
>> caller function is local variable rather than error_abort/fatal/warn,
>> error_propagate() seems useful. How about do propagate error and return
>> directly as following:
>>
>> @@ -868,7 +868,8 @@ 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;
>>           }
> 
> This is strictly worse.  One, it's more verbose.  Two, the stack
> backtrace on failure is less useful, which matters when @errp is
> &error_abort, and when you set a breakpoint on error_handle(), abort(),
> or exit().  Three, it doesn't actually add useful functionality.
> 
> To help you see the latter, let's compare the two versions, i.e.
> 
>     direct: error_setg(errp, ...)
> 
> and
> 
>     propagate: two steps, first error_setg(&err, ...), and then
>     error_propagate(errp, err);
> 
> Cases: @errp can be NULL, &error_abort, &error_fatal, &error_warn, or a
> non-null pointer to variable containing NULL.
> 
> 1. @errp is NULL
> 
>     Direct does nothing.
> 
>     Propagate step 1 creates an error object, and stores it in @err.
>     Step 2 frees the error object.  Roundabout way to do nothing.
> 
> 2. @errp is &error_abort
> 
>     Direct creates an error object, reports it to stderr, and abort()s.
>     Note that the stack backtrace shows where the error is created.
> 
>     Propagate step 1 creates an error object, and stores it in @err.
>     Step 2 reports it to stderr, and abort()s.  No difference, except the
>     stack backtrace shows where the error is propagated, which is less
>     useful.
> 
> 3. @errp is &error_fatal
> 
>     Direct creates an error object, reports it to stderr, frees it, and
>     exit(1)s.
> 
>     Propagate step 1 creates an error object, and stores it in @err.
>     Step 2 reports it to stderr, frees it, and exit(1)s.  No difference.
> 
> 4. @errp is &error_warn
> 
>     Direct creates an error object, reports it to stderr, and frees it.
> 
>     Propagate step 1 creates an error object, and stores it in @err.
>     Step 2 reports it to stderr, and frees it.  No difference.
> 
> 5. @errp points to variable containing NULL
> 
>     Direct creates an error object, and stores it in the variable.
> 
>     Propagate step 1 creates an error object, and stores it in @err.
>     Step 2 copies it to the variable.  No difference.
> 
> Questions?
The question how to use error_propagate() comparing with error_setg() 
since there is such API. :)

Regards
Bibo Mao
> 
> [...]
> 

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

[bibo mao]

On 2025/3/14 下午2:28, bibo mao wrote:
> 
> 
> On 2025/3/14 下午1:38, Markus Armbruster wrote:
>> bibo mao <maobibo@loongson.cn> writes:
>>
>> On 2025/3/13 下午6:32, Markus Armbruster wrote:
>>
>> [...]
>>
>>>> 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;
>>>
>>> Hi Markus,
>>>
>>>   From APIs, it seems that function error_propagate() do much more than
>>> error appending, such as comparing dest_err with error_abort etc. Though
>>> caller function is local variable rather than error_abort/fatal/warn,
>>> error_propagate() seems useful. How about do propagate error and return
>>> directly as following:
>>>
>>> @@ -868,7 +868,8 @@ 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;
>>>           }
>>
>> This is strictly worse.  One, it's more verbose.  Two, the stack
>> backtrace on failure is less useful, which matters when @errp is
>> &error_abort, and when you set a breakpoint on error_handle(), abort(),
>> or exit().  Three, it doesn't actually add useful functionality.
>>
>> To help you see the latter, let's compare the two versions, i.e.
>>
>>     direct: error_setg(errp, ...)
>>
>> and
>>
>>     propagate: two steps, first error_setg(&err, ...), and then
>>     error_propagate(errp, err);
>>
>> Cases: @errp can be NULL, &error_abort, &error_fatal, &error_warn, or a
>> non-null pointer to variable containing NULL.
>>
>> 1. @errp is NULL
>>
>>     Direct does nothing.
>>
>>     Propagate step 1 creates an error object, and stores it in @err.
>>     Step 2 frees the error object.  Roundabout way to do nothing.
>>
>> 2. @errp is &error_abort
>>
>>     Direct creates an error object, reports it to stderr, and abort()s.
>>     Note that the stack backtrace shows where the error is created.
>>
>>     Propagate step 1 creates an error object, and stores it in @err.
>>     Step 2 reports it to stderr, and abort()s.  No difference, except the
>>     stack backtrace shows where the error is propagated, which is less
>>     useful.
>>
>> 3. @errp is &error_fatal
>>
>>     Direct creates an error object, reports it to stderr, frees it, and
>>     exit(1)s.
>>
>>     Propagate step 1 creates an error object, and stores it in @err.
>>     Step 2 reports it to stderr, frees it, and exit(1)s.  No difference.
>>
>> 4. @errp is &error_warn
>>
>>     Direct creates an error object, reports it to stderr, and frees it.
>>
>>     Propagate step 1 creates an error object, and stores it in @err.
>>     Step 2 reports it to stderr, and frees it.  No difference.
>>
>> 5. @errp points to variable containing NULL
>>
>>     Direct creates an error object, and stores it in the variable.
>>
>>     Propagate step 1 creates an error object, and stores it in @err.
>>     Step 2 copies it to the variable.  No difference.
>>
>> Questions?
> The question how to use error_propagate() comparing with error_setg() 
> since there is such API. :)
I know nothing about error module, almost 80% code is copied from others.

 From header file include/qapi/error.h, It seems that error_setg() is 
used in error report leaf function, error_propagate() is used by caller 
node function. Is that right?

Regards
Bibo Mao
> 
> Regards
> Bibo Mao
>>
>> [...]
>>
> 

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

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

> The question how to use error_propagate() comparing with error_setg() since there is such API. :)

error_propagate() should be mostly avoided in new code.  It still exists
because plenty of old code uses it.  It can also be used to keep only
first of several errors, but that's rarely a good idea.

There's usage advice in include/qapi/error.h's big comment.  Relevant
parts for your convenience:

 * = Passing errors around =
 *
 * Errors get passed to the caller through the conventional @errp
 * parameter.
 *
 * Create a new error and pass it to the caller:
 *     error_setg(errp, "situation normal, all fouled up");
 *
 * Call a function, receive an error from it, and pass it to the caller
 * - when the function returns a value that indicates failure, say
 *   false:
 *     if (!foo(arg, errp)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     ERRP_GUARD();
 *     foo(arg, errp);
 *     if (*errp) {
 *         handle the error...
 *     }
 * More on ERRP_GUARD() below.
 *
 * Code predating ERRP_GUARD() still exists, and looks like this:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err); // deprecated
 *     }
 * Avoid in new code.  Do *not* "optimize" it to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL without the ERRP_GUARD() guard.
 *
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.
[...]
 * Pass an existing error to the caller:
 *     error_propagate(errp, err);
 * This is rarely needed.  When @err is a local variable, use of
 * ERRP_GUARD() commonly results in more readable code.
 *
 * Pass an existing error to the caller with the message modified:
 *     error_propagate_prepend(errp, err,
 *                             "Could not frobnicate '%s': ", name);
 * This is more concise than
 *     error_propagate(errp, err); // don't do this
 *     error_prepend(errp, "Could not frobnicate '%s': ", name);
 * and works even when @errp is &error_fatal.
 *
 * Receive and accumulate multiple errors (first one wins):
 *     Error *err = NULL, *local_err = NULL;
 *     foo(arg, &err);
 *     bar(arg, &local_err);
 *     error_propagate(&err, local_err);
 *     if (err) {
 *         handle the error...
 *     }
 *
 * Do *not* "optimize" this to
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     bar(arg, &err); // WRONG!
 *     if (err) {
 *         handle the error...
 *     }
 * because this may pass a non-null err to bar().
 *
 * Likewise, do *not*
 *     Error *err = NULL;
 *     if (cond1) {
 *         error_setg(&err, ...);
 *     }
 *     if (cond2) {
 *         error_setg(&err, ...); // WRONG!
 *     }
 * because this may pass a non-null err to error_setg().

Questions?