[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-devel
Subject:    Re: [Qemu-devel] [PATCH v2 28/30] tests: add specialized device_find function
From:       Eric Blake <eblake () redhat ! com>
Date:       2017-02-28 23:21:05
Message-ID: 304c6066-fea0-f5e4-9c6f-384afa9ff5f2 () redhat ! com
[Download RAW message or body]


On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Allows to specify which slot to look for the device.

"Allow[s] to ${verb}" is not idiomatic; it's missing a subject.  But
"Allows $subject to" (as in "allows someone to" or "allows me to") is
wordy, compared to just saying "Allows ${verb}ing".  I'd suggest:

Allow specifying which slot to favor when looking for the device.

> 
> This will be used in the following patch to avoid leaking when multiple
> devices exists and we want to lookup the hotplug one.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  4 ++--
>  tests/libqos/virtio-pci.c | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 

> @@ -55,10 +57,11 @@ static void qvirtio_pci_foreach_callback(
>      QVirtioPCIForeachData *d = data;
>      QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev);
>  
> -    if (vpcidev->vdev.device_type == d->device_type) {
> +    if (vpcidev->vdev.device_type == d->device_type &&
> +        (!d->has_slot || vpcidev->pdev->devfn == d->slot << 3)) {
>          d->func(&vpcidev->vdev, d->user_data);
>      } else {
> -        g_free(vpcidev);
> +        qvirtio_pci_device_free(vpcidev);

Is this an unmentioned leak plug?  Either it should be mentioned, or
squashed into the leak fix of 29.

With those cleanups,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]