On 30/09/2016 10:33, Greg Kurz wrote: > On Thu, 29 Sep 2016 19:15:05 +0200 > Laurent Vivier <lviv...@redhat.com> wrote: > >> Signed-off-by: Laurent Vivier <lviv...@redhat.com> >> --- >> tests/virtio-9p-test.c | 53 ++++++++-------- >> tests/virtio-blk-test.c | 154 >> +++++++++++++++++++++-------------------------- >> tests/virtio-net-test.c | 40 +++++------- >> tests/virtio-scsi-test.c | 70 ++++++++++----------- >> 4 files changed, 140 insertions(+), 177 deletions(-) >> > > Hi Laurent, > > Please find my comments below. > >> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c >> index e8b2196..28d7f5b 100644 >> --- a/tests/virtio-9p-test.c >> +++ b/tests/virtio-9p-test.c >> @@ -10,62 +10,57 @@ >> #include "qemu/osdep.h" >> #include "libqtest.h" >> #include "qemu-common.h" >> -#include "libqos/pci-pc.h" >> +#include "libqos/libqos-pc.h" >> #include "libqos/virtio.h" >> #include "libqos/virtio-pci.h" >> -#include "libqos/malloc.h" >> -#include "libqos/malloc-pc.h" >> #include "standard-headers/linux/virtio_ids.h" >> #include "standard-headers/linux/virtio_pci.h" >> >> static const char mount_tag[] = "qtest"; >> static char *test_share; >> >> -static void qvirtio_9p_start(void) >> -{ >> - char *args; >> >> +static QOSState *qvirtio_9p_start(void) >> +{ >> test_share = g_strdup("/tmp/qtest.XXXXXX"); >> g_assert_nonnull(mkdtemp(test_share)); >> + const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s " >> + "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s"; >> >> - args = g_strdup_printf("-fsdev >> local,id=fsdev0,security_model=none,path=%s " >> - "-device >> virtio-9p-pci,fsdev=fsdev0,mount_tag=%s", >> - test_share, mount_tag); >> - >> - qtest_start(args); >> - g_free(args); >> + return qtest_pc_boot(cmd, test_share, mount_tag); >> } >> >> -static void qvirtio_9p_stop(void) >> +static void qvirtio_9p_stop(QOSState *qs) >> { >> - qtest_end(); >> + qtest_pc_shutdown(qs); >> rmdir(test_share); >> g_free(test_share); >> } >> >> static void pci_nop(void) >> { >> - qvirtio_9p_start(); >> - qvirtio_9p_stop(); >> + QOSState *qs; >> + >> + qs = qvirtio_9p_start(); >> + g_assert(qs); > > The appropriate macro to use here is: g_assert_nonnull().
OK > > BTW, how can qs be NULL ? we should not know what happens in qtest_pc_boot() (or qtest_spapr_boot(), or qtest_XXX_boot()) So I think it i better to check it before to use it. >> + qvirtio_9p_stop(qs); >> } >> >> typedef struct { >> QVirtioDevice *dev; >> - QGuestAllocator *alloc; >> - QPCIBus *bus; >> + QOSState *qs; >> QVirtQueue *vq; >> } QVirtIO9P; >> >> -static QVirtIO9P *qvirtio_9p_pci_init(void) >> +static QVirtIO9P *qvirtio_9p_pci_init(QOSState *qs) >> { >> QVirtIO9P *v9p; >> QVirtioPCIDevice *dev; >> >> v9p = g_new0(QVirtIO9P, 1); >> - v9p->alloc = pc_alloc_init(); >> - v9p->bus = qpci_init_pc(NULL); >> >> - dev = qvirtio_pci_device_find(v9p->bus, VIRTIO_ID_9P); >> + v9p->qs = qs; >> + dev = qvirtio_pci_device_find(v9p->qs->pcibus, VIRTIO_ID_9P); >> g_assert_nonnull(dev); >> g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_9P); >> v9p->dev = (QVirtioDevice *) dev; >> @@ -75,17 +70,15 @@ static QVirtIO9P *qvirtio_9p_pci_init(void) >> qvirtio_set_acknowledge(&qvirtio_pci, v9p->dev); >> qvirtio_set_driver(&qvirtio_pci, v9p->dev); >> >> - v9p->vq = qvirtqueue_setup(&qvirtio_pci, v9p->dev, v9p->alloc, 0); >> + v9p->vq = qvirtqueue_setup(&qvirtio_pci, v9p->dev, v9p->qs->alloc, 0); >> return v9p; >> } >> >> static void qvirtio_9p_pci_free(QVirtIO9P *v9p) >> { >> - qvirtqueue_cleanup(&qvirtio_pci, v9p->vq, v9p->alloc); >> - pc_alloc_uninit(v9p->alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, v9p->vq, v9p->qs->alloc); >> qvirtio_pci_device_disable(container_of(v9p->dev, QVirtioPCIDevice, >> vdev)); >> g_free(v9p->dev); >> - qpci_free_pc(v9p->bus); >> g_free(v9p); >> } >> >> @@ -96,9 +89,11 @@ static void pci_basic_config(void) >> size_t tag_len; >> char *tag; >> int i; >> + QOSState *qs; >> >> - qvirtio_9p_start(); >> - v9p = qvirtio_9p_pci_init(); >> + qs = qvirtio_9p_start(); >> + g_assert(qs); > > Null qs ? > >> + v9p = qvirtio_9p_pci_init(qs); >> >> addr = ((QVirtioPCIDevice *) v9p->dev)->addr + >> VIRTIO_PCI_CONFIG_OFF(false); >> tag_len = qvirtio_config_readw(&qvirtio_pci, v9p->dev, >> @@ -115,7 +110,7 @@ static void pci_basic_config(void) >> g_free(tag); >> >> qvirtio_9p_pci_free(v9p); >> - qvirtio_9p_stop(); >> + qvirtio_9p_stop(qs); >> } >> >> int main(int argc, char **argv) >> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c >> index 3c4fecc..8cf62f6 100644 >> --- a/tests/virtio-blk-test.c >> +++ b/tests/virtio-blk-test.c >> @@ -10,12 +10,10 @@ >> >> #include "qemu/osdep.h" >> #include "libqtest.h" >> +#include "libqos/libqos-pc.h" >> #include "libqos/virtio.h" >> #include "libqos/virtio-pci.h" >> #include "libqos/virtio-mmio.h" >> -#include "libqos/pci-pc.h" >> -#include "libqos/malloc.h" >> -#include "libqos/malloc-pc.h" >> #include "libqos/malloc-generic.h" >> #include "qemu/bswap.h" >> #include "standard-headers/linux/virtio_ids.h" >> @@ -58,24 +56,21 @@ static char *drive_create(void) >> return tmp_path; >> } >> >> -static QPCIBus *pci_test_start(void) >> +static QOSState *pci_test_start(void) >> { >> - char *cmdline; >> + QOSState *qs = NULL; > > Why setting qs to NULL ? It is necessarily set... Yes, I've mixed my patches: later we add a "if (arch == pc) { qs = } else if (arch == spapr) { qs = }" and this case qs can be uninitialized. >> char *tmp_path; >> + const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw " >> + "-drive if=none,id=drive1,file=/dev/null,format=raw " >> + "-device virtio-blk-pci,id=drv0,drive=drive0," >> + "addr=%x.%x"; >> >> tmp_path = drive_create(); >> >> - cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s,format=raw " >> - "-drive if=none,id=drive1,file=/dev/null,format=raw >> " >> - "-device virtio-blk-pci,id=drv0,drive=drive0," >> - "addr=%x.%x", >> - tmp_path, PCI_SLOT, PCI_FN); >> - qtest_start(cmdline); >> + qs = qtest_pc_boot(cmd, tmp_path, PCI_SLOT, PCI_FN); > > ... here. > >> unlink(tmp_path); >> g_free(tmp_path); >> - g_free(cmdline); >> - >> - return qpci_init_pc(NULL); >> + return qs; >> } >> >> static void arm_test_start(void) >> @@ -279,39 +274,35 @@ static void test_basic(const QVirtioBus *bus, >> QVirtioDevice *dev, >> static void pci_basic(void) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> + QOSState *qs; >> QVirtQueuePCI *vqpci; >> - QGuestAllocator *alloc; >> void *addr; >> >> - bus = pci_test_start(); >> - dev = virtio_blk_pci_init(bus, PCI_SLOT); >> + qs = pci_test_start(); >> + g_assert(qs); > > Null qs ? > >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); >> >> - alloc = pc_alloc_init(); >> vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, >> 0); >> + qs->alloc, 0); >> >> /* MSI-X is not enabled */ >> addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); >> >> - test_basic(&qvirtio_pci, &dev->vdev, alloc, &vqpci->vq, >> + test_basic(&qvirtio_pci, &dev->vdev, qs->alloc, &vqpci->vq, >> >> (uint64_t)(uintptr_t)addr); > > Maybe worth to fix the funky indentation... this can be done globally in a > followup patch. I will resend, so I will fix this > >> /* End test */ >> - qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, alloc); >> - pc_alloc_uninit(alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, qs->alloc); >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > The title is "tests: use qtest_pc_boot()/qtest_pc_shutdown() in virtio tests" > > Even if qtest_shutdown() happens to be equivalent to qtest_pc_shutdown() when > qtest_pc_boot() was called, I would rather stick to the title, and convert > all qtest_pc_shutdown() to qtest_shutdown() in patch 3/3... > > No big deal though, you may s/qtest_pc_shutdown/qtest_shutdown in the title > as well :) I'm to change all qtest_pc_shutdown() to qtest_shutdown() here >> } >> >> static void pci_indirect(void) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> QVirtQueuePCI *vqpci; >> - QGuestAllocator *alloc; >> + QOSState *qs; >> QVirtioBlkReq req; >> QVRingIndirectDesc *indirect; >> void *addr; >> @@ -322,9 +313,10 @@ static void pci_indirect(void) >> uint8_t status; >> char *data; >> >> - bus = pci_test_start(); >> + qs = pci_test_start(); >> + g_assert(qs); >> > > Same remark about qs being NULL. > >> - dev = virtio_blk_pci_init(bus, PCI_SLOT); >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); >> >> /* MSI-X is not enabled */ >> addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); >> @@ -340,9 +332,8 @@ static void pci_indirect(void) >> (1u << VIRTIO_BLK_F_SCSI)); >> qvirtio_set_features(&qvirtio_pci, &dev->vdev, features); >> >> - alloc = pc_alloc_init(); >> vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, >> 0); >> + qs->alloc, 0); >> qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev); >> >> /* Write request */ >> @@ -352,11 +343,11 @@ static void pci_indirect(void) >> req.data = g_malloc0(512); >> strcpy(req.data, "TEST"); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> - indirect = qvring_indirect_desc_setup(&dev->vdev, alloc, 2); >> + indirect = qvring_indirect_desc_setup(&dev->vdev, qs->alloc, 2); >> qvring_indirect_desc_add(indirect, req_addr, 528, false); >> qvring_indirect_desc_add(indirect, req_addr + 528, 1, true); >> free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect); >> @@ -368,7 +359,7 @@ static void pci_indirect(void) >> g_assert_cmpint(status, ==, 0); >> >> g_free(indirect); >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* Read request */ >> req.type = VIRTIO_BLK_T_IN; >> @@ -377,11 +368,11 @@ static void pci_indirect(void) >> req.data = g_malloc0(512); >> strcpy(req.data, "TEST"); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> - indirect = qvring_indirect_desc_setup(&dev->vdev, alloc, 2); >> + indirect = qvring_indirect_desc_setup(&dev->vdev, qs->alloc, 2); >> qvring_indirect_desc_add(indirect, req_addr, 16, false); >> qvring_indirect_desc_add(indirect, req_addr + 16, 513, true); >> free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect); >> @@ -398,28 +389,27 @@ static void pci_indirect(void) >> g_free(data); >> >> g_free(indirect); >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* End test */ >> - qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, alloc); >> - pc_alloc_uninit(alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, qs->alloc); >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? No, as we have qtest_shutdown() from a previous series, we can use it now. > >> } >> >> static void pci_config(void) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> + QOSState *qs; >> int n_size = TEST_IMAGE_SIZE / 2; >> void *addr; >> uint64_t capacity; >> >> - bus = pci_test_start(); >> + qs = pci_test_start(); >> + g_assert(qs); >> > > Same remark about qs being NULL. > >> - dev = virtio_blk_pci_init(bus, PCI_SLOT); >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); >> >> /* MSI-X is not enabled */ >> addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); >> @@ -440,16 +430,15 @@ static void pci_config(void) >> >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> >> static void pci_msix(void) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> + QOSState *qs; >> QVirtQueuePCI *vqpci; >> - QGuestAllocator *alloc; >> QVirtioBlkReq req; >> int n_size = TEST_IMAGE_SIZE / 2; >> void *addr; >> @@ -460,13 +449,13 @@ static void pci_msix(void) >> uint8_t status; >> char *data; >> >> - bus = pci_test_start(); >> - alloc = pc_alloc_init(); >> + qs = pci_test_start(); >> + g_assert(qs); >> > > Null qs ? > >> - dev = virtio_blk_pci_init(bus, PCI_SLOT); >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); >> qpci_msix_enable(dev->pdev); >> >> - qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0); >> + qvirtio_pci_set_msix_configuration_vector(dev, qs->alloc, 0); >> >> /* MSI-X is enabled */ >> addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true); >> @@ -483,8 +472,8 @@ static void pci_msix(void) >> qvirtio_set_features(&qvirtio_pci, &dev->vdev, features); >> >> vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, >> 0); >> - qvirtqueue_pci_msix_setup(dev, vqpci, alloc, 1); >> + qs->alloc, 0); >> + qvirtqueue_pci_msix_setup(dev, vqpci, qs->alloc, 1); >> >> qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev); >> >> @@ -504,7 +493,7 @@ static void pci_msix(void) >> req.data = g_malloc0(512); >> strcpy(req.data, "TEST"); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> @@ -519,7 +508,7 @@ static void pci_msix(void) >> status = readb(req_addr + 528); >> g_assert_cmpint(status, ==, 0); >> >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* Read request */ >> req.type = VIRTIO_BLK_T_IN; >> @@ -527,7 +516,7 @@ static void pci_msix(void) >> req.sector = 0; >> req.data = g_malloc0(512); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> @@ -549,24 +538,21 @@ static void pci_msix(void) >> g_assert_cmpstr(data, ==, "TEST"); >> g_free(data); >> >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* End test */ >> - qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, alloc); >> - pc_alloc_uninit(alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, qs->alloc); >> qpci_msix_disable(dev->pdev); >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> >> static void pci_idx(void) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> + QOSState *qs; >> QVirtQueuePCI *vqpci; >> - QGuestAllocator *alloc; >> QVirtioBlkReq req; >> void *addr; >> uint64_t req_addr; >> @@ -576,13 +562,13 @@ static void pci_idx(void) >> uint8_t status; >> char *data; >> >> - bus = pci_test_start(); >> - alloc = pc_alloc_init(); >> + qs = pci_test_start(); >> + g_assert(qs); >> > > Null qs ? > >> - dev = virtio_blk_pci_init(bus, PCI_SLOT); >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); >> qpci_msix_enable(dev->pdev); >> >> - qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0); >> + qvirtio_pci_set_msix_configuration_vector(dev, qs->alloc, 0); >> >> /* MSI-X is enabled */ >> addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true); >> @@ -599,8 +585,8 @@ static void pci_idx(void) >> qvirtio_set_features(&qvirtio_pci, &dev->vdev, features); >> >> vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, >> 0); >> - qvirtqueue_pci_msix_setup(dev, vqpci, alloc, 1); >> + qs->alloc, 0); >> + qvirtqueue_pci_msix_setup(dev, vqpci, qs->alloc, 1); >> >> qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev); >> >> @@ -611,7 +597,7 @@ static void pci_idx(void) >> req.data = g_malloc0(512); >> strcpy(req.data, "TEST"); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> @@ -630,7 +616,7 @@ static void pci_idx(void) >> req.data = g_malloc0(512); >> strcpy(req.data, "TEST"); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> @@ -647,7 +633,7 @@ static void pci_idx(void) >> QVIRTIO_BLK_TIMEOUT_US); >> g_assert_cmpint(status, ==, 0); >> >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* Read request */ >> req.type = VIRTIO_BLK_T_IN; >> @@ -655,7 +641,7 @@ static void pci_idx(void) >> req.sector = 1; >> req.data = g_malloc0(512); >> >> - req_addr = virtio_blk_request(alloc, &req, 512); >> + req_addr = virtio_blk_request(qs->alloc, &req, 512); >> >> g_free(req.data); >> >> @@ -676,38 +662,36 @@ static void pci_idx(void) >> g_assert_cmpstr(data, ==, "TEST"); >> g_free(data); >> >> - guest_free(alloc, req_addr); >> + guest_free(qs->alloc, req_addr); >> >> /* End test */ >> - qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, alloc); >> - pc_alloc_uninit(alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, qs->alloc); >> qpci_msix_disable(dev->pdev); >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> >> static void pci_hotplug(void) >> { >> - QPCIBus *bus; >> QVirtioPCIDevice *dev; >> + QOSState *qs; >> >> - bus = pci_test_start(); >> + qs = pci_test_start(); >> + g_assert(qs); >> >> /* plug secondary disk */ >> qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP, >> "'drive': 'drive1'"); >> >> - dev = virtio_blk_pci_init(bus, PCI_SLOT_HP); >> + dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP); >> g_assert(dev); >> qvirtio_pci_device_disable(dev); >> g_free(dev); >> >> /* unplug secondary disk */ >> qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> >> static void mmio_basic(void) >> @@ -746,8 +730,8 @@ static void mmio_basic(void) >> >> /* End test */ >> qvirtqueue_cleanup(&qvirtio_mmio, vq, alloc); >> - generic_alloc_uninit(alloc); >> g_free(dev); >> + generic_alloc_uninit(alloc); >> test_end(); >> } >> >> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c >> index a343a6b..3e83685 100644 >> --- a/tests/virtio-net-test.c >> +++ b/tests/virtio-net-test.c >> @@ -12,12 +12,9 @@ >> #include "qemu-common.h" >> #include "qemu/sockets.h" >> #include "qemu/iov.h" >> -#include "libqos/pci-pc.h" >> +#include "libqos/libqos-pc.h" >> #include "libqos/virtio.h" >> #include "libqos/virtio-pci.h" >> -#include "libqos/malloc.h" >> -#include "libqos/malloc-pc.h" >> -#include "libqos/malloc-generic.h" >> #include "qemu/bswap.h" >> #include "hw/virtio/virtio-net.h" >> #include "standard-headers/linux/virtio_ids.h" >> @@ -53,16 +50,12 @@ static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus >> *bus, int slot) >> return dev; >> } >> >> -static QPCIBus *pci_test_start(int socket) >> +static QOSState *pci_test_start(int socket) >> { >> - char *cmdline; >> + const char *cmd = "-netdev socket,fd=%d,id=hs0 -device " >> + "virtio-net-pci,netdev=hs0"; >> >> - cmdline = g_strdup_printf("-netdev socket,fd=%d,id=hs0 -device " >> - "virtio-net-pci,netdev=hs0", socket); >> - qtest_start(cmdline); >> - g_free(cmdline); >> - >> - return qpci_init_pc(NULL); >> + return qtest_pc_boot(cmd, socket); >> } >> >> static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev) >> @@ -205,9 +198,8 @@ static void stop_cont_test(const QVirtioBus *bus, >> QVirtioDevice *dev, >> static void pci_basic(gconstpointer data) >> { >> QVirtioPCIDevice *dev; >> - QPCIBus *bus; >> + QOSState *qs; >> QVirtQueuePCI *tx, *rx; >> - QGuestAllocator *alloc; >> void (*func) (const QVirtioBus *bus, >> QVirtioDevice *dev, >> QGuestAllocator *alloc, >> @@ -219,28 +211,26 @@ static void pci_basic(gconstpointer data) >> ret = socketpair(PF_UNIX, SOCK_STREAM, 0, sv); >> g_assert_cmpint(ret, !=, -1); >> >> - bus = pci_test_start(sv[1]); >> - dev = virtio_net_pci_init(bus, PCI_SLOT); >> + qs = pci_test_start(sv[1]); >> + g_assert(qs); > > Null qs ? > >> + dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT); >> >> - alloc = pc_alloc_init(); >> rx = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, 0); >> + qs->alloc, 0); >> tx = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev, >> - alloc, 1); >> + qs->alloc, 1); >> >> driver_init(&qvirtio_pci, &dev->vdev); >> - func(&qvirtio_pci, &dev->vdev, alloc, &rx->vq, &tx->vq, sv[0]); >> + func(&qvirtio_pci, &dev->vdev, qs->alloc, &rx->vq, &tx->vq, sv[0]); >> >> /* End test */ >> close(sv[0]); >> - qvirtqueue_cleanup(&qvirtio_pci, &tx->vq, alloc); >> - qvirtqueue_cleanup(&qvirtio_pci, &rx->vq, alloc); >> - pc_alloc_uninit(alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &tx->vq, qs->alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, &rx->vq, qs->alloc); >> qvirtio_pci_device_disable(dev); >> g_free(dev->pdev); >> g_free(dev); >> - qpci_free_pc(bus); >> - test_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> #endif >> >> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c >> index 79088bb..721ae1f 100644 >> --- a/tests/virtio-scsi-test.c >> +++ b/tests/virtio-scsi-test.c >> @@ -11,12 +11,9 @@ >> #include "qemu/osdep.h" >> #include "libqtest.h" >> #include "block/scsi.h" >> +#include "libqos/libqos-pc.h" >> #include "libqos/virtio.h" >> #include "libqos/virtio-pci.h" >> -#include "libqos/pci-pc.h" >> -#include "libqos/malloc.h" >> -#include "libqos/malloc-pc.h" >> -#include "libqos/malloc-generic.h" >> #include "standard-headers/linux/virtio_ids.h" >> #include "standard-headers/linux/virtio_pci.h" >> #include "standard-headers/linux/virtio_scsi.h" >> @@ -29,28 +26,23 @@ >> >> typedef struct { >> QVirtioDevice *dev; >> - QGuestAllocator *alloc; >> - QPCIBus *bus; >> + QOSState *qs; >> int num_queues; >> QVirtQueue *vq[MAX_NUM_QUEUES + 2]; >> } QVirtIOSCSI; >> >> -static void qvirtio_scsi_start(const char *extra_opts) >> +static QOSState *qvirtio_scsi_start(const char *extra_opts) >> { >> - char *cmdline; >> - >> - cmdline = g_strdup_printf( >> - "-drive id=drv0,if=none,file=/dev/null,format=raw " >> - "-device virtio-scsi-pci,id=vs0 " >> - "-device scsi-hd,bus=vs0.0,drive=drv0 %s", >> - extra_opts ? : ""); >> - qtest_start(cmdline); >> - g_free(cmdline); >> + const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw " >> + "-device virtio-scsi-pci,id=vs0 " >> + "-device scsi-hd,bus=vs0.0,drive=drv0 %s"; >> + >> + return qtest_pc_boot(cmd, extra_opts ? : ""); >> } >> >> -static void qvirtio_scsi_stop(void) >> +static void qvirtio_scsi_stop(QOSState *qs) >> { >> - qtest_end(); >> + qtest_shutdown(qs); > > qtest_pc_shutdown() ? > >> } >> >> static void qvirtio_scsi_pci_free(QVirtIOSCSI *vs) >> @@ -58,12 +50,10 @@ static void qvirtio_scsi_pci_free(QVirtIOSCSI *vs) >> int i; >> >> for (i = 0; i < vs->num_queues + 2; i++) { >> - qvirtqueue_cleanup(&qvirtio_pci, vs->vq[i], vs->alloc); >> + qvirtqueue_cleanup(&qvirtio_pci, vs->vq[i], vs->qs->alloc); >> } >> - pc_alloc_uninit(vs->alloc); >> qvirtio_pci_device_disable(container_of(vs->dev, QVirtioPCIDevice, >> vdev)); >> g_free(vs->dev); >> - qpci_free_pc(vs->bus); >> } >> >> static uint64_t qvirtio_scsi_alloc(QVirtIOSCSI *vs, size_t alloc_size, >> @@ -71,7 +61,7 @@ static uint64_t qvirtio_scsi_alloc(QVirtIOSCSI *vs, size_t >> alloc_size, >> { >> uint64_t addr; >> >> - addr = guest_alloc(vs->alloc, alloc_size); >> + addr = guest_alloc(vs->qs->alloc, alloc_size); >> if (data) { >> memwrite(addr, data, alloc_size); >> } >> @@ -128,10 +118,10 @@ static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, >> const uint8_t *cdb, >> memread(resp_addr, resp_out, sizeof(*resp_out)); >> } >> >> - guest_free(vs->alloc, req_addr); >> - guest_free(vs->alloc, resp_addr); >> - guest_free(vs->alloc, data_in_addr); >> - guest_free(vs->alloc, data_out_addr); >> + guest_free(vs->qs->alloc, req_addr); >> + guest_free(vs->qs->alloc, resp_addr); >> + guest_free(vs->qs->alloc, data_in_addr); >> + guest_free(vs->qs->alloc, data_out_addr); >> return response; >> } >> >> @@ -145,10 +135,12 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) >> int i; >> >> vs = g_new0(QVirtIOSCSI, 1); >> - vs->alloc = pc_alloc_init(); >> - vs->bus = qpci_init_pc(NULL); >> >> - dev = qvirtio_pci_device_find(vs->bus, VIRTIO_ID_SCSI); >> + vs->qs = qvirtio_scsi_start("-drive file=blkdebug::null-co://," >> + "if=none,id=dr1,format=raw,file.align=4k " >> + "-device >> scsi-disk,drive=dr1,lun=0,scsi-id=1"); >> + g_assert(vs->qs); > > Null vs->qs ? > >> + dev = qvirtio_pci_device_find(vs->qs->pcibus, VIRTIO_ID_SCSI); >> vs->dev = (QVirtioDevice *)dev; >> g_assert(dev != NULL); >> g_assert_cmphex(vs->dev->device_type, ==, VIRTIO_ID_SCSI); >> @@ -165,7 +157,7 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) >> g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES); >> >> for (i = 0; i < vs->num_queues + 2; i++) { >> - vs->vq[i] = qvirtqueue_setup(&qvirtio_pci, vs->dev, vs->alloc, i); >> + vs->vq[i] = qvirtqueue_setup(&qvirtio_pci, vs->dev, vs->qs->alloc, >> i); >> } >> >> /* Clear the POWER ON OCCURRED unit attention */ >> @@ -184,15 +176,20 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) >> /* Tests only initialization so far. TODO: Replace with functional tests */ >> static void pci_nop(void) >> { >> - qvirtio_scsi_start(NULL); >> - qvirtio_scsi_stop(); >> + QOSState *qs; >> + >> + qs = qvirtio_scsi_start(NULL); >> + g_assert(qs); > > Null qs ? > >> + qvirtio_scsi_stop(qs); >> } >> >> static void hotplug(void) >> { >> QDict *response; >> + QOSState *qs; >> >> - qvirtio_scsi_start("-drive id=drv1,if=none,file=/dev/null,format=raw"); >> + qs = qvirtio_scsi_start("-drive >> id=drv1,if=none,file=/dev/null,format=raw"); >> + g_assert(qs); > > Null qs ? > >> response = qmp("{\"execute\": \"device_add\"," >> " \"arguments\": {" >> " \"driver\": \"scsi-hd\"," >> @@ -214,7 +211,7 @@ static void hotplug(void) >> g_assert(qdict_haskey(response, "event")); >> g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED")); >> QDECREF(response); >> - qvirtio_scsi_stop(); >> + qvirtio_scsi_stop(qs); >> } >> >> /* Test WRITE SAME with the lba not aligned */ >> @@ -230,9 +227,6 @@ static void test_unaligned_write_same(void) >> 0x41, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x33, 0x00, 0x00 >> }; >> >> - qvirtio_scsi_start("-drive file=blkdebug::null-co://,if=none,id=dr1" >> - ",format=raw,file.align=4k " >> - "-device scsi-disk,drive=dr1,lun=0,scsi-id=1"); >> vs = qvirtio_scsi_pci_init(PCI_SLOT); >> >> g_assert_cmphex(0, ==, >> @@ -242,7 +236,7 @@ static void test_unaligned_write_same(void) >> virtio_scsi_do_command(vs, write_same_cdb_2, NULL, 0, buf2, 512, >> NULL)); >> >> qvirtio_scsi_pci_free(vs); >> - qvirtio_scsi_stop(); >> + qvirtio_scsi_stop(vs->qs); > > Is still vs->qs still valid ? Also it looks wrong to call qvirtio_scsi_stop() > without any prior call to qvirtio_scsi_start()... > >> } >> >> int main(int argc, char **argv) > > Cheers. Thanks, Laurent