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 <address@hidden> 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 <address@hidden>
> Cc: Scott Wood <address@hidden>
> Cc: address@hidden
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  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);
>          }
>      }