Message ID | 1423128889-18260-3-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
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;
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
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.
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;
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(-)