[PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()

[Laine Stump]

Previously there was no way to differentiate between this function 1)
encountering an error while reading the pci config, and 2) determining
that the device in question is a conventional PCI device, and so has
no Express Capabilities.

The difference between these two conditions is important, because an
unprivileged libvirtd will be unable to read all of the pci config (it
can only read the first 64 bytes, and will get ENOENT when it tries to
seek past that limit) even though the device is in fact a PCIe device.

This patch just changes virPCIDeviceFindCapabilityOffset() to put the
determined offset into an argument of the function (rather than
sending it back as the return value), and to return the standard "0 on
success, -1 on failure". Failure is determined by checking the value
of errno after each attemptd read of the config file (which can only
work reliably if errno is reset to 0 before each read, and after
virPCIDeviceFindCapabilityOffset() has finished examining it).

(NB: if the config file is read successfully, but no Express
Capabilities are found, then the function returns success, but the
returned offset will be 0 (which is an impossible offset for Express
Capabilities, and so easily recognizeable).

An upcoming patch will take advantage of the change made here.

Signed-off-by: Laine Stump <laine at redhat.com>
---
 src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9109fb4f3a..2f99bb93bd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
                  unsigned int buflen)
 {
     memset(buf, 0, buflen);
+    errno = 0;
 
     if (lseek(cfgfd, pos, SEEK_SET) != pos ||
         saferead(cfgfd, buf, buflen) != buflen) {
@@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
     return ret;
 }
 
-static uint8_t
+static int
 virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
                                  int cfgfd,
-                                 unsigned int capability)
+                                 unsigned int capability,
+                                 unsigned int *offset)
 {
     uint16_t status;
     uint8_t pos;
 
+    *offset = 0; /* assume failure (*nothing* can be at offset 0) */
+
     status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
-    if (!(status & PCI_STATUS_CAP_LIST))
-        return 0;
+    if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
+        goto error;
 
     pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
+    if (errno != 0)
+        goto error;
 
     /* Zero indicates last capability, capabilities can't
      * be in the config space header and 0xff is returned
@@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
      */
     while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
         uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
+        if (errno != 0)
+            goto error;
+
         if (capid == capability) {
             VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
                       dev->id, dev->name, capability, pos);
-            return pos;
+            *offset = pos;
+            return 0;
         }
 
         pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
+        if (errno != 0)
+            goto error;
     }
 
-    VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name, capability);
+ error:
+    VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
+              dev->id, dev->name, capability, g_strerror(errno));
 
-    return 0;
+    /* reset errno in case the failure was due to insufficient
+     * privileges to read the entire PCI config file
+     */
+    errno = 0;
+
+    return -1;
 }
 
 static unsigned int
@@ -553,7 +572,7 @@ static bool
 virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
 {
     uint32_t caps;
-    uint8_t pos;
+    unsigned int pos;
     g_autofree char *path = NULL;
     int found;
 
@@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
      * the same thing, except for conventional PCI
      * devices. This is not common yet.
      */
-    pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
+    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+
     if (pos) {
         caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
         if (caps & PCI_AF_CAP_FLR) {
@@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
 static int
 virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
 {
-    dev->pcie_cap_pos   = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
-    dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
+    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP, &dev->pcie_cap_pos);
+    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM, &dev->pci_pm_cap_pos);
     dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
     dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
 
-- 
2.28.0