Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] libqos: add PCI management in qtest_vboot()/qtest_shutdown()

[David Gibson]

On Tue, Sep 27, 2016 at 09:33:58AM +0200, Laurent Vivier wrote:
> 
> 
> On 27/09/2016 05:48, David Gibson wrote:
> > On Mon, Sep 26, 2016 at 04:10:47PM +0200, Laurent Vivier wrote:
> >> Signed-off-by: Laurent Vivier <address@hidden>
> >> ---
> >>  tests/e1000e-test.c         |  2 +-
> >>  tests/i440fx-test.c         |  2 +-
> >>  tests/ide-test.c            |  2 +-
> >>  tests/ivshmem-test.c        |  2 +-
> >>  tests/libqos/ahci.c         |  2 +-
> >>  tests/libqos/libqos-pc.c    |  5 ++++-
> >>  tests/libqos/libqos-spapr.c |  5 ++++-
> >>  tests/libqos/libqos.c       | 21 ++++++++++++++++-----
> >>  tests/libqos/libqos.h       |  3 +++
> >>  tests/libqos/pci-pc.c       |  2 +-
> >>  tests/libqos/pci-pc.h       |  3 ++-
> >>  tests/q35-test.c            |  2 +-
> >>  tests/rtl8139-test.c        |  2 +-
> >>  tests/tco-test.c            |  2 +-
> >>  tests/usb-hcd-ehci-test.c   |  2 +-
> >>  tests/usb-hcd-uhci-test.c   |  2 +-
> >>  tests/vhost-user-test.c     |  2 +-
> >>  tests/virtio-9p-test.c      |  2 +-
> >>  tests/virtio-blk-test.c     |  2 +-
> >>  tests/virtio-net-test.c     |  2 +-
> >>  tests/virtio-scsi-test.c    |  2 +-
> >>  21 files changed, 45 insertions(+), 24 deletions(-)
> > 
> > Couple of queries below.
> > 
> 
> ...
> 
> >> @@ -49,9 +54,15 @@ QOSState *qtest_boot(QOSOps *ops, const char 
> >> *cmdline_fmt, ...)
> >>   */
> >>  void qtest_shutdown(QOSState *qs)
> >>  {
> >> -    if (qs->alloc && qs->ops && qs->ops->uninit_allocator) {
> >> -        qs->ops->uninit_allocator(qs->alloc);
> >> -        qs->alloc = NULL;
> >> +    if (qs->ops) {
> >> +        if (qs->alloc && qs->ops->uninit_allocator) {
> >> +            qs->ops->uninit_allocator(qs->alloc);
> >> +            qs->alloc = NULL;
> >> +        }
> >> +        if (qs->pcibus && qs->ops->qpci_free) {
> >> +            qs->ops->qpci_free(qs->pcibus);
> >> +            qs->pcibus = NULL;
> >> +        }
> > 
> > Is it safe to cleanup the allocator before the PCI stuff?  Usually
> > cleanups want to go in the opposite order to initialization.
> 
> Yes, you're right. Im' going to fix that.

Ok.

> >>      }
> >>      qtest_quit(qs->qts);
> >>      g_free(qs);
> >> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> >> index 604980d..a9f6990 100644
> >> --- a/tests/libqos/libqos.h
> >> +++ b/tests/libqos/libqos.h
> >> @@ -8,11 +8,14 @@
> >>  typedef struct QOSOps {
> >>      QGuestAllocator *(*init_allocator)(QAllocOpts);
> >>      void (*uninit_allocator)(QGuestAllocator *);
> >> +    QPCIBus *(*qpci_init)(QGuestAllocator *alloc);
> >> +    void (*qpci_free)(QPCIBus *bus);
> >>  } QOSOps;
> >>  
> >>  typedef struct QOSState {
> >>      QTestState *qts;
> >>      QGuestAllocator *alloc;
> >> +    QPCIBus *pcibus;
> >>      QOSOps *ops;
> >>  } QOSState;
> >>  
> >> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> >> index 82066b8..9600ed6 100644
> >> --- a/tests/libqos/pci-pc.c
> >> +++ b/tests/libqos/pci-pc.c
> >> @@ -212,7 +212,7 @@ static void qpci_pc_iounmap(QPCIBus *bus, void *data)
> >>      /* FIXME */
> >>  }
> >>  
> >> -QPCIBus *qpci_init_pc(void)
> >> +QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> >>  {
> >>      QPCIBusPC *ret;
> >>
> > 
> > You've added the alloc parameter, but you don't actually appear to use it..
> 
> It's normal: qpci_init_spapr() needs it and to have the same function
> signature we have to add it to qpci_init_pc() even if it is not used.
> (it's why I have added a lot of of qpci_init_pc(NULL)), so we can add
> init in a generic way in "struct QOSOps". Perhaps we can use "void
> *opaque" instead of "QGuestAllocator *alloc"?

Oh, of course.  Sorry I missed that.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature