[libvirt] [PATCHv2 04/12] pci: new iommu_group functions

[Laine Stump]

On 06/25/2013 06:40 AM, Daniel P. Berrange wrote:
> On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
>> Any device which belongs to an "IOMMU group" (used by vfio) will
>> have links to all devices of its group listed in
>> /sys/bus/pci/$device/iommu_group/devices;
>> /sys/bus/pci/$device/iommu_group is actually a link to
>> /sys/kernel/iommu_groups/$n, where $n is the group number (there
>> will be a corresponding device node at /dev/vfio/$n once the
>> devices are bound to the vfio-pci driver)
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index fc04cac..5209372 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -1852,6 +1852,223 @@ cleanup:
>>      return ret;
>>  }
>>  
>> +
>> +/* virPCIIOMMUGroupIterate:
>> + *   Call @actor for all devices in the same iommu_group as orig
>> + *   (including orig itself) Even if there is no iommu_group for the
>> + *   device, call @actor once for orig.
>> + */
>> +int
>> +virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
>> +                        virPCIDeviceAddressActor actor,
>> +                        void *opaque)
>> +{
>> +    char *groupPath = NULL;
>> +    DIR *groupDir = NULL;
>> +    int ret = -1;
>> +    struct dirent *ent;
>> +
>> +    if (virAsprintf(&groupPath,
>> +                    PCI_SYSFS "devices/%04x:%02x:%02x.%x/iommu_group/devices",
>> +                    orig->domain, orig->bus, orig->slot, orig->function) < 0) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    if (!(groupDir = opendir(groupPath))) {
>> +        /* just process the original device, nothing more */
>> +        ret = (actor)(orig, opaque);
>> +        goto cleanup;
>> +    }
>> +
> Since Eric isn't around to nit-pick, I'll do it :-)
>
> Set 'errno = 0' here before the readdir() call
>
>> +    while ((ent = readdir(groupDir)) != NULL) {
>> +        virPCIDeviceAddress newDev;
>> +
>> +        if (ent->d_name[0] == '.')
>> +            continue;
>> +
>> +        if (virPCIParseDeviceAddress(ent->d_name, &newDev) < 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Found invalid device link '%s' in '%s'"),
>> +                           ent->d_name, groupPath);
>> +            goto cleanup;
>> +        }
>> +
>> +        if ((actor)(&newDev, opaque) < 0)
>> +            goto cleanup;
> And here
>
>            errno = 0;
>
>> +    }
> Then do
>
>        if (errno != 0) {
>           virReportSystemError(errno, _("Failed to read directory entry for %s"), grouppath)
>           goto cleanup
>        }
>
> that way you distinguish EOF from error when readdir() returns NULL.
>
> /me makes a note that we should write a wrapper around readdir(),
> since almost all our code has this wrong.

Definitely. If I ever did know about this idiosyncracy of readdir() in
the past, I had certainly forgotten it.


>> +
>> +    ret = 0;
>> +
>> +cleanup:
>> +    VIR_FREE(groupPath);
>> +    if (groupDir)
>> +        closedir(groupDir);
>> +    return ret;
>> +}
>> +
>> +
>> +static int
>> +virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
>> +{
>> +    int ret = -1;
>> +    virPCIDeviceListPtr groupList = opaque;
>> +    virPCIDevicePtr newDev;
>> +
>> +    if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus,
>> +                                   newDevAddr->slot, newDevAddr->function))) {
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virPCIDeviceListAdd(groupList, newDev) < 0) {
>> +        goto cleanup;
>> +    }
>> +    newDev = NULL; /* it's now on the list */
>> +    ret = 0;
>> +cleanup:
>> +    virPCIDeviceFree(newDev);
>> +    return ret;
>> +}
>> +
>> +
>> +/*
>> + * virPCIDeviceGetIOMMUGroupList - return a virPCIDeviceList containing
>> + * all of the devices in the same iommu_group as @dev.
>> + *
>> + * Return the new list, or NULL on failure
>> + */
>> +virPCIDeviceListPtr
>> +virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
>> +{
>> +    virPCIDeviceListPtr groupList = virPCIDeviceListNew();
>> +    virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
>> +                                    dev->slot, dev->function };
>> +
>> +    if (!groupList)
>> +        goto error;
>> +
>> +    if (virPCIIOMMUGroupIterate(&devAddr, virPCIDeviceGetIOMMUGroupAddOne,
>> +                                groupList) < 0) {
>> +        goto error;
>> +    }
> Overkill with  {} here


In addition to the necessary braces when the body is > 1 line, I also
like to have {} when the conditional itself is > 1 line (and I've been
doing that ever since I can remember). If you dislike it I can cease and
desist, though :-)


>> +static int
>> +virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
>> +{
>> +    int ret = -1;
>> +    virPCIDeviceAddressListPtr addrList = opaque;
>> +    virPCIDeviceAddressPtr copyAddr;
>> +
>> +    /* make a copy to insert onto the list */
>> +    if (VIR_ALLOC(copyAddr) < 0)
>> +        goto cleanup;
>> +
>> +    *copyAddr = *newDevAddr;
>> +
>> +    if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
>> +                           *addrList->nIommuGroupDevices, copyAddr) < 0) {
>> +        goto cleanup;
>> +    }
> Overkill with {}
>
> Missing virReportOOMError() ?

Yes. I caught myself omitting one of those the other night, and at the
time thought to myself "I think I left one out somewhere else too...".

Maybe we should send *all* VIR_* macros down the same road as
VIR_STRDUP, and integrate OOM error logging into them.


>
>> +
>> +    ret = 0;
>> +cleanup:
>> +    VIR_FREE(copyAddr);
>> +    return ret;
>> +}
>> +
>> +
>> +/*
>> + * virPCIGetIOMMUGroupAddresses - return a virPCIDeviceList containing
>> + * all of the devices in the same iommu_group as @dev.
>> + *
>> + * Return the new list, or NULL on failure
>> + */
>> +int
>> +virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
>> +                             virPCIDeviceAddressPtr **iommuGroupDevices,
>> +                             size_t *nIommuGroupDevices)
>> +{
>> +    int ret = -1;
>> +    virPCIDeviceAddressList addrList = { iommuGroupDevices,
>> +                                         nIommuGroupDevices };
>> +
>> +    if (virPCIIOMMUGroupIterate(devAddr,
>> +                                virPCIGetIOMMUGroupAddressesAddOne,
>> +                                &addrList) < 0) {
>> +        goto cleanup;
>> +    }
> Overkill with {}

Same deal - the condition is 3 lines long and this gives me a visual
crush. It could just be because I'm accustomed to it though (I also used
to hate the K&R/libvirt style of putting the opening brace at the end of
the line, but I've gotten used to that too)