Re: [Qemu-devel] [PATCH RFC 01/10] pci: Convert core to realize

[Marcel Apfelbaum]

On Tue, 2014-10-28 at 10:38 +0100, Markus Armbruster wrote:
> Gonglei <address@hidden> writes:
> 
> > On 2014/10/28 15:35, Markus Armbruster wrote:
> >
> >> Implement DeviceClass methods realize() and unrealize() instead of
> >> init() and exit().  The core's initialization errors now get
> >> propagated properly, and QMP sends them instead of an unspecific
> >> "Device initialization failed" error.  Unrealize can't fail, so no
> >> change there.
> >> 
> >> PCIDeviceClass is unchanged: it still provides init() and exit().
> >> Therefore, device models' errors are still not propagated.
> >> 
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> ---
> >>  hw/pci/pci.c | 91 
> >> +++++++++++++++++++++++++++++++-----------------------------
> >>  1 file changed, 47 insertions(+), 44 deletions(-)
> >> 
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index cd7a403..aef95c3 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> [...]
> >>  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> >> @@ -1742,10 +1747,11 @@ PCIDevice *pci_find_device(PCIBus *bus, int 
> >> bus_num, uint8_t devfn)
> >>      return bus->devices[devfn];
> >>  }
> >>  
> >> -static int pci_qdev_init(DeviceState *qdev)
> >> +static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >>  {
> >>      PCIDevice *pci_dev = (PCIDevice *)qdev;
> >>      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> >> +    Error *local_err = NULL;
> >>      PCIBus *bus;
> >>      int rc;
> >>      bool is_default_rom;
> >> @@ -1758,15 +1764,16 @@ static int pci_qdev_init(DeviceState *qdev)
> >>      bus = PCI_BUS(qdev_get_parent_bus(qdev));
> >>      pci_dev = do_pci_register_device(pci_dev, bus,
> >>                                       object_get_typename(OBJECT(qdev)),
> >> -                                     pci_dev->devfn);
> >> +                                     pci_dev->devfn, errp);
> >>      if (pci_dev == NULL)
> >
> >> -        return -1;
> >> +        return;
> >>  
> >
> > Maybe we can use '{}' for if statement follow Qemu's coding style.
I suggest adding a trivial patch on top of Markus's series

Thanks,
Marcel

> 
> scripts/checkpatch.pl is happy with the patch as is.
> 
> I prefer to add braces only when I touch the conditional.
> 
> Naturally, I also add them when a maintainer asks me to :)
> 
> >>      if (pc->init) {
> >>          rc = pc->init(pci_dev);
> >>          if (rc != 0) {
> >>              do_pci_unregister_device(pci_dev);
> >> -            return rc;
> >> +            error_setg(errp, "Device initialization failed");
> >> +            return;
> >>          }
> >>      }
> >>  
> [...]
> 
> Thanks!
>