[Qemu-devel] [PATCH 0/3] Clean up misuse of qdev_init() in interrupt controller creation

[Qemu-devel] [PATCH 0/3] Clean up misuse of qdev_init() in interrupt controller creation

[Markus Armbruster]

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

Unparenting is wanted in realize methods only when we do unusual
things like trying a set of devices and keeping the first one that
works.  This is the case in PATCH 1+2.

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, none of the realize methods in this
patch are reachable from QMP.

Markus Armbruster (3):
  PPC: Clean up misuse of qdev_init() in kvm-openpic creation
  spapr: Clean up misuse of qdev_init() in xics-kvm creation
  s390x: Replace unchecked qdev_init() by qdev_init_nofail()

 hw/intc/s390_flic.c |  6 +-----
 hw/ppc/e500.c       | 20 +++++++++++---------
 hw/ppc/spapr.c      | 25 ++++++++++++-------------
 3 files changed, 24 insertions(+), 27 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] PPC: Clean up misuse of qdev_init() in kvm-openpic creation

[Markus Armbruster]

We call ppce500_init_mpic_kvm() to create a "kvm-openpic".  If it
fails, we call ppce500_init_mpic_qemu() to fall back to plain
"openpic".

ppce500_init_mpic_kvm() uses qdev_init().  qdev_init()'s error
handling has an unwanted side effect: it calls qerror_report_err(),
which prints to stderr.  Looks like an error, but isn't.

In QMP context, it would stash the error in the monitor instead,
making the QMP command fail.  Fortunately, it's only called from board
initialization, never in QMP context.

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

While there, improve the error message when we can't satisfy an
explicit user request for "kvm-openpic", and exit(1) instead of
abort().

Cc: Alexander Graf <agraf@suse.de>
Cc: Scott Wood <scottwood@freescale.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/e500.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 7e17d18..fd0d138 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -706,17 +706,19 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
 }
 
 static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
-                                          qemu_irq **irqs)
+                                          qemu_irq **irqs, Error **errp)
 {
+    Error *err = NULL;
     DeviceState *dev;
     CPUState *cs;
-    int r;
 
     dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
     qdev_prop_set_uint32(dev, "model", params->mpic_version);
 
-    r = qdev_init(dev);
-    if (r) {
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        object_unparent(OBJECT(dev));
         return NULL;
     }
 
@@ -747,15 +749,15 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
                                                 "kernel_irqchip", true);
         bool irqchip_required = qemu_opt_get_bool(machine_opts,
                                                   "kernel_irqchip", false);
+        Error *err = NULL;
 
         if (irqchip_allowed) {
-            dev = ppce500_init_mpic_kvm(params, irqs);
+            dev = ppce500_init_mpic_kvm(params, irqs, &err);
         }
-
         if (irqchip_required && !dev) {
-            fprintf(stderr, "%s: irqchip requested but unavailable\n",
-                    __func__);
-            abort();
+            error_report("kernel_irqchip requested but unavailable: %s",
+                         error_get_pretty(err));
+            exit(1);
         }
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] spapr: Clean up misuse of qdev_init() in xics-kvm creation

[Markus Armbruster]

We call try_create_xics() to create a "xics-kvm".  If it fails, we
call it again to fall back to plain "xics".

try_create_xics() uses qdev_init().  qdev_init()'s error handling has
an unwanted side effect: it calls qerror_report_err(), which prints to
stderr.  Looks like an error, but isn't.

In QMP context, it would stash the error in the monitor instead,
making the QMP command fail.  Fortunately, it's only called from board
initialization, never in QMP context.

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

While there, improve the error message when we can't satisfy an
explicit user request for "xics-kvm", and exit(1) instead of abort().
Simplify the abort when we can't create "xics".

Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..de6d0bb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -109,17 +109,20 @@ struct sPAPRMachineState {
 sPAPREnvironment *spapr;
 
 static XICSState *try_create_xics(const char *type, int nr_servers,
-                                  int nr_irqs)
+                                  int nr_irqs, Error **errp)
 {
+    Error *err;
     DeviceState *dev;
 
     dev = qdev_create(NULL, type);
     qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
     qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
-    if (qdev_init(dev) < 0) {
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        object_unparent(OBJECT(dev));
         return NULL;
     }
-
     return XICS_COMMON(dev);
 }
 
