[libvirt] [PATCH v2 0/5] Test our PCI device handling functions
The 4/5 is actually fix for a bug, Michal Privoznik (5): tests: Introduce virpcitest virpcitest: Test virPCIDeviceDetach virpcitest: Introduce testVirPCIDeviceReattach virpci: Don't error on not binded devices virpcitest: Introduce check for unbinded devices .gitignore | 1 + cfg.mk | 4 +- src/util/virpci.c | 12 +- tests/Makefile.am | 21 +- tests/virpcimock.c | 913 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/virpcitest.c | 204 ++++++++++++ 6 files changed, 1148 insertions(+), 7 deletions(-) create mode 100644 tests/virpcimock.c create mode 100644 tests/virpcitest.c -- 1.8.1.5
Among with this test introduce virpcimock as we need to mock some syscalls, e.g. redirect open() of a file under /sys/bus/pci to a stub sysfs tree. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- .gitignore | 1 + cfg.mk | 2 +- tests/Makefile.am | 13 +++ tests/virpcimock.c | 312 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/virpcitest.c | 103 ++++++++++++++++++ 5 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 tests/virpcimock.c create mode 100644 tests/virpcitest.c diff --git a/.gitignore b/.gitignore index e372876..6b024e7 100644 --- a/.gitignore +++ b/.gitignore @@ -212,6 +212,7 @@ /tests/virlockspacetest /tests/virlogtest /tests/virnet*test +/tests/virpcitest /tests/virportallocatortest /tests/virshtest /tests/virstoragetest diff --git a/cfg.mk b/cfg.mk index 1b2fd46..89b5613 100644 --- a/cfg.mk +++ b/cfg.mk @@ -946,7 +946,7 @@ exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/ exclude_file_name_regexp--sc_copyright_usage = \ ^COPYING(|\.LESSER)$$ -exclude_file_name_regexp--sc_flags_usage = ^(docs/|src/util/virnetdevtap\.c$$|tests/vircgroupmock\.c$$) +exclude_file_name_regexp--sc_flags_usage = ^(docs/|src/util/virnetdevtap\.c$$|tests/(vircgroup|virpci)mock\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) diff --git a/tests/Makefile.am b/tests/Makefile.am index 866ecd4..6d3245b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -123,6 +123,7 @@ test_programs = virshtest sockettest \ virauthconfigtest \ virbitmaptest \ vircgrouptest \ + virpcitest \ virendiantest \ viridentitytest \ virkeycodetest \ @@ -306,6 +307,7 @@ test_libraries = libshunload.la \ libvirportallocatormock.la \ virnetserverclientmock.la \ vircgroupmock.la \ + virpcimock.la \ $(NULL) if WITH_QEMU test_libraries += libqemumonitortestutils.la @@ -742,6 +744,17 @@ vircgroupmock_la_CFLAGS = $(AM_CFLAGS) vircgroupmock_la_LDFLAGS = -module -avoid-version \ -rpath /evil/libtool/hack/to/force/shared/lib/creation +virpcitest_SOURCES = \ + virpcitest.c testutils.h testutils.c +virpcitest_LDADD = $(LDADDS) + +virpcimock_la_SOURCES = \ + virpcimock.c +virpcimock_la_CFLAGS = $(AM_CFLAGS) +virpcimock_la_LIBADD = ../src/libvirt.la +virpcimock_la_LDFLAGS = -module -avoid-version \ + -rpath /evil/libtool/hack/to/force/shared/lib/creation + if WITH_DBUS virdbustest_SOURCES = \ virdbustest.c testutils.h testutils.c diff --git a/tests/virpcimock.c b/tests/virpcimock.c new file mode 100644 index 0000000..d545361 --- /dev/null +++ b/tests/virpcimock.c @@ -0,0 +1,312 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#ifdef __linux__ +# include "internal.h" +# include <stdio.h> +# include <dlfcn.h> +# include <stdlib.h> +# include <unistd.h> +# include <fcntl.h> +# include <sys/stat.h> +# include <stdarg.h> +# include "viralloc.h" +# include "virstring.h" +# include "virfile.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 int (*realopen)(const char *path, int flags, ...); + +/* Don't make static, since it causes problems with clang + * when passed as an arg to virAsprintf() + * vircgroupmock.c:462:22: error: static variable 'fakesysfsdir' is used in an inline function with external linkage [-Werror,-Wstatic-in-inline] + */ +char *fakesysfsdir; + +# define PCI_SYSFS_PREFIX "/sys/bus/pci/" + +# define STDERR(...) \ + fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__); \ + fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + +# define ABORT(...) \ + do { \ + STDERR(__VA_ARGS__); \ + abort(); \ + } while (0) + +# define ABORT_OOM() \ + ABORT("Out of memory") +/* + * The plan: + * + * 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. + */ + +/* + * + * Functions to model kernel behavior + * + */ + +struct pciDevice { + char *id; + int vendor; + int device; +}; + +struct pciDevice **pciDevices = NULL; +size_t pciDevices_size = 0; + +static void init_env(void); + +static void pci_device_new_from_stub(const struct pciDevice *data); + + +/* + * Helper functions + */ +static void +make_file(const char *path, + const char *name, + const char *value) +{ + int fd = -1; + char *filepath = NULL; + + if (virAsprintfQuiet(&filepath, "%s/%s", path, name) < 0) + ABORT_OOM(); + + if ((fd = realopen(filepath, O_CREAT|O_WRONLY, 0666)) < 0) + ABORT("Unable to open: %s", filepath); + + if (value && safewrite(fd, value, strlen(value)) != strlen(value)) + ABORT("Unable to write: %s", filepath); + + VIR_FORCE_CLOSE(fd); + VIR_FREE(filepath); +} + +static int +getrealpath(char **newpath, + const char *path) +{ + if (!fakesysfsdir) + init_env(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + if (virAsprintfQuiet(newpath, "%s/%s", + fakesysfsdir, + path + strlen(PCI_SYSFS_PREFIX)) < 0) { + errno = ENOMEM; + return -1; + } + } else { + if (VIR_STRDUP_QUIET(*newpath, path) < 0) + return -1; + } + + return 0; +} + + +/* + * PCI Device functions + */ +static void +pci_device_new_from_stub(const struct pciDevice *data) +{ + struct pciDevice *dev; + char *devpath; + char tmp[32]; + + if (VIR_ALLOC_QUIET(dev) < 0 || + virAsprintfQuiet(&devpath, "%s/devices/%s", fakesysfsdir, data->id) < 0) + ABORT_OOM(); + + memcpy(dev, data, sizeof(*dev)); + + if (virFileMakePath(devpath) < 0) + ABORT("Unable to create: %s", devpath); + + make_file(devpath, "config", "some dummy config"); + + if (snprintf(tmp, sizeof(tmp), "0x%.4x", dev->vendor) < 0) + ABORT("@tmp overflow"); + make_file(devpath, "vendor", tmp); + + if (snprintf(tmp, sizeof(tmp), "0x%.4x", dev->device) < 0) + ABORT("@tmp overflow"); + make_file(devpath, "device", tmp); + + if (VIR_APPEND_ELEMENT_QUIET(pciDevices, pciDevices_size, dev) < 0) + ABORT_OOM(); + + VIR_FREE(devpath); +} + + +/* + * Functions to load the symbols and init the environment + */ +static void +init_syms(void) +{ + if (realaccess) + return; + +# define LOAD_SYM(name) \ + do { \ + if (!(real ## name = dlsym(RTLD_NEXT, #name))) \ + ABORT("Cannot find real '%s' symbol\n", #name); \ + } while (0) + +# define LOAD_SYM_ALT(name1, name2) \ + do { \ + if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) && \ + !(real ## name2 = dlsym(RTLD_NEXT, #name2))) \ + ABORT("Cannot find real '%s' or '%s' symbol\n", \ + #name1, #name2); \ + } while (0) + + LOAD_SYM(access); + LOAD_SYM_ALT(lstat, __lxstat); + LOAD_SYM(open); +} + +static void +init_env(void) +{ + if (fakesysfsdir) + return; + + if (!(fakesysfsdir = getenv("LIBVIRT_FAKE_SYSFS_DIR"))) + ABORT("Missing LIBVIRT_FAKE_SYSFS_DIR env variable\n"); + +# define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \ + do { \ + struct pciDevice dev = {.id = (char *)Id, .vendor = Vendor, \ + .device = Device, __VA_ARGS__}; \ + pci_device_new_from_stub(&dev); \ + } while (0) + + MAKE_PCI_DEVICE("0000:00:00.0", 0x8086, 0x0044); +} + + +/* + * + * Mocked functions + * + */ + +int +access(const char *path, int mode) +{ + int ret; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + char *newpath; + if (getrealpath(&newpath, path) < 0) + return -1; + ret = realaccess(newpath, mode); + VIR_FREE(newpath); + } else { + ret = realaccess(path, mode); + } + return ret; +} + +int +__lxstat(int ver, const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + char *newpath; + if (getrealpath(&newpath, path) < 0) + return -1; + ret = real__lxstat(ver, newpath, sb); + VIR_FREE(newpath); + } else { + ret = real__lxstat(ver, path, sb); + } + return ret; +} + +int +lstat(const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + char *newpath; + if (getrealpath(&newpath, path) < 0) + return -1; + ret = reallstat(newpath, sb); + VIR_FREE(newpath); + } else { + ret = reallstat(path, sb); + } + return ret; +} + +int +open(const char *path, int flags, ...) +{ + int ret; + char *newpath = NULL; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX) && + getrealpath(&newpath, path) < 0) + return -1; + + if (flags & O_CREAT) { + va_list ap; + mode_t mode; + va_start(ap, flags); + mode = va_arg(ap, mode_t); + va_end(ap); + ret = realopen(newpath ? newpath : path, flags, mode); + } else { + ret = realopen(newpath ? newpath : path, flags); + } + VIR_FREE(newpath); + return ret; +} + +#else +/* Nothing to override on non-__linux__ platforms */ +#endif diff --git a/tests/virpcitest.c b/tests/virpcitest.c new file mode 100644 index 0000000..96f11d6 --- /dev/null +++ b/tests/virpcitest.c @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef __linux__ + +# include <stdlib.h> +# include <stdio.h> +# include <sys/types.h> +# include <sys/stat.h> +# include <fcntl.h> +# include <virpci.h> + +# define VIR_FROM_THIS VIR_FROM_NONE + +# define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX" +char *fakesysfsdir; + +static int +testVirPCIDeviceNew(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + virPCIDevicePtr dev; + const char *devName; + + if (!(dev = virPCIDeviceNew(0, 0, 0, 0))) + goto cleanup; + + devName = virPCIDeviceGetName(dev); + if (STRNEQ(devName, "0000:00:00.0")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "PCI device name mismatch: %s, expecting %s", + devName, "0000:00:00.0"); + goto cleanup; + } + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (VIR_STRDUP_QUIET(fakesysfsdir, FAKESYSFSDIRTEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mkdtemp(fakesysfsdir)) { + fprintf(stderr, "Cannot create fakesysfsdir"); + abort(); + } + + setenv("LIBVIRT_FAKE_SYSFS_DIR", fakesysfsdir, 1); + +# define DO_TEST(fnc) \ + do { \ + if (virtTestRun(#fnc, fnc, NULL) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST(testVirPCIDeviceNew); + + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + virFileDeleteTree(fakesysfsdir); + + VIR_FREE(fakesysfsdir); + + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virpcimock.so") +#else +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif -- 1.8.1.5
On Thu, Oct 31, 2013 at 11:23:38AM +0000, Michal Privoznik wrote:
Among with this test introduce virpcimock as we need to mock some syscalls, e.g. redirect open() of a file under /sys/bus/pci to a stub sysfs tree.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
OK, Now I noticed your v2, so I'm not going to repeat myself, just relate to the first one. This is the same as v1, so the same applies. Martin
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@redhat.com> --- cfg.mk | 2 +- tests/Makefile.am | 10 +- tests/virpcimock.c | 603 ++++++++++++++++++++++++++++++++++++++++++++++++++++- tests/virpcitest.c | 42 ++++ 4 files changed, 652 insertions(+), 5 deletions(-) diff --git a/cfg.mk b/cfg.mk index 89b5613..f210057 100644 --- a/cfg.mk +++ b/cfg.mk @@ -968,7 +968,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..2adc337 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -32,11 +32,16 @@ # 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 int (*realstat)(const char *path, struct stat *sb); +static int (*real__xstat)(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 +69,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 +101,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 */ +}; + 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 +173,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 +229,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 +323,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 +710,10 @@ init_syms(void) LOAD_SYM(access); LOAD_SYM_ALT(lstat, __lxstat); + LOAD_SYM_ALT(stat, __xstat); + LOAD_SYM(canonicalize_file_name); LOAD_SYM(open); + LOAD_SYM(close); } static void @@ -207,6 +725,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 +740,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); } @@ -282,6 +810,63 @@ lstat(const char *path, struct stat *sb) } int +__xstat(int ver, const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + char *newpath; + if (getrealpath(&newpath, path) < 0) + return -1; + ret = real__xstat(ver, newpath, sb); + VIR_FREE(newpath); + } else { + ret = real__xstat(ver, path, sb); + } + return ret; +} + +int +stat(const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (STRPREFIX(path, PCI_SYSFS_PREFIX)) { + char *newpath; + if (getrealpath(&newpath, path) < 0) + return -1; + ret = realstat(newpath, sb); + VIR_FREE(newpath); + } else { + ret = realstat(path, 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, ...) { int ret; @@ -303,10 +888,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; + } + VIR_FREE(newpath); return ret; } +int +close(int fd) +{ + if (remove_fd(fd) < 0) + return -1; + return realclose(fd); +} #else /* Nothing to override on non-__linux__ platforms */ #endif diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 96f11d6..3eaa469 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -60,6 +60,47 @@ cleanup: return ret; } +# define CHECK_LIST_COUNT(list, cnt) \ + if ((count = virPCIDeviceListCount(list)) != cnt) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Unexpected count of items in " #list ": %d, " \ + "expecting " #cnt, count); \ + goto cleanup; \ + } + +static int +testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + virPCIDevicePtr dev; + virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; + int count; + + if (!(dev = virPCIDeviceNew(0, 0, 1, 0)) || + !(activeDevs = virPCIDeviceListNew()) || + !(inactiveDevs = virPCIDeviceListNew())) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 0); + + if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) + goto cleanup; + + if (virPCIDeviceDetach(dev, activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 1); + + ret = 0; +cleanup: + virPCIDeviceFree(dev); + virObjectUnref(activeDevs); + virObjectUnref(inactiveDevs); + return ret; +} + static int mymain(void) { @@ -84,6 +125,7 @@ mymain(void) } while (0) DO_TEST(testVirPCIDeviceNew); + DO_TEST(testVirPCIDeviceDetach); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakesysfsdir); -- 1.8.1.5
On Thu, Oct 31, 2013 at 11:23:39AM +0000, 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@redhat.com> --- cfg.mk | 2 +- tests/Makefile.am | 10 +- tests/virpcimock.c | 603 ++++++++++++++++++++++++++++++++++++++++++++++++++++- tests/virpcitest.c | 42 ++++ 4 files changed, 652 insertions(+), 5 deletions(-)
[...]
diff --git a/tests/virpcimock.c b/tests/virpcimock.c index d545361..2adc337 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -163,12 +323,367 @@ pci_device_new_from_stub(const struct pciDevice *data) [...] +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; +}
One more newline here? I missed that in v1 ;-)
+static int +pci_driver_handle_new_id(const char *path) +{ [...] @@ -195,7 +710,10 @@ init_syms(void)
LOAD_SYM(access); LOAD_SYM_ALT(lstat, __lxstat); + LOAD_SYM_ALT(stat, __xstat);
I couldn't find what you need stat() for, but this works and makes no harm. Moreover it makes us sure there won't be any stat() done wrong in the future. [...]
diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 96f11d6..3eaa469 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -60,6 +60,47 @@ cleanup: return ret; }
+# define CHECK_LIST_COUNT(list, cnt) \ + if ((count = virPCIDeviceListCount(list)) != cnt) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Unexpected count of items in " #list ": %d, " \ + "expecting " #cnt, count); \ + goto cleanup; \ + } + +static int +testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + virPCIDevicePtr dev; + virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; + int count; + + if (!(dev = virPCIDeviceNew(0, 0, 1, 0)) || + !(activeDevs = virPCIDeviceListNew()) || + !(inactiveDevs = virPCIDeviceListNew())) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 0); + + if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) + goto cleanup; + + if (virPCIDeviceDetach(dev, activeDevs, inactiveDevs) < 0) + goto cleanup; +
virPCIDeviceDetach() adds a copy, I missed that, thanks. [...] It's almost the same as v1 (with one leak fixed and stat() mocking added), so the same as for v1 applies here. Martin
On Thu, Oct 31, 2013 at 11:23:39AM +0000, 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@redhat.com> --- cfg.mk | 2 +- tests/Makefile.am | 10 +- tests/virpcimock.c | 603 ++++++++++++++++++++++++++++++++++++++++++++++++++++- tests/virpcitest.c | 42 ++++ 4 files changed, 652 insertions(+), 5 deletions(-)
[...]
diff --git a/tests/virpcimock.c b/tests/virpcimock.c index d545361..2adc337 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c [...] +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; + } +
I spoke too soon, this function might check whether the driver is what the device is really binded to and error out if not.
+ ret = pci_driver_unbind(dev->driver, dev);
than this one can be called only with 'dev'. But since we are not testing/emulating faulty kernel behaviour, this is not necesarily needed. However it would be nice to have cleaned up so it's easier to read for others when adding more tests. Martin
On 10/31/2013 07:23 AM, 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@redhat.com> --- cfg.mk | 2 +- tests/Makefile.am | 10 +- tests/virpcimock.c | 603 ++++++++++++++++++++++++++++++++++++++++++++++++++++- tests/virpcitest.c | 42 ++++ 4 files changed, 652 insertions(+), 5 deletions(-)
Coverity has found a problem... <...snip...>
diff --git a/tests/virpcimock.c b/tests/virpcimock.c index d545361..2adc337 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c
<...snip...>
+/* + * 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); +
391 ABORT("Unable to create: %s", driverpath); 392 (5) Event va_init: Initializing va_list "args". Also see events: [missing_va_end] 393 va_start(args, name);
+ 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();
413 if (VIR_APPEND_ELEMENT_QUIET(pciDrivers, nPciDrivers, driver) < 0) 414 ABORT_OOM(); (15) Event missing_va_end: va_end was not called for "args". Also see events: [va_init] 415 } 416
This test will reattach the PCI device detached in the previous test. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 3eaa469..8811add 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -102,6 +102,42 @@ cleanup: } static int +testVirPCIDeviceReattach(const void *oaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + virPCIDevicePtr dev = NULL; + virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; + int count; + + if (!(dev = virPCIDeviceNew(0, 0, 1, 0)) || + !(activeDevs = virPCIDeviceListNew()) || + !(inactiveDevs = virPCIDeviceListNew())) + goto cleanup; + + if (virPCIDeviceListAdd(inactiveDevs, dev) < 0) { + virPCIDeviceFree(dev); + goto cleanup; + } + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 1); + + if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) + goto cleanup; + + if (virPCIDeviceReattach(dev, activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 0); + + ret = 0; +cleanup: + virObjectUnref(activeDevs); + virObjectUnref(inactiveDevs); + return ret; +} +static int mymain(void) { int ret = 0; @@ -126,6 +162,7 @@ mymain(void) DO_TEST(testVirPCIDeviceNew); DO_TEST(testVirPCIDeviceDetach); + DO_TEST(testVirPCIDeviceReattach); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakesysfsdir); -- 1.8.1.5
On Thu, Oct 31, 2013 at 11:23:40AM +0000, Michal Privoznik wrote:
This test will reattach the PCI device detached in the previous test.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
ACK, you could also check that the Reattach fails when the dev is also not reattached when active already ;-) Martin
If a PCI deivce is not binded to any driver (e.g. there's yet no PCI driver in the linux kernel) but still users want to passthru the device we fail the whole operation as we fail to resolve the 'driver' link under the PCI device sysfs tree. Obviously, this is not a fatal error and it shouldn't be error at all. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 65d7168..0727085 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1095,10 +1095,16 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *newDriverName = NULL; if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 || - virPCIFile(&driverLink, dev->name, "driver") < 0 || - virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, - &oldDriverName) < 0) { + virPCIFile(&driverLink, dev->name, "driver") < 0) goto cleanup; + + if (virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, + &oldDriverName) < 0) { + /* It's okay if device is not binded to any driver. If that's the case, + * there's no /sys/bus/pci/devices/.../driver symlink. */ + if (errno != ENOENT) + goto cleanup; + virResetLastError(); } if (virFileExists(driverLink)) { -- 1.8.1.5
On Thu, Oct 31, 2013 at 11:23:41AM +0000, Michal Privoznik wrote: s/not binded/unbinded/ in $SUBJ
If a PCI deivce is not binded to any driver (e.g. there's yet no PCI driver in the linux kernel) but still users want to passthru the device we fail the whole operation as we fail to resolve the 'driver' link under the PCI device sysfs tree. Obviously, this is not a fatal error and it shouldn't be error at all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 65d7168..0727085 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1095,10 +1095,16 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *newDriverName = NULL;
if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 || - virPCIFile(&driverLink, dev->name, "driver") < 0 || - virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, - &oldDriverName) < 0) { + virPCIFile(&driverLink, dev->name, "driver") < 0) goto cleanup; + + if (virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, + &oldDriverName) < 0) { + /* It's okay if device is not binded to any driver. If that's the case, + * there's no /sys/bus/pci/devices/.../driver symlink. */ + if (errno != ENOENT) + goto cleanup; + virResetLastError(); }
if (virFileExists(driverLink)) { -- 1.8.1.5
If you don't use the result of this function, you don't need to call it at all, because there is no code utilizing those oldDriver{Path,Name} variables. ACK if you remove the call to the function completely. Martin
At Mon, 4 Nov 2013 15:59:18 +0100, Martin Kletzander wrote:
On Thu, Oct 31, 2013 at 11:23:41AM +0000, Michal Privoznik wrote:
s/not binded/unbinded/ in $SUBJ
s/unbinded/unbound/ Somebody else with a _guess_? :-) -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On Tue, Nov 05, 2013 at 07:59:32AM +0100, Claudio Bley wrote:
At Mon, 4 Nov 2013 15:59:18 +0100, Martin Kletzander wrote:
On Thu, Oct 31, 2013 at 11:23:41AM +0000, Michal Privoznik wrote:
s/not binded/unbinded/ in $SUBJ
s/unbinded/unbound/
Somebody else with a _guess_? :-)
'unbound' is the correct usage Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/31/2013 01:23 PM, Michal Privoznik wrote:
If a PCI deivce is not binded to any driver (e.g. there's yet no PCI driver in the linux kernel) but still users want to passthru the device we fail the whole operation as we fail to resolve the 'driver' link under the PCI device sysfs tree. Obviously, this is not a fatal error and it shouldn't be error at all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 65d7168..0727085 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1095,10 +1095,16 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, const char *newDriverName = NULL;
if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 || - virPCIFile(&driverLink, dev->name, "driver") < 0 || - virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, - &oldDriverName) < 0) { + virPCIFile(&driverLink, dev->name, "driver") < 0) goto cleanup; + + if (virPCIDeviceGetDriverPathAndName(dev, &oldDriverPath, + &oldDriverName) < 0) { + /* It's okay if device is not binded to any driver. If that's the case, + * there's no /sys/bus/pci/devices/.../driver symlink. */ + if (errno != ENOENT) + goto cleanup; + virResetLastError();
The problem with fixing it like this is that the log will already have an error message, and we'll end up getting a bug report about it even though the operation is successful. I think this should instead be fixed by having virPCIDeviceDriverPathAndName() return success with NULL path and name in the case that the device isn't bound to anything, then check for that specific condition in all the callers of virPCIDeviceDriverPathAndName(), and change the behavior accordingly. In virPCIDeviceBindToStub and virPCIDeviceReset, no change would be needed. In virPCIDeviceUnbindFromStub, I'm not sure what would be the appropriate action - doing a "drivers_reprobe" might be correct, but depending on what led to the current state, that might end up re-binding the stub driver; it almost seems that what's needed in that case is to do the reprobe, then do the entire function a 2nd time in case the first try caused a re-bind to the stub (and then generate an error if it has failed after the 2nd time).
}
if (virFileExists(driverLink)) {
This just introduces the test for bug fixed in the previous patch. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 8811add..5fb35ce 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -72,11 +72,12 @@ static int testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) { int ret = -1; - virPCIDevicePtr dev; + virPCIDevicePtr dev = NULL, unbindedDev = NULL; virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; int count; if (!(dev = virPCIDeviceNew(0, 0, 1, 0)) || + !(unbindedDev = virPCIDeviceNew(0, 0, 3, 0)) || !(activeDevs = virPCIDeviceListNew()) || !(inactiveDevs = virPCIDeviceListNew())) goto cleanup; @@ -84,7 +85,8 @@ testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, 0); - if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) + if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0 || + virPCIDeviceSetStubDriver(unbindedDev, "pci-stub") < 0) goto cleanup; if (virPCIDeviceDetach(dev, activeDevs, inactiveDevs) < 0) @@ -93,9 +95,16 @@ testVirPCIDeviceDetach(const void *oaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, 1); + if (virPCIDeviceDetach(unbindedDev, activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 2); + ret = 0; cleanup: virPCIDeviceFree(dev); + virPCIDeviceFree(unbindedDev); virObjectUnref(activeDevs); virObjectUnref(inactiveDevs); return ret; @@ -105,22 +114,29 @@ static int testVirPCIDeviceReattach(const void *oaque ATTRIBUTE_UNUSED) { int ret = -1; - virPCIDevicePtr dev = NULL; + virPCIDevicePtr dev = NULL, unbindedDev = NULL; virPCIDeviceListPtr activeDevs = NULL, inactiveDevs = NULL; int count; if (!(dev = virPCIDeviceNew(0, 0, 1, 0)) || + !(unbindedDev = virPCIDeviceNew(0, 0, 3, 0)) || !(activeDevs = virPCIDeviceListNew()) || !(inactiveDevs = virPCIDeviceListNew())) goto cleanup; if (virPCIDeviceListAdd(inactiveDevs, dev) < 0) { virPCIDeviceFree(dev); + virPCIDeviceFree(unbindedDev); + goto cleanup; + } + + if (virPCIDeviceListAdd(inactiveDevs, unbindedDev) < 0) { + virPCIDeviceFree(unbindedDev); goto cleanup; } CHECK_LIST_COUNT(activeDevs, 0); - CHECK_LIST_COUNT(inactiveDevs, 1); + CHECK_LIST_COUNT(inactiveDevs, 2); if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) goto cleanup; @@ -129,6 +145,12 @@ testVirPCIDeviceReattach(const void *oaque ATTRIBUTE_UNUSED) goto cleanup; CHECK_LIST_COUNT(activeDevs, 0); + CHECK_LIST_COUNT(inactiveDevs, 1); + + if (virPCIDeviceReattach(unbindedDev, activeDevs, inactiveDevs) < 0) + goto cleanup; + + CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, 0); ret = 0; -- 1.8.1.5
On Thu, Oct 31, 2013 at 11:23:42AM +0000, Michal Privoznik wrote:
This just introduces the test for bug fixed in the previous patch.
I was once warned that "previous", "next" and "4/5" are not good pointers in a comit message as the commit might be cherry-picked, rebased or commited somewhere out-of-order and such references will thus be false. Might be better to use a commit name or hash, although using the second option, it is not very usable as well.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)
Other than that, ACK. Martin
participants (6)
-
Claudio Bley -
Daniel P. Berrange -
John Ferlan -
Laine Stump -
Martin Kletzander -
Michal Privoznik