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

[Markus Armbruster <armbru@redhat.com>]

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?

[...]