@@ -133,23 +136,19 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
                                                 "kernel_irqchip", true);
         bool irqchip_required = qemu_opt_get_bool(machine_opts,
                                                   "kernel_irqchip", false);
+        Error *err = NULL;
+
         if (irqchip_allowed) {
-            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
+            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs, &err);
         }
-
         if (irqchip_required && !icp) {
-            perror("Failed to create in-kernel XICS\n");
-            abort();
+            error_report("kernel_irqchip requested but unavailable: %s",
+                         error_get_pretty(err));
         }
     }
 
     if (!icp) {
-        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
-    }
-
-    if (!icp) {
-        perror("Failed to create XICS\n");
-        abort();
+        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
     }
 
     return icp;
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()

[Markus Armbruster]

s390_flic_init() is a helper to create and realize either
"s390-flic-kvm" or "s390-flic-qemu".  When qdev_init() fails, it
complains to stderr and succeeds.

Except it can't actually fail, because the "s390-flic-qemu" is a dummy
without a realize method, and "s390-flic-kvm"'s realize can't fail,
even when the kernel device is really unavailable.  Odd.

Replace qdev_init() by qdev_init_nofail() to make "can't fail" locally
obvious, and get rid of the unreachable error reporting.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/intc/s390_flic.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index 03c5e89..02e10b7 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -30,7 +30,6 @@ S390FLICState *s390_get_flic(void)
 void s390_flic_init(void)
 {
     DeviceState *dev;
-    int r;
 
     dev = s390_flic_kvm_create();
     if (!dev) {
@@ -38,10 +37,7 @@ void s390_flic_init(void)
         object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC,
                                   OBJECT(dev), NULL);
     }
-    r = qdev_init(dev);
-    if (r) {
-        error_report("flic: couldn't create qdev");
-    }
+    qdev_init_nofail(dev);
 }
 
 static int qemu_s390_register_io_adapter(S390FLICState *fs, uint32_t id,
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] spapr: Clean up misuse of qdev_init() in xics-kvm creation

[Markus Armbruster]

David, your commit 11ad93f suggests you're highly qualified to review.
No good deed shall go unpunished ;)

Markus Armbruster <armbru@redhat.com> writes:

> We call try_create_xics() to create a "xics-kvm".  If it fails, we
> call it again to fall back to plain "xics".
>
> try_create_xics() uses qdev_init().  qdev_init()'s error handling has
> an unwanted side effect: it calls qerror_report_err(), which prints to
> stderr.  Looks like an error, but isn't.
>
> In QMP context, it would stash the error in the monitor instead,
> making the QMP command fail.  Fortunately, it's only called from board
> initialization, never in QMP context.
>
> Clean up by cutting out the qdev_init() middle-man: set property
> "realized" directly.
>
> While there, improve the error message when we can't satisfy an
> explicit user request for "xics-kvm", and exit(1) instead of abort().
> Simplify the abort when we can't create "xics".
>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b560459..de6d0bb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -109,17 +109,20 @@ struct sPAPRMachineState {
>  sPAPREnvironment *spapr;
>  
>  static XICSState *try_create_xics(const char *type, int nr_servers,
> -                                  int nr_irqs)
> +                                  int nr_irqs, Error **errp)
>  {
> +    Error *err;
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, type);
>      qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>      qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
> -    if (qdev_init(dev) < 0) {
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        object_unparent(OBJECT(dev));
>          return NULL;
>      }
> -
>      return XICS_COMMON(dev);
>  }
>  
> @@ -133,23 +136,19 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>                                                  "kernel_irqchip", true);
>          bool irqchip_required = qemu_opt_get_bool(machine_opts,
>                                                    "kernel_irqchip", false);
> +        Error *err = NULL;
> +
>          if (irqchip_allowed) {
> -            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs, &err);
>          }
> -
>          if (irqchip_required && !icp) {
> -            perror("Failed to create in-kernel XICS\n");
> -            abort();
> +            error_report("kernel_irqchip requested but unavailable: %s",
> +                         error_get_pretty(err));
>          }
>      }
>  
>      if (!icp) {
> -        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> -    }
> -
> -    if (!icp) {
> -        perror("Failed to create XICS\n");
> -        abort();
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
>      }
>  
>      return icp;

Re: [Qemu-devel] [PATCH 1/3] PPC: Clean up misuse of qdev_init() in kvm-openpic creation

