[libvirt] [PATCH] pci: Give an explicit error if device not found
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/pci.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 81193b7..e1e11bc 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1028,6 +1028,7 @@ pciGetDevice(unsigned domain, unsigned function) { pciDevice *dev; + char devdir[PATH_MAX]; char *vendor, *product; if (VIR_ALLOC(dev) < 0) { @@ -1043,8 +1044,17 @@ pciGetDevice(unsigned domain, snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", dev->domain, dev->bus, dev->slot, dev->function); + snprintf(devdir, sizeof(devdir), + PCI_SYSFS "devices/%s", dev->name); snprintf(dev->path, sizeof(dev->path), - PCI_SYSFS "devices/%s/config", dev->name); + "%s/%s/config", devdir, dev->name); + + if (access(devdir, X_OK) != 0) { + pciReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s not found"), dev->name); + pciFreeDevice(dev); + return NULL; + } vendor = pciReadDeviceID(dev, "vendor"); product = pciReadDeviceID(dev, "device"); -- 1.7.0.1
On 04/30/2010 09:44 AM, Cole Robinson wrote:
@@ -1028,6 +1028,7 @@ pciGetDevice(unsigned domain, unsigned function) { pciDevice *dev; + char devdir[PATH_MAX];
Using PATH_MAX as an array size is dangerous; it fails on GNU Hurd where there is no minimum size. Also, it wastes a lot of space, given your usage...
char *vendor, *product;
if (VIR_ALLOC(dev) < 0) { @@ -1043,8 +1044,17 @@ pciGetDevice(unsigned domain,
snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", dev->domain, dev->bus, dev->slot, dev->function); + snprintf(devdir, sizeof(devdir), + PCI_SYSFS "devices/%s", dev->name);
...here, of concatenating two relatively short strings. I'd almost rather see a virAsprintf/free.
snprintf(dev->path, sizeof(dev->path), - PCI_SYSFS "devices/%s/config", dev->name); + "%s/%s/config", devdir, dev->name); + + if (access(devdir, X_OK) != 0) {
Is this ever run in a situation where the effective id might not equal the user id (that is, as a setuid script, or as root)? If so, would it be better to use faccessat(...,AT_EACCESS) instead of access() to be querying the correct permissions? (Gnulib provides faccessat for all platforms). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/04/2010 05:07 PM, Eric Blake wrote:
On 04/30/2010 09:44 AM, Cole Robinson wrote:
@@ -1028,6 +1028,7 @@ pciGetDevice(unsigned domain, unsigned function) { pciDevice *dev; + char devdir[PATH_MAX];
Using PATH_MAX as an array size is dangerous; it fails on GNU Hurd where there is no minimum size. Also, it wastes a lot of space, given your usage...
char *vendor, *product;
if (VIR_ALLOC(dev) < 0) { @@ -1043,8 +1044,17 @@ pciGetDevice(unsigned domain,
snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", dev->domain, dev->bus, dev->slot, dev->function); + snprintf(devdir, sizeof(devdir), + PCI_SYSFS "devices/%s", dev->name);
...here, of concatenating two relatively short strings. I'd almost rather see a virAsprintf/free.
snprintf(dev->path, sizeof(dev->path), - PCI_SYSFS "devices/%s/config", dev->name); + "%s/%s/config", devdir, dev->name); + + if (access(devdir, X_OK) != 0) {
Is this ever run in a situation where the effective id might not equal the user id (that is, as a setuid script, or as root)? If so, would it be better to use faccessat(...,AT_EACCESS) instead of access() to be querying the correct permissions? (Gnulib provides faccessat for all platforms).
Actually, my code was wrong, I just wanted to test for the existence of the file with F_OK, so sounds like faccessat isn't what I want in that case. I'm sending an updated patch which is also much simpler, and just checks dev->path rather than using a new variable. Thanks, Cole
participants (2)
-
Cole Robinson -
Eric Blake