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.3Bibo 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,
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,
>
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?
[...]
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
>
> [...]
>
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?
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
>>
>> [...]
>>
>
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,
>
© 2016 - 2025 Red Hat, Inc.