[Qemu-devel] Error handling in cpu_x86_create

[Qemu-devel] Error handling in cpu_x86_create

[Jan Kiszka]

Hi Igor,

just noticed by chance that error handling in cpu_x86_create is likely
broken after your changes. Any error after cpu = ...object_new() will
not properly release the CPU object again nor report NULL to the caller.
That means errors should slip through, no? And cpu_x86_init looks similar.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Error handling in cpu_x86_create

[Igor Mammedov]

On Tue, 30 Jul 2013 13:00:40 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Hi Igor,
> 
> just noticed by chance that error handling in cpu_x86_create is likely
> broken after your changes. Any error after cpu = ...object_new() will
> not properly release the CPU object again nor report NULL to the caller.
> That means errors should slip through, no? And cpu_x86_init looks similar.
Failure of object_new() in cpu_x86_create() is not checked since it should
never fail.

As for following error handling caller of cpu_x86_create(..., errp) should
use errp to check for errors and release failed cpu.

cpu_x86_init() that calls it has:
===
out:
    if (error) {
        fprintf(stderr, "%s\n", error_get_pretty(error));
        error_free(error);
        if (cpu != NULL) {
            object_unref(OBJECT(cpu));
===
similar code-path exists in pc_new_cpu()

end goal is to replace cpu_x86_create() with just object_new(FOO_CPU),
but that needs to x86 CPU subclasses and properties in place so
we could remove/isolate legacy stuff to legacy hook.

Did you hit any particular problem with current code?

> 
> Jan
> 

Re: [Qemu-devel] Error handling in cpu_x86_create

[Jan Kiszka]

On 2013-07-30 14:21, Igor Mammedov wrote:
> On Tue, 30 Jul 2013 13:00:40 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Hi Igor,
>>
>> just noticed by chance that error handling in cpu_x86_create is likely
>> broken after your changes. Any error after cpu = ...object_new() will
>> not properly release the CPU object again nor report NULL to the caller.
>> That means errors should slip through, no? And cpu_x86_init looks similar.
> Failure of object_new() in cpu_x86_create() is not checked since it should
> never fail.
> 
> As for following error handling caller of cpu_x86_create(..., errp) should
> use errp to check for errors and release failed cpu.
> 
> cpu_x86_init() that calls it has:
> ===
> out:
>     if (error) {
>         fprintf(stderr, "%s\n", error_get_pretty(error));
>         error_free(error);
>         if (cpu != NULL) {
>             object_unref(OBJECT(cpu));
> ===
> similar code-path exists in pc_new_cpu()

Not truly:

    cpu = cpu_x86_create(cpu_model, icc_bridge, errp);
    if (!cpu) {
        return cpu;
    }

    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);

    if (local_err) {
        if (cpu != NULL) {
            object_unref(OBJECT(cpu));
            cpu = NULL;
        }


That's what made me think the error is supposed to be derived from cpu
== NULL, rather than from errp. So the actual uncleanness is here: errp
should be checked instead of cpu, right?

Jan

> 
> end goal is to replace cpu_x86_create() with just object_new(FOO_CPU),
> but that needs to x86 CPU subclasses and properties in place so
> we could remove/isolate legacy stuff to legacy hook.
> 
> Did you hit any particular problem with current code?
> 
>>
>> Jan
>>
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH for-1.6] target-i386: Fix X86CPU error handling

[Andreas Färber]

Error **errp argument is not for emitting warnings, it means an error
has occurred and the caller should not make any assumptions about the
state of other return values (unless otherwise documented).

Therefore cpu_x86_create() must unref the new X86CPU itself, and
pc_new_cpu() must check for an Error rather than NULL return value.

While at it, clean up a superfluous NULL check.

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Cc: qemu-stable@nongnu.org
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/pc.c      | 13 ++++++-------
 target-i386/cpu.c |  6 +++++-
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a2b9d88..6a0b042 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -912,20 +912,19 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
     X86CPU *cpu;
     Error *local_err = NULL;
 
-    cpu = cpu_x86_create(cpu_model, icc_bridge, errp);
-    if (!cpu) {
-        return cpu;
+    cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return NULL;
     }
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
     if (local_err) {
-        if (cpu != NULL) {
-            object_unref(OBJECT(cpu));
-            cpu = NULL;
-        }
         error_propagate(errp, local_err);
+        object_unref(OBJECT(cpu));
+        cpu = NULL;
     }
     return cpu;
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 71ab915..2efbeca 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1824,7 +1824,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     }
 
 out:
-    error_propagate(errp, error);
+    if (error != NULL) {
+        error_propagate(errp, error);
+        object_unref(OBJECT(cpu));
+        cpu = NULL;
+    }
     g_strfreev(model_pieces);
     return cpu;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.6] target-i386: Fix X86CPU error handling

[Andreas Färber]

Am 02.08.2013 19:22, schrieb Andreas Färber:
> Error **errp argument is not for emitting warnings, it means an error
> has occurred and the caller should not make any assumptions about the
> state of other return values (unless otherwise documented).
> 
> Therefore cpu_x86_create() must unref the new X86CPU itself, and
> pc_new_cpu() must check for an Error rather than NULL return value.
> 
> While at it, clean up a superfluous NULL check.
> 
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: qemu-stable@nongnu.org
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Ping! Jan, does this address your concerns / use cases?
-rc2 is on Wednesday.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6] target-i386: Fix X86CPU error handling

[Jan Kiszka]

On 2013-08-05 14:06, Andreas Färber wrote:
> Am 02.08.2013 19:22, schrieb Andreas Färber:
>> Error **errp argument is not for emitting warnings, it means an error
>> has occurred and the caller should not make any assumptions about the
>> state of other return values (unless otherwise documented).
>>
>> Therefore cpu_x86_create() must unref the new X86CPU itself, and
>> pc_new_cpu() must check for an Error rather than NULL return value.
>>
>> While at it, clean up a superfluous NULL check.
>>
>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Cc: qemu-stable@nongnu.org
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Ping! Jan, does this address your concerns / use cases?
> -rc2 is on Wednesday.

Yes, thanks, this looks good.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux