Re: [Qemu-devel] [PATCH v3] tests: Functions bus_foreach and device_find from libqos virtio API

[Stefan Hajnoczi]

On Mon, Jun 30, 2014 at 11:55:46AM +0200, Marc Marí wrote:
> +static void qvirtio_pci_foreach_callback(
> +                        QPCIDevice *dev, int devfn, void *data)
> +{
> +    QVirtioPCIForeachData *d = data;
> +    QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev);
> +
> +    if (vpcidev->vdev.device_type == d->device_type) {
> +        d->func(&vpcidev->vdev, d->user_data);
> +    }

We should think about memory management here.  Who will free vpcidev?

In the case where device_type matches, ->func() should be responsible
for the virtio device it receives.

In the case where device_type does not match, we must free vpcidev.

> +}
> +
> +static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data)
> +{
> +    QVirtioPCIDevice *vpcidev = data;
> +    vpcidev->pdev               = ((QVirtioPCIDevice *)d)->pdev;
> +    vpcidev->vdev.device_type   = ((QVirtioPCIDevice *)d)->vdev.device_type;
> +}

This function may need to g_free(d) once you've settled on a memory
management rules for virtio-pci.c.

> +typedef struct QVirtioPCIForeachData {
> +    void (*func)(QVirtioDevice *d, void *data);
> +    uint16_t device_type;
> +    void *user_data;
> +} QVirtioPCIForeachData;

This is only used within virtio-pci.c.  It should be declared there too
since it doesn't need to be public.

> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> new file mode 100644
> index 0000000..52f6b5b
> --- /dev/null
> +++ b/tests/libqos/virtio.h
> @@ -0,0 +1,28 @@
> +/*
> + * libqos virtio definitions
> + *
> + * Copyright (c) 2014 Marc Marí
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_VIRTIO_H
> +#define LIBQOS_VIRTIO_H
> +
> +#define QVIRTQUEUE_MAX_SIZE     1024

Unused, please drop.

> +#define QVIRTIO_VENDOR_ID       0x1AF4
> +
> +#define QVIRTIO_NET_DEVICE_ID   0x1
> +#define QVIRTIO_BLK_DEVICE_ID   0x2
> +
> +typedef struct QVirtioDevice QVirtioDevice;
> +typedef struct QVirtQueue QVirtQueue;
> +typedef struct QVRing QVRing;

QVirtQueue and QVRing are unused.  Please drop them.

> +static void pci_basic(void)
> +{
> +    QVirtioPCIDevice *dev;
> +    QPCIBus *bus;
> +
> +    bus = test_start();
> +
> +    dev = qvirtio_pci_device_find(bus, QVIRTIO_BLK_DEVICE_ID);
> +    g_assert_cmphex(dev->vdev.device_type, ==, QVIRTIO_BLK_DEVICE_ID);
> +    g_assert_cmphex(dev->pdev->devfn, ==, ((PCI_SLOT << 3) | PCI_FN));

Should dev be freed once we are done with it?

pgpKwbwyJ339V.pgp
Description: PGP signature