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 <lviv...@redhat.com>
>> ---
>> 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.
>> }
>> 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"?
Thanks,
Laurent
