bibo mao <maob...@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?
[...]
