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