[Qemu-devel] [PATCH 1/1] target-i386: Clean up misuse of qdev_init() in realize method

[Qemu-devel] [PATCH 1/1] target-i386: Clean up misuse of qdev_init() in realize method

[Markus Armbruster]

x86_cpu_apic_realize() calls qdev_init() to realize the APIC.
qdev_init()'s error handling has unwanted side effects: it unparents
the device, and it calls qerror_report_err().

qerror_report_err() is always inappropriate in realize methods,
because it doesn't return the Error object.  It either reports the
error to stderr or the human monitor, or it stores it in the QMP
monitor, where it makes the QMP command fail even though the realize
method succeeded.

Fortunately, qdev_init() can't actually fail here, because realize
can't fail for any of the three possible APIC device models.

Clean up by cutting out the qdev_init() middle-man: set property
"realized" directly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target-i386/cpu.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a9b32e..2a82407 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2752,12 +2752,8 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
     if (cpu->apic_state == NULL) {
         return;
     }
-
-    if (qdev_init(cpu->apic_state)) {
-        error_setg(errp, "APIC device '%s' could not be initialized",
-                   object_get_typename(OBJECT(cpu->apic_state)));
-        return;
-    }
+    object_property_set_bool(OBJECT(cpu->apic_state), true, "realized",
+                             errp);
 }
 #else
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/1] target-i386: Clean up misuse of qdev_init() in realize method

[Igor Mammedov]

On Thu,  5 Feb 2015 10:29:15 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> x86_cpu_apic_realize() calls qdev_init() to realize the APIC.
> qdev_init()'s error handling has unwanted side effects: it unparents
> the device, and it calls qerror_report_err().
> 
> qerror_report_err() is always inappropriate in realize methods,
> because it doesn't return the Error object.  It either reports the
> error to stderr or the human monitor, or it stores it in the QMP
> monitor, where it makes the QMP command fail even though the realize
> method succeeded.
> 
> Fortunately, qdev_init() can't actually fail here, because realize
> can't fail for any of the three possible APIC device models.
> 
> Clean up by cutting out the qdev_init() middle-man: set property
> "realized" directly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target-i386/cpu.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3a9b32e..2a82407 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2752,12 +2752,8 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>      if (cpu->apic_state == NULL) {
>          return;
>      }
> -
> -    if (qdev_init(cpu->apic_state)) {
> -        error_setg(errp, "APIC device '%s' could not be initialized",
> -                   object_get_typename(OBJECT(cpu->apic_state)));
> -        return;
> -    }
> +    object_property_set_bool(OBJECT(cpu->apic_state), true, "realized",
> +                             errp);
>  }
>  #else
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH 1/1] target-i386: Clean up misuse of qdev_init() in realize method

[Andreas Färber]

Am 05.02.2015 um 13:51 schrieb Igor Mammedov:
> On Thu,  5 Feb 2015 10:29:15 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> x86_cpu_apic_realize() calls qdev_init() to realize the APIC.
>> qdev_init()'s error handling has unwanted side effects: it unparents
>> the device, and it calls qerror_report_err().
>>
>> qerror_report_err() is always inappropriate in realize methods,
>> because it doesn't return the Error object.  It either reports the
>> error to stderr or the human monitor, or it stores it in the QMP
>> monitor, where it makes the QMP command fail even though the realize
>> method succeeded.
>>
>> Fortunately, qdev_init() can't actually fail here, because realize
>> can't fail for any of the three possible APIC device models.
>>
>> Clean up by cutting out the qdev_init() middle-man: set property
>> "realized" directly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)