Re: [Qemu-devel] spapr-pci: convert init() callback to realize()

[Alexey Kardashevskiy]

On 03/13/2014 12:56 AM, Mike Day wrote:
> On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
>> This converts the old-style init() callback to a new style realize()
>> callback as init() now is supposed to do only trivial initialization.
>>
>> As a part of convertion, this replaces fprintf(stderr) with error_setg()
>> as realize() does not "return" any value, instead it puts the extended
>> error into **errp.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> Reviewed-by: Mike Day <address@hidden>
> 
>> ---
>> Changes:
>> v5:
>> * finish_finalize() moved to a separate patch
>> ---
>>  hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 7763149..aeb012d 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -32,6 +32,7 @@
>>  #include "exec/address-spaces.h"
>>  #include <libfdt.h>
>>  #include "trace.h"
>> +#include "qemu/error-report.h"
>>  
>>  #include "hw/pci/pci_bus.h"
>>  
>> @@ -494,9 +495,9 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, 
>> void *opaque, int devfn)
>>      return &phb->iommu_as;
>>  }
>>  
>> -static int spapr_phb_init(SysBusDevice *s)
>> +static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>  {
>> -    DeviceState *dev = DEVICE(s);
>> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>      const char *busname;
>> @@ -510,9 +511,9 @@ static int spapr_phb_init(SysBusDevice *s)
>>          if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
>>              || (sphb->mem_win_addr != -1)
>>              || (sphb->io_win_addr != -1)) {
>> -            fprintf(stderr, "Either \"index\" or other parameters must"
>> -                    " be specified for PAPR PHB, not both\n");
>> -            return -1;
>> +            error_setg(errp, "Either \"index\" or other parameters must"
>> +                       " be specified for PAPR PHB, not both");
>> +            return;
>>          }
> 
> Seems like these exit clauses should return an integer here and below.


Nope. Bad initfn (which returns int) became good realizefn (which does not
return anything and uses Error** instead) :)

Thanks for review!

>   
>>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>> @@ -525,28 +526,28 @@ static int spapr_phb_init(SysBusDevice *s)
>>      }
>>  
>>      if (sphb->buid == -1) {
>> -        fprintf(stderr, "BUID not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "BUID not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->dma_liobn == -1) {
>> -        fprintf(stderr, "LIOBN not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "LIOBN not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->mem_win_addr == -1) {
>> -        fprintf(stderr, "Memory window address not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "Memory window address not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->io_win_addr == -1) {
>> -        fprintf(stderr, "IO window address not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "IO window address not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (find_phb(spapr, sphb->buid)) {
>> -        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
>> -        return -1;
>> +        error_setg(errp, "PCI host bridges must have unique BUIDs");
>> +        return;
>>      }
>>  
>>      sphb->dtbusname = g_strdup_printf("address@hidden" PRIx64, sphb->buid);
>> @@ -613,8 +614,9 @@ static int spapr_phb_init(SysBusDevice *s)
>>      sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>>                                       sphb->dma_window_size);
>>      if (!sphb->tcet) {
>> -        fprintf(stderr, "Unable to create TCE table for %s\n", 
>> sphb->dtbusname);
>> -        return -1;
>> +        error_setg(errp, "Unable to create TCE table for %s",
>> +                   sphb->dtbusname);
>> +        return;
>>      }
>>      address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>>                         sphb->dtbusname);
>> @@ -631,13 +633,12 @@ static int spapr_phb_init(SysBusDevice *s)
>>  
>>          irq = spapr_allocate_lsi(0);
>>          if (!irq) {
>> -            return -1;
>> +            error_setg(errp, "spapr_allocate_lsi failed");
>> +            return;
>>          }
>>  
>>          sphb->lsi_table[i].irq = irq;
>>      }
>> -
>> -    return 0;
>>  }
>>  
>>  static void spapr_phb_reset(DeviceState *qdev)
>> @@ -720,11 +721,10 @@ static const char 
>> *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>>  {
>>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      hc->root_bus_path = spapr_phb_root_bus_path;
>> -    sdc->init = spapr_phb_init;
>> +    dc->realize = spapr_phb_realize;
>>      dc->props = spapr_phb_properties;
>>      dc->reset = spapr_phb_reset;
>>      dc->vmsd = &vmstate_spapr_pci;
>> -- 
>> 1.8.4.rc4
>>
>>
>>
>>


-- 
Alexey