[Markus Armbruster]

Scott, can you review?

Markus Armbruster <armbru@redhat.com> writes:

> We call ppce500_init_mpic_kvm() to create a "kvm-openpic".  If it
> fails, we call ppce500_init_mpic_qemu() to fall back to plain
> "openpic".
>
> ppce500_init_mpic_kvm() uses qdev_init().  qdev_init()'s error
> handling has an unwanted side effect: it calls qerror_report_err(),
> which prints to stderr.  Looks like an error, but isn't.
>
> In QMP context, it would stash the error in the monitor instead,
> making the QMP command fail.  Fortunately, it's only called from board
> initialization, never in QMP context.
>
> Clean up by cutting out the qdev_init() middle-man: set property
> "realized" directly.
>
> While there, improve the error message when we can't satisfy an
> explicit user request for "kvm-openpic", and exit(1) instead of
> abort().
>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/e500.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 7e17d18..fd0d138 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -706,17 +706,19 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
>  }
>  
>  static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
> -                                          qemu_irq **irqs)
> +                                          qemu_irq **irqs, Error **errp)
>  {
> +    Error *err = NULL;
>      DeviceState *dev;
>      CPUState *cs;
> -    int r;
>  
>      dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
>      qdev_prop_set_uint32(dev, "model", params->mpic_version);
>  
> -    r = qdev_init(dev);
> -    if (r) {
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        object_unparent(OBJECT(dev));
>          return NULL;
>      }
>  
> @@ -747,15 +749,15 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>                                                  "kernel_irqchip", true);
>          bool irqchip_required = qemu_opt_get_bool(machine_opts,
>                                                    "kernel_irqchip", false);
> +        Error *err = NULL;
>  
>          if (irqchip_allowed) {
> -            dev = ppce500_init_mpic_kvm(params, irqs);
> +            dev = ppce500_init_mpic_kvm(params, irqs, &err);
>          }
> -
>          if (irqchip_required && !dev) {
> -            fprintf(stderr, "%s: irqchip requested but unavailable\n",
> -                    __func__);
> -            abort();
> +            error_report("kernel_irqchip requested but unavailable: %s",
> +                         error_get_pretty(err));
> +            exit(1);
>          }
>      }

Re: [Qemu-devel] [PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()

[Markus Armbruster]

Connie, your commit 7b35d0c suggests you're highly qualified to review.
No good deed shall go unpunished ;)

Markus Armbruster <armbru@redhat.com> writes:

> s390_flic_init() is a helper to create and realize either
> "s390-flic-kvm" or "s390-flic-qemu".  When qdev_init() fails, it
> complains to stderr and succeeds.
>
> Except it can't actually fail, because the "s390-flic-qemu" is a dummy
> without a realize method, and "s390-flic-kvm"'s realize can't fail,
> even when the kernel device is really unavailable.  Odd.
>
> Replace qdev_init() by qdev_init_nofail() to make "can't fail" locally
> obvious, and get rid of the unreachable error reporting.
>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/intc/s390_flic.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index 03c5e89..02e10b7 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -30,7 +30,6 @@ S390FLICState *s390_get_flic(void)
>  void s390_flic_init(void)
>  {
>      DeviceState *dev;
> -    int r;
>  
>      dev = s390_flic_kvm_create();
>      if (!dev) {
> @@ -38,10 +37,7 @@ void s390_flic_init(void)
>          object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC,
>                                    OBJECT(dev), NULL);
>      }
> -    r = qdev_init(dev);
> -    if (r) {
> -        error_report("flic: couldn't create qdev");
> -    }
> +    qdev_init_nofail(dev);
>  }
>  
>  static int qemu_s390_register_io_adapter(S390FLICState *fs, uint32_t id,

Re: [Qemu-devel] [PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()

[Cornelia Huck]

On Wed, 18 Feb 2015 15:45:01 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Connie, your commit 7b35d0c suggests you're highly qualified to review.
> No good deed shall go unpunished ;)

Yeah, it has been on my todo list...

> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > s390_flic_init() is a helper to create and realize either
> > "s390-flic-kvm" or "s390-flic-qemu".  When qdev_init() fails, it
> > complains to stderr and succeeds.
> >
> > Except it can't actually fail, because the "s390-flic-qemu" is a dummy
> > without a realize method, and "s390-flic-kvm"'s realize can't fail,
> > even when the kernel device is really unavailable.  Odd.

Yes, this _is_ odd. I'd say we'd want to die if we can't instatiate the
kvm-flic, but at least for injecting interrupts we have a fallback to
the old method (but looking at the code, this does not quite work out
as I thought it would). The qemu-flic is really just a dummy.

> >
> > Replace qdev_init() by qdev_init_nofail() to make "can't fail" locally
> > obvious, and get rid of the unreachable error reporting.
> >
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/intc/s390_flic.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > index 03c5e89..02e10b7 100644
> > --- a/hw/intc/s390_flic.c
> > +++ b/hw/intc/s390_flic.c
> > @@ -30,7 +30,6 @@ S390FLICState *s390_get_flic(void)
> >  void s390_flic_init(void)
> >  {
> >      DeviceState *dev;
> > -    int r;
> >  
> >      dev = s390_flic_kvm_create();
> >      if (!dev) {
> > @@ -38,10 +37,7 @@ void s390_flic_init(void)
> >          object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC,
> >                                    OBJECT(dev), NULL);
> >      }
> > -    r = qdev_init(dev);
> > -    if (r) {
> > -        error_report("flic: couldn't create qdev");
> > -    }
> > +    qdev_init_nofail(dev);
> >  }
> >  
> >  static int qemu_s390_register_io_adapter(S390FLICState *fs, uint32_t id,

I think this is OK for the time being. We need to look into the various
weirdnesses for the kvm-flic, though.

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 2/3] spapr: Clean up misuse of qdev_init() in xics-kvm creation

