Re: [PATCH 2/3] virpcitest: Test virPCIDeviceDetach

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 25, 2013 at 02:03:42PM +0100, Michal Privoznik wrote:
> This commit introduces yet another test under virpcitest:
> virPCIDeviceDetach. However, in order to be able to do this, the
> virpcimock needs to be extended to model the kernel behavior on PCI
> device binding and unbinding (create 'driver' symlinks under the device
> tree, check for device ID in driver's ID table, etc.)
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  cfg.mk             |   2 +-
>  tests/Makefile.am  |  10 +-
>  tests/virpcimock.c | 562 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  tests/virpcitest.c |  44 +++++
>  4 files changed, 613 insertions(+), 5 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 15114a9..306b052 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -964,7 +964,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \
>    ^(docs/|examples/|python/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
>  
>  exclude_file_name_regexp--sc_prohibit_close = \
> -  (\.p[yl]$$|^docs/|^(src/util/virfile\.c|src/libvirt\.c|tests/vircgroupmock\.c)$$)
> +  (\.p[yl]$$|^docs/|^(src/util/virfile\.c|src/libvirt\.c|tests/vir(cgroup|pci)mock\.c)$$)
>  
>  exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
>    (^tests/(qemuhelp|nodeinfo)data/|\.(gif|ico|png|diff)$$)
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 6d3245b..2c2e5a9 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -48,12 +48,15 @@ if WITH_DTRACE_PROBES
>  PROBES_O += ../src/libvirt_probes.lo
>  endif WITH_DTRACE_PROBES
>  
> +GNULIB_LIBS = \
> +       ../gnulib/lib/libgnu.la
> +
>  LDADDS = \
>          $(WARN_CFLAGS) \
>  	$(NO_INDIRECT_LDFLAGS) \
>  	$(PROBES_O) \
> -	../src/libvirt.la \
> -	../gnulib/lib/libgnu.la
> +	$(GNULIB_LIBS) \
> +	../src/libvirt.la
>  
>  EXTRA_DIST =		\
>  	capabilityschemadata \
> @@ -751,7 +754,8 @@ virpcitest_LDADD = $(LDADDS)
>  virpcimock_la_SOURCES = \
>  	virpcimock.c
>  virpcimock_la_CFLAGS = $(AM_CFLAGS)
> -virpcimock_la_LIBADD = ../src/libvirt.la
> +virpcimock_la_LIBADD = $(GNULIB_LIBS) \
> +					   ../src/libvirt.la
>  virpcimock_la_LDFLAGS = -module -avoid-version \
>          -rpath /evil/libtool/hack/to/force/shared/lib/creation
>  
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index d545361..16a5ff3 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -32,11 +32,14 @@
>  # include "viralloc.h"
>  # include "virstring.h"
>  # include "virfile.h"
> +# include "dirname.h"
>  
>  static int (*realaccess)(const char *path, int mode);
>  static int (*reallstat)(const char *path, struct stat *sb);
>  static int (*real__lxstat)(int ver, const char *path, struct stat *sb);
> +static char *(*realcanonicalize_file_name)(const char *path);
>  static int (*realopen)(const char *path, int flags, ...);
> +static int (*realclose)(int fd);
>  
>  /* Don't make static, since it causes problems with clang
>   * when passed as an arg to virAsprintf()
> @@ -64,7 +67,30 @@ char *fakesysfsdir;
>   *
>   * Mock some file handling functions. Redirect them into a stub tree passed via
>   * LIBVIRT_FAKE_SYSFS_DIR env variable. All files and links within stub tree is
> - * created by us.
> + * created by us. There are some actions that we must take if some special
> + * files are written to. Here's the list of files we watch:
> + *
> + * /sys/bus/pci/drivers/<driver>/new_id:
> + *   Learn the driver new vendor and device combination.
> + *   Data in format "VVVV DDDD".
> + *
> + * /sys/bus/pci/drivers/<driver>/remove_id
> + *   Make the driver forget about vendor and device.
> + *   Data in format "VVVV DDDD".
> + *
> + * /sys/bus/pci/drivers/<driver>/bind
> + *   Check if driver supports the device and bind driver to it (create symlink
> + *   called 'driver' pointing to the /sys/but/pci/drivers/<driver>).
> + *   Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function).
> + *
> + * /sys/bus/pci/drivers/<driver>/unbind
> + *   Unbind driver from the device.
> + *   Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function).
> + *
> + * As a little hack, we are not mocking write to these files, but close()
> + * instead. The advantage is we don't need any self growing array to hold the
> + * partial writes and construct them back. We can let all the writes finish,
> + * and then just read the file content back.
>   */
>  
>  /*
> @@ -73,18 +99,51 @@ char *fakesysfsdir;
>   *
>   */
>  
> +struct pciDriver {
> +    char *name;
> +    int *vendor;        /* List of vendor:device IDs the driver can handle */
> +    int *device;
> +    size_t len;            /* @len is used for both @vendor and @device */
> +};
> +

Don't you want to use few typedefs to save a bunch of 'struct'
keywords?

>  struct pciDevice {
>      char *id;
>      int vendor;
>      int device;
> +    struct pciDriver *driver;   /* Driver attached. NULL if attached to no driver */
> +};
> +
> +struct fdCallback {
> +    int fd;
> +    char *path;
>  };
>  
>  struct pciDevice **pciDevices = NULL;
>  size_t pciDevices_size = 0;
>  
> +struct pciDriver **pciDrivers = NULL;
> +size_t pciDrivers_size = 0;
> +
> +struct fdCallback *callbacks = NULL;
> +size_t callbacks_size = 0;
> +
>  static void init_env(void);
>  
> +static int pci_device_autobind(struct pciDevice *dev);
>  static void pci_device_new_from_stub(const struct pciDevice *data);
> +static struct pciDevice *pci_device_find_by_id(const char *id);
> +static struct pciDevice *pci_device_find_by_content(const char *path);
> +
> +static void pci_driver_new(const char *name, ...);
> +static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev);
> +static struct pciDriver *pci_driver_find_by_path(const char *path);
> +static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev);
> +static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev);
> +static int pci_driver_handle_change(int fd, const char *path);
> +static int pci_driver_handle_bind(const char *path);
> +static int pci_driver_handle_unbind(const char *path);
> +static int pci_driver_handle_new_id(const char *path);
> +static int pci_driver_handle_remove_id(const char *path);
>  
>  
>  /*
> @@ -112,6 +171,41 @@ make_file(const char *path,
>  }
>  
>  static int
> +pci_read_file(const char *path,
> +              char *buf,
> +              size_t buf_size)
> +{
> +    int ret = -1;
> +    int fd = -1;
> +    char *newpath;
> +
> +    if (virAsprintfQuiet(&newpath, "%s/%s",
> +                         fakesysfsdir,
> +                         path + strlen(PCI_SYSFS_PREFIX)) < 0) {
> +        errno = ENOMEM;
> +        goto cleanup;
> +    }
> +
> +    if ((fd = realopen(newpath, O_RDWR)) < 0)
> +        goto cleanup;
> +
> +    bzero(buf, buf_size);
> +    if (saferead(fd, buf, buf_size - 1) < 0) {
> +        STDERR("Unable to read from %s", newpath);
> +        goto cleanup;
> +    }
> +
> +    if (ftruncate(fd, 0) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(newpath);
> +    realclose(fd);
> +    return ret;
> +}
> +
> +static int
>  getrealpath(char **newpath,
>              const char *path)
>  {
> @@ -133,6 +227,70 @@ getrealpath(char **newpath,
>      return 0;
>  }
>  
> +static bool
> +find_fd(int fd, size_t *indx)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < callbacks_size; i++) {
> +        if (callbacks[i].fd == fd) {
> +            if (indx)
> +                *indx = i;
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static int
> +add_fd(int fd, const char *path)
> +{
> +    int ret = -1;
> +    size_t i;
> +
> +    if (find_fd(fd, &i)) {
> +        struct fdCallback cb = callbacks[i];
> +        ABORT("FD %d %s already present in the array as %d %s",
> +              fd, path, cb.fd, cb.path);
> +    }
> +
> +    if (VIR_REALLOC_N_QUIET(callbacks, callbacks_size + 1) < 0 ||
> +        VIR_STRDUP_QUIET(callbacks[callbacks_size].path, path) < 0) {
> +        errno = ENOMEM;
> +        goto cleanup;
> +    }
> +
> +    callbacks[callbacks_size++].fd = fd;
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +static int
> +remove_fd(int fd)
> +{
> +    int ret = -1;
> +    size_t i;
> +
> +    if (find_fd(fd, &i)) {
> +        struct fdCallback cb = callbacks[i];
> +
> +        if (pci_driver_handle_change(cb.fd, cb.path) < 0)
> +            goto cleanup;
> +
> +        VIR_FREE(cb.path);
> +        if (VIR_DELETE_ELEMENT(callbacks, i, callbacks_size) < 0) {
> +            errno = EINVAL;
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
>  
>  /*
>   * PCI Device functions
> @@ -163,12 +321,367 @@ pci_device_new_from_stub(const struct pciDevice *data)
>          ABORT("@tmp overflow");
>      make_file(devpath, "device", tmp);
>  
> +    if (pci_device_autobind(dev) < 0)
> +        ABORT("Unable to bind: %s", data->id);
> +
>      if (VIR_APPEND_ELEMENT_QUIET(pciDevices, pciDevices_size, dev) < 0)
>          ABORT_OOM();
>  
>      VIR_FREE(devpath);
>  }
>  
> +static struct pciDevice *
> +pci_device_find_by_id(const char *id)
> +{
> +    size_t i;
> +    for (i = 0; i < pciDevices_size; i++) {
> +        struct pciDevice *dev = pciDevices[i];
> +
> +        if (STREQ(dev->id, id))
> +            return dev;
> +    }
> +
> +    return NULL;
> +}
> +
> +static struct pciDevice *
> +pci_device_find_by_content(const char *path)
> +{
> +    char tmp[32];
> +
> +    if (pci_read_file(path, tmp, sizeof(tmp)) < 0)
> +        return NULL;
> +
> +    return pci_device_find_by_id(tmp);
> +}
> +
> +static int
> +pci_device_autobind(struct pciDevice *dev)
> +{
> +    struct pciDriver *driver = pci_driver_find_by_dev(dev);
> +
> +    if (!driver) {
> +        /* No driver found. Nothing to do */
> +        return 0;
> +    }
> +
> +    return pci_driver_bind(driver, dev);
> +}
> +
> +
> +/*
> + * PCI Driver functions
> + */
> +static void
> +pci_driver_new(const char *name, ...)
> +{
> +    struct pciDriver *driver;
> +    va_list args;
> +    int vendor, device;
> +    char *driverpath;
> +
> +    if (VIR_ALLOC_QUIET(driver) < 0 ||
> +        VIR_STRDUP_QUIET(driver->name, name) < 0 ||
> +        virAsprintfQuiet(&driverpath, "%s/drivers/%s", fakesysfsdir, name) < 0)
> +        ABORT_OOM();
> +
> +    if (virFileMakePath(driverpath) < 0)
> +        ABORT("Unable to create: %s", driverpath);
> +
> +    va_start(args, name);
> +
> +    while ((vendor = va_arg(args, int)) != -1) {
> +        if ((device = va_arg(args, int)) == -1)
> +            ABORT("Invalid vendor device pair for driver %s", name);
> +
> +        if (VIR_REALLOC_N_QUIET(driver->vendor, driver->len + 1) < 0 ||
> +            VIR_REALLOC_N_QUIET(driver->device, driver->len + 1) < 0)
> +            ABORT_OOM();
> +
> +        driver->vendor[driver->len] = vendor;
> +        driver->device[driver->len] = device;
> +        driver->len++;
> +    }
> +
> +    make_file(driverpath, "bind", NULL);
> +    make_file(driverpath, "unbind", NULL);
> +    make_file(driverpath, "new_id", NULL);
> +    make_file(driverpath, "remove_id", NULL);
> +
> +    if (VIR_APPEND_ELEMENT_QUIET(pciDrivers, pciDrivers_size, driver) < 0)
> +        ABORT_OOM();
> +}
> +
> +static struct pciDriver *
> +pci_driver_find_by_dev(struct pciDevice *dev)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < pciDrivers_size; i++) {
> +        struct pciDriver *driver = pciDrivers[i];
> +        size_t j;
> +
> +        for (j = 0; j < driver->len; j++) {
> +            if (driver->vendor[j] == dev->vendor &&
> +                driver->device[j] == dev->device)
> +                return driver;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static struct pciDriver *
> +pci_driver_find_by_path(const char *path)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < pciDrivers_size; i++) {
> +        struct pciDriver *driver = pciDrivers[i];
> +
> +        if (strstr(path, driver->name))
> +            return driver;
> +    }
> +
> +    return NULL;
> +}
> +
> +static int
> +pci_driver_bind(struct pciDriver *driver,
> +                struct pciDevice *dev)
> +{
> +    int ret = -1;
> +    char *devpath = NULL, *driverpath = NULL;
> +
> +    if (dev->driver) {
> +        /* Device already binded */
> +        errno = ENODEV;
> +        return ret;
> +    }
> +
> +    /* Make symlink under device tree */
> +    if (virAsprintfQuiet(&devpath, "%s/devices/%s/driver",
> +                         fakesysfsdir, dev->id) < 0 ||
> +        virAsprintfQuiet(&driverpath, "%s/drivers/%s",
> +                         fakesysfsdir, driver->name) < 0) {
> +        errno = ENOMEM;
> +        goto cleanup;
> +    }
> +
> +    if (symlink(driverpath, devpath) < 0)
> +        goto cleanup;
> +
> +    /* Make symlink under driver tree */
> +    if (virAsprintfQuiet(&devpath, "%s/devices/%s",
> +                         fakesysfsdir, dev->id) < 0 ||
> +        virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s",
> +                         fakesysfsdir, driver->name, dev->id) < 0) {
> +        errno = ENOMEM;
> +        goto cleanup;
> +    }
> +
> +    if (symlink(devpath, driverpath) < 0)
> +        goto cleanup;
> +
> +    dev->driver = driver;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(devpath);
> +    VIR_FREE(driverpath);
> +    return ret;
> +}
> +
> +static int
> +pci_driver_unbind(struct pciDriver *driver,
> +                  struct pciDevice *dev)
> +{
> +    int ret = -1;
> +    char *devpath = NULL, *driverpath = NULL;
> +
> +    if (!dev->driver) {
> +        /* Device not binded */
> +        errno = ENODEV;
> +        return ret;
> +    }
> +
> +    /* Make symlink under device tree */
> +    if (virAsprintfQuiet(&devpath, "%s/devices/%s/driver",
> +                         fakesysfsdir, dev->id) < 0 ||
> +        virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s",
> +                         fakesysfsdir, driver->name, dev->id) < 0) {
> +        errno = ENOMEM;
> +        goto cleanup;
> +    }
> +
> +    if (unlink(devpath) < 0 ||
> +        unlink(driverpath) < 0)
> +        goto cleanup;
> +
> +    dev->driver = NULL;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(devpath);
> +    VIR_FREE(driverpath);
> +    return ret;
> +}
> +
> +static int
> +pci_driver_handle_change(int fd ATTRIBUTE_UNUSED, const char *path)
> +{
> +    int ret;
> +    const char *file = last_component(path);
> +
> +    if (STREQ(file, "bind")) {
> +        /* handle write to bind */
> +        ret = pci_driver_handle_bind(path);
> +    } else if (STREQ(file, "unbind")) {
> +        /* handle write to unbind */
> +        ret = pci_driver_handle_unbind(path);
> +    } else if (STREQ(file, "new_id")) {
> +        /* handle write to new_id */
> +        ret = pci_driver_handle_new_id(path);
> +    } else if (STREQ(file, "remove_id")) {
> +        /* handle write to remove_id */
> +        ret = pci_driver_handle_remove_id(path);
> +    } else {
> +        /* yet not handled write */
> +        ABORT("Not handled write to: %s", path);
> +    }
> +    return ret;
> +}
> +
> +static int
> +pci_driver_handle_bind(const char *path)
> +{
> +    int ret = -1;
> +    struct pciDevice *dev = pci_device_find_by_content(path);
> +    struct pciDriver *driver = pci_driver_find_by_path(path);
> +
> +    if (!driver || !dev) {
> +        /* This should never happen (TM) */
> +        errno = ENODEV;
> +        goto cleanup;
> +    }
> +
> +    ret = pci_driver_bind(driver, dev);
> +cleanup:
> +    return ret;
> +}
> +
> +static int
> +pci_driver_handle_unbind(const char *path)
> +{
> +    int ret = -1;
> +    struct pciDevice *dev = pci_device_find_by_content(path);
> +
> +    if (!dev || !dev->driver) {
> +        /* This should never happen (TM) */
> +        errno = ENODEV;
> +        goto cleanup;
> +    }
> +
> +    ret = pci_driver_unbind(dev->driver, dev);
> +cleanup:
> +    return ret;
> +}
> +static int
> +pci_driver_handle_new_id(const char *path)
> +{
> +    int ret = -1;
> +    struct pciDriver *driver = pci_driver_find_by_path(path);
> +    size_t i;
> +    int vendor, device;
> +    char buf[32];
> +
> +    if (!driver) {
> +        /* This should never happen (TM) */
> +        errno = ENODEV;
> +        goto cleanup;
> +    }
> +
> +    if (pci_read_file(path, buf, sizeof(buf)) < 0)
> +        goto cleanup;
> +
> +    if (sscanf(buf, "%x %x", &vendor, &device) < 2) {
> +        errno = EINVAL;
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < driver->len; i++) {
> +        if (driver->vendor[i] == vendor &&
> +            driver->device[i] == device)
> +            break;
> +    }
> +
> +    if (i == driver->len) {
> +        if (VIR_REALLOC_N_QUIET(driver->vendor, driver->len + 1) < 0 ||
> +            VIR_REALLOC_N_QUIET(driver->device, driver->len + 1) < 0) {
> +            errno = ENOMEM;
> +            goto cleanup;
> +        }
> +
> +        driver->vendor[driver->len] = vendor;
> +        driver->device[driver->len] = device;
> +        driver->len++;
> +    }
> +
> +    for (i = 0; i < pciDevices_size; i++) {
> +        struct pciDevice *dev = pciDevices[i];
> +
> +        if (!dev->driver &&
> +            dev->vendor == vendor &&
> +            dev->device == device &&
> +            pci_driver_bind(driver, dev) < 0)
> +                goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +static int
> +pci_driver_handle_remove_id(const char *path)
> +{
> +    int ret = -1;
> +    struct pciDriver *driver = pci_driver_find_by_path(path);
> +    size_t i;
> +    int vendor, device;
> +    char buf[32];
> +
> +    if (!driver) {
> +        /* This should never happen (TM) */
> +        errno = ENODEV;
> +        goto cleanup;
> +    }
> +
> +    if (pci_read_file(path, buf, sizeof(buf)) < 0)
> +        goto cleanup;
> +
> +    if (sscanf(buf, "%x %x", &vendor, &device) < 2) {
> +        errno = EINVAL;
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < driver->len; i++) {
> +        if (driver->vendor[i] == vendor &&
> +            driver->device[i] == device)
> +            continue;
> +    }
> +
> +    if (i != driver->len) {
> +        if (VIR_DELETE_ELEMENT(driver->vendor, i, driver->len) < 0)
> +            goto cleanup;
> +        driver->len++;
> +        if (VIR_DELETE_ELEMENT(driver->device, i, driver->len) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
>  
>  /*
>   * Functions to load the symbols and init the environment
> @@ -195,7 +708,9 @@ init_syms(void)
>  
>      LOAD_SYM(access);
>      LOAD_SYM_ALT(lstat, __lxstat);
> +    LOAD_SYM(canonicalize_file_name);
>      LOAD_SYM(open);
> +    LOAD_SYM(close);
>  }
>  
>  static void
> @@ -207,6 +722,13 @@ init_env(void)
>      if (!(fakesysfsdir = getenv("LIBVIRT_FAKE_SYSFS_DIR")))
>          ABORT("Missing LIBVIRT_FAKE_SYSFS_DIR env variable\n");
>  
> +# define MAKE_PCI_DRIVER(name, ...)                                     \
> +    pci_driver_new(name, __VA_ARGS__, -1, -1)
> +
> +    MAKE_PCI_DRIVER("iwlwifi", 0x8086, 0x0044);
> +    MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047);
> +    MAKE_PCI_DRIVER("pci-stub", -1, -1);
> +
>  # define MAKE_PCI_DEVICE(Id, Vendor, Device, ...)                       \
>      do {                                                                \
>          struct pciDevice dev = {.id = (char *)Id, .vendor = Vendor,     \
> @@ -215,6 +737,9 @@ init_env(void)
>      } while (0)
>  
>      MAKE_PCI_DEVICE("0000:00:00.0", 0x8086, 0x0044);
> +    MAKE_PCI_DEVICE("0000:00:01.0", 0x8086, 0x0044);
> +    MAKE_PCI_DEVICE("0000:00:02.0", 0x8086, 0x0046);
> +    MAKE_PCI_DEVICE("0000:00:03.0", 0x8086, 0x0048);
>  }
>  
>  
> @@ -281,6 +806,25 @@ lstat(const char *path, struct stat *sb)
>      return ret;
>  }
>  
> +char *
> +canonicalize_file_name(const char *path)
> +{
> +    char *ret;
> +
> +    init_syms();
> +
> +    if (STRPREFIX(path, PCI_SYSFS_PREFIX)) {
> +        char *newpath;
> +        if (getrealpath(&newpath, path) < 0)
> +            return NULL;
> +        ret = realcanonicalize_file_name(newpath);
> +        VIR_FREE(newpath);
> +    } else {
> +        ret = realcanonicalize_file_name(path);
> +    }
> +    return ret;
> +}
> +
>  int
>  open(const char *path, int flags, ...)
>  {
> @@ -303,10 +847,26 @@ open(const char *path, int flags, ...)
>      } else {
>          ret = realopen(newpath ? newpath : path, flags);
>      }
> +
> +    /* Catch both: /sys/bus/pci/drivers/... and
> +     * /sys/bus/pci/device/.../driver/... */
> +    if (ret >= 0 && STRPREFIX(path, PCI_SYSFS_PREFIX) &&
> +        strstr(path, "driver") && add_fd(ret, path) < 0) {
> +        realclose(ret);
> +        ret = -1;
> +    }
> +

Wouldn't it be safer to canonicalize the path (which should be done in
other functions anyway, since there could be a call to
'open("/sys//bus/pci/...", ...)', which this code would miss?  Not
that we're doing this somewhere right now, but there's no performance
impact when it is done in tests and it's a bit future-safer.

[...]

Other than that, this patch looks OK, so ACK (conditional; when [1/3]
gets in).

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]