Re: [Qemu-devel] [PATCH 4/6] xics: minor changes and cleanups

["Andreas Färber" <afaerber@suse.de>]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
> 
> This does:
> 1. add assert in ics_realize()
> 2. change variable names from "k" to more informative ones
> 3. add "const" to every TypeInfo
> 4. replace fprintf(stderr, ..."\n") with error_report
> 5. replace old style qdev_init_nofail() with new style
> object_property_set_bool().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/intc/xics.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 436c788..20840e3 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -29,6 +29,7 @@
>  #include "trace.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "qemu/error-report.h"
>  
>  /*
>   * ICP: Presentation layer
> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vmstate_icp_server;
>  }
>  
> -static TypeInfo icp_info = {
> +static const TypeInfo icp_info = {
>      .name = TYPE_ICP,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICPState),
> @@ -446,6 +447,7 @@ static int ics_realize(DeviceState *dev)
>  {
>      ICSState *ics = ICS(dev);
>  
> +    assert(ics->nr_irqs);

Why assert here? As pointed out, this function is bogus, it should have
an Error **errp argument, so you should just do something like this:

    if (ics->nr_irqs == 0) {
        error_setg(errp, "Number of interrupts needs to be greater 0");
        return;
    }

which propagates the error to the caller and lets him decide how to
handle it, such as actually specifying a different property value and
trying realized = true again.

>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>      ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>      ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
> @@ -456,15 +458,15 @@ static int ics_realize(DeviceState *dev)
>  static void ics_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    ICSStateClass *k = ICS_CLASS(klass);
> +    ICSStateClass *icsc = ICS_CLASS(klass);
>  
>      dc->init = ics_realize;
>      dc->vmsd = &vmstate_ics;
>      dc->reset = ics_reset;
> -    k->post_load = ics_post_load;
> +    icsc->post_load = ics_post_load;
>  }
>  
> -static TypeInfo ics_info = {
> +static const TypeInfo ics_info = {
>      .name = TYPE_ICS,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICSState),
> @@ -680,8 +682,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>          break;
>  
>      default:
> -        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> -                "bus model\n");
> +        error_report("XICS interrupt controller does not support this CPU "
> +                "bus model");
>          abort();
>      }
>  }
> @@ -690,6 +692,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *icp = XICS(dev);
>      ICSState *ics = icp->ics;
> +    Error *error = NULL;
>      int i;
>  
>      /* Registration of global state belongs into realize */
> @@ -706,15 +709,24 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      ics->nr_irqs = icp->nr_irqs;
>      ics->offset = XICS_IRQ_BASE;
>      ics->icp = icp;
> -    qdev_init_nofail(DEVICE(ics));
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
>  
> +    assert(icp->nr_servers);

Ditto here - this one already has errp.

Otherwise good, thanks.

Andreas

>      icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>      for (i = 0; i < icp->nr_servers; i++) {
>          char buffer[32];
>          object_initialize(&icp->ss[i], TYPE_ICP);
>          snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>          object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> -        qdev_init_nofail(DEVICE(&icp->ss[i]));
> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            return;
> +        }
>      }
>  }
>  
> @@ -735,12 +747,12 @@ static Property xics_properties[] = {
>  static void xics_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> -    XICSStateClass *k = XICS_CLASS(oc);
> +    XICSStateClass *xsc = XICS_CLASS(oc);
>  
>      dc->realize = xics_realize;
>      dc->props = xics_properties;
>      dc->reset = xics_reset;
> -    k->cpu_setup = xics_cpu_setup;
> +    xsc->cpu_setup = xics_cpu_setup;
>  }
>  
>  static const TypeInfo xics_info = {

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