qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] tests: use qtest_pc_boot()/qtest_pc_shutdow


From: Greg Kurz
Subject: Re: [Qemu-devel] [PATCH 1/3] tests: use qtest_pc_boot()/qtest_pc_shutdown() in virtio tests
Date: Fri, 30 Sep 2016 10:33:41 +0200

On Thu, 29 Sep 2016 19:15:05 +0200
Laurent Vivier <address@hidden> wrote:

> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  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().

BTW, how can qs be NULL ?

> +    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...

>      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.

>      /* 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 :)

>  }
>  
>  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() ?

>  }
>  
>  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.

--
Greg



reply via email to

[Prev in Thread] Current Thread [Next in Thread]