[Alexander Graf]

On 05.02.15 10:34, Markus Armbruster wrote:
> We call try_create_xics() to create a "xics-kvm".  If it fails, we
> call it again to fall back to plain "xics".
> 
> try_create_xics() uses qdev_init().  qdev_init()'s error handling has
> an unwanted side effect: it calls qerror_report_err(), which prints to
> stderr.  Looks like an error, but isn't.
> 
> In QMP context, it would stash the error in the monitor instead,
> making the QMP command fail.  Fortunately, it's only called from board
> initialization, never in QMP context.
> 
> Clean up by cutting out the qdev_init() middle-man: set property
> "realized" directly.
> 
> While there, improve the error message when we can't satisfy an
> explicit user request for "xics-kvm", and exit(1) instead of abort().
> Simplify the abort when we can't create "xics".
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks, applied patches 1 and 2 to ppc-next.


Alex

Re: [Qemu-devel] [PATCH 2/3] spapr: Clean up misuse of qdev_init() in xics-kvm creation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Wed, Feb 18, 2015 at 03:42:44PM +0100, Markus Armbruster wrote:
> David, your commit 11ad93f suggests you're highly qualified to review.
> No good deed shall go unpunished ;)

Sorry, I somehow missed this one.

Reivewed-by: David Gibson <david@gibson.dropbear.id.au>

Though apparently Alex has merged it already, so I guess it's no
longer relevant.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/3] PPC: Clean up misuse of qdev_init() in kvm-openpic creation

[Scott Wood]

On Wed, 2015-02-18 at 15:43 +0100, Markus Armbruster wrote:
> Scott, can you review?
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > We call ppce500_init_mpic_kvm() to create a "kvm-openpic".  If it
> > fails, we call ppce500_init_mpic_qemu() to fall back to plain
> > "openpic".
> >
> > ppce500_init_mpic_kvm() uses qdev_init().  qdev_init()'s error
> > handling has an unwanted side effect: it calls qerror_report_err(),
> > which prints to stderr.  Looks like an error, but isn't.
> >
> > In QMP context, it would stash the error in the monitor instead,
> > making the QMP command fail.  Fortunately, it's only called from board
> > initialization, never in QMP context.
> >
> > Clean up by cutting out the qdev_init() middle-man: set property
> > "realized" directly.
> >
> > While there, improve the error message when we can't satisfy an
> > explicit user request for "kvm-openpic", and exit(1) instead of
> > abort().

I'm OK with this if setting the realized property directly is considered
good practice, but if we're not supposed to call qdev_init() in cases
where it could legitimately fail, why is it distinct from
qdev_init_nofail()?

-Scott

Re: [Qemu-devel] [PATCH 1/3] PPC: Clean up misuse of qdev_init() in kvm-openpic creation

