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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list