[Markus Armbruster]

Scott Wood <scottwood@freescale.com> writes:

> On Wed, 2015-02-18 at 15:43 +0100, Markus Armbruster wrote:
>> Scott, can you review?
>> 
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > We call ppce500_init_mpic_kvm() to create a "kvm-openpic".  If it
>> > fails, we call ppce500_init_mpic_qemu() to fall back to plain
>> > "openpic".
>> >
>> > ppce500_init_mpic_kvm() uses qdev_init().  qdev_init()'s error
>> > handling has an unwanted side effect: it calls qerror_report_err(),
>> > which prints to stderr.  Looks like an error, but isn't.
>> >
>> > In QMP context, it would stash the error in the monitor instead,
>> > making the QMP command fail.  Fortunately, it's only called from board
>> > initialization, never in QMP context.
>> >
>> > Clean up by cutting out the qdev_init() middle-man: set property
>> > "realized" directly.
>> >
>> > While there, improve the error message when we can't satisfy an
>> > explicit user request for "kvm-openpic", and exit(1) instead of
>> > abort().
>
> I'm OK with this if setting the realized property directly is considered
> good practice,

I'm following precedence set by new code, and I double-checked with
Andreas to make sure it's how he wants it done.

>                but if we're not supposed to call qdev_init() in cases
> where it could legitimately fail, why is it distinct from
> qdev_init_nofail()?

Fair question!

Both qdev_init() and qdev_init_nofail() are deprecated according to
qdev-core.h:

 * As an interim step, the #DeviceState:realized property is set by deprecated
 * functions qdev_init() and qdev_init_nofail().

qdev_init() needs to go away, because it fundamentally relies on
qerror_report() to get the errors out, and that one needs to go away,
because it doesn't coexist nicely with the newer Error API.

I'm working towards getting rid of both, and this patch is part of the
effort.

I'm not working towards getting rid of qdev_init_nofail().  As long as
board code has to realize devices, having a helper that exits when
realization fails makes sense.  It may make sense to rename it once
qdev_init() is gone.

Re: [Qemu-devel] [PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()

[Cornelia Huck]

On Wed, 18 Feb 2015 16:31:28 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> > Markus Armbruster <armbru@redhat.com> writes:
> > 
> > > s390_flic_init() is a helper to create and realize either
> > > "s390-flic-kvm" or "s390-flic-qemu".  When qdev_init() fails, it
> > > complains to stderr and succeeds.
> > >
> > > Except it can't actually fail, because the "s390-flic-qemu" is a dummy
> > > without a realize method, and "s390-flic-kvm"'s realize can't fail,
> > > even when the kernel device is really unavailable.  Odd.
> 
> Yes, this _is_ odd. I'd say we'd want to die if we can't instatiate the
> kvm-flic, but at least for injecting interrupts we have a fallback to
> the old method (but looking at the code, this does not quite work out
> as I thought it would). The qemu-flic is really just a dummy.
> 
> > >
> > > Replace qdev_init() by qdev_init_nofail() to make "can't fail" locally
> > > obvious, and get rid of the unreachable error reporting.
> > >
> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > ---
> > >  hw/intc/s390_flic.c | 6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > >
> > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > > index 03c5e89..02e10b7 100644
> > > --- a/hw/intc/s390_flic.c
> > > +++ b/hw/intc/s390_flic.c
> > > @@ -30,7 +30,6 @@ S390FLICState *s390_get_flic(void)
> > >  void s390_flic_init(void)
> > >  {
> > >      DeviceState *dev;
> > > -    int r;
> > >  
> > >      dev = s390_flic_kvm_create();
> > >      if (!dev) {
> > > @@ -38,10 +37,7 @@ void s390_flic_init(void)
> > >          object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC,
> > >                                    OBJECT(dev), NULL);
> > >      }
> > > -    r = qdev_init(dev);
> > > -    if (r) {
> > > -        error_report("flic: couldn't create qdev");
> > > -    }
> > > +    qdev_init_nofail(dev);
> > >  }
> > >  
> > >  static int qemu_s390_register_io_adapter(S390FLICState *fs, uint32_t id,
> 
> I think this is OK for the time being. We need to look into the various
> weirdnesses for the kvm-flic, though.
> 
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Applied to my s390-next tree.