
Original patch here: http://www.redhat.com/archives/libvir-list/2014-January/msg01408.html Changes since v1: * Add virkmod.{c,h} to perform the various modprobe commands virModprobeConfig() -> modprobe -c virModprobeLoad() -> modprobe driver virModprobeUnload() -> modprobe -r driver virModprobeUseBlacklist() -> modprobe -b driver * virkmodtest.c to test the functions Caveat emptor... The config is the only test run by default. Loading and Unloading modules is something to be done carefully. * virpci.c Use the virModprobe*() API's instead of a direct MODPROBE * .gitignore Added the /tests/virkmodtest and /examples/domain-events/ which seems to have appeared recently. I can separate them out if desired. NOTE: Since I have no messages in virkmod.c, modifying po/POTFILES.in was deemed unnecessary by a 'make check-syntax' John Ferlan (3): utils: Introduce functions for modprobe tests: Add test for new virkmod functions Honor blacklist for modprobe command .gitignore | 2 + src/Makefile.am | 1 + src/libvirt_private.syms | 7 ++ src/util/virkmod.c | 138 ++++++++++++++++++++++++++++++++ src/util/virkmod.h | 35 +++++++++ src/util/virpci.c | 49 +++++++++--- tests/Makefile.am | 5 ++ tests/virkmodtest.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 429 insertions(+), 9 deletions(-) create mode 100644 src/util/virkmod.c create mode 100644 src/util/virkmod.h create mode 100644 tests/virkmodtest.c -- 1.8.4.2

This patch adds functions for various usages of modprobe Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 1 + src/libvirt_private.syms | 7 +++ src/util/virkmod.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virkmod.h | 35 ++++++++++++ 4 files changed, 181 insertions(+) create mode 100644 src/util/virkmod.c create mode 100644 src/util/virkmod.h diff --git a/src/Makefile.am b/src/Makefile.am index abe0a51..b704045 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -125,6 +125,7 @@ UTIL_SOURCES = \ util/virnetdevvportprofile.h util/virnetdevvportprofile.c \ util/virnetlink.c util/virnetlink.h \ util/virnodesuspend.c util/virnodesuspend.h \ + util/virkmod.c util/virkmod.h \ util/virnuma.c util/virnuma.h \ util/virobject.c util/virobject.h \ util/virpci.c util/virpci.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 45f3117..11fc320 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1382,6 +1382,13 @@ virKeyFileLoadFile; virKeyFileNew; +# util/virkmod.h +virModprobeConfig; +virModprobeLoad; +virModprobeUnload; +virModprobeUseBlacklist; + + # util/virlockspace.h virLockSpaceAcquireResource; virLockSpaceCreateResource; diff --git a/src/util/virkmod.c b/src/util/virkmod.c new file mode 100644 index 0000000..b8de8ea --- /dev/null +++ b/src/util/virkmod.c @@ -0,0 +1,138 @@ +/* + * virkmod.c: helper APIs for managing kernel modules + * + * Copyright (C) 2014 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/>. + * + */ + +#include <config.h> +#include "virkmod.h" +#include "vircommand.h" + +static int +doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf) +{ + int ret = -1; + virCommandPtr cmd = NULL; + + cmd = virCommandNew(MODPROBE); + if (opts) + virCommandAddArg(cmd, opts); + if (module) + virCommandAddArg(cmd, module); + if (outbuf) + virCommandSetOutputBuffer(cmd, outbuf); + if (errbuf) + virCommandSetErrorBuffer(cmd, errbuf); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/** + * virModprobeConfig: + * + * Get the current kernel module configuration + * + * Returns NULL on failure or a pointer to the output which + * must be VIR_FREE()'d by the caller + */ +char * +virModprobeConfig(void) +{ + char *outbuf = NULL; + + if (doModprobe("-c", NULL, &outbuf, NULL) < 0) + return NULL; + + return outbuf; +} + + +/** + * virModprobeLoad: + * @module: Name of the module to load regardless of being on blacklist + * + * Attempts to load a kernel module + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeLoad(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe(NULL, module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} + + +/** + * virModprobeUseBlacklist: + * @module: Name of the module to load applying blacklist lookup + * + * Like ModprobeLoad, except adhere to the blacklist if present + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeUseBlacklist(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe("-b", module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} + + +/** + * virModprobeUnload: + * @module: Name of the module to unload + * + * Remove or unload a module + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeUnload(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe("-r", module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} diff --git a/src/util/virkmod.h b/src/util/virkmod.h new file mode 100644 index 0000000..31b2663 --- /dev/null +++ b/src/util/virkmod.h @@ -0,0 +1,35 @@ +/* + * virkmod.h: helper APIs for managing kernel modprobe + * + * Copyright (C) 2014 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/>. + * + */ + +#ifndef __VIR_KMOD_H__ +# define __VIR_KMOD_H__ + +# include "internal.h" +# include "viralloc.h" + +char *virModprobeConfig(void); +char *virModprobeLoad(const char *) + ATTRIBUTE_NONNULL(1); +char *virModprobeUseBlacklist(const char *); + ATTRIBUTE_NONNULL(1) +char *virModprobeUnload(const char *) + ATTRIBUTE_NONNULL(1); +#endif /* __VIR_KMOD_H__ */ -- 1.8.4.2

On Wed, Jan 29, 2014 at 07:52:43PM -0500, John Ferlan wrote:
diff --git a/src/util/virkmod.c b/src/util/virkmod.c new file mode 100644 index 0000000..b8de8ea --- /dev/null +++ b/src/util/virkmod.c @@ -0,0 +1,138 @@ +/* + * virkmod.c: helper APIs for managing kernel modules + * + * Copyright (C) 2014 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/>. + * + */ + +#include <config.h> +#include "virkmod.h" +#include "vircommand.h" + +static int +doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf) +{ + int ret = -1; + virCommandPtr cmd = NULL; + + cmd = virCommandNew(MODPROBE); + if (opts) + virCommandAddArg(cmd, opts); + if (module) + virCommandAddArg(cmd, module); + if (outbuf) + virCommandSetOutputBuffer(cmd, outbuf); + if (errbuf) + virCommandSetErrorBuffer(cmd, errbuf); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/** + * virModprobeConfig: + * + * Get the current kernel module configuration + * + * Returns NULL on failure or a pointer to the output which + * must be VIR_FREE()'d by the caller + */ +char * +virModprobeConfig(void) +{ + char *outbuf = NULL; + + if (doModprobe("-c", NULL, &outbuf, NULL) < 0) + return NULL; + + return outbuf; +} + + +/** + * virModprobeLoad: + * @module: Name of the module to load regardless of being on blacklist + * + * Attempts to load a kernel module + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeLoad(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe(NULL, module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} + + +/** + * virModprobeUseBlacklist: + * @module: Name of the module to load applying blacklist lookup + * + * Like ModprobeLoad, except adhere to the blacklist if present + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeUseBlacklist(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe("-b", module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +}
I think we could probably just have a 'bool useBlacklist' parameter for the virModeprobeLoad method - we'll likely want all callers to use the blacklist anyway, so not much need to have separate methods for each usage.
+ + +/** + * virModprobeUnload: + * @module: Name of the module to unload + * + * Remove or unload a module + * + * returns NULL in case of success and the error buffer output from the + * virCommandRun() on failure. The returned buffer must be VIR_FREE() + * by the caller + */ +char * +virModprobeUnload(const char *module) +{ + char *errbuf = NULL; + + if (doModprobe("-r", module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +}
Oh actually this reminds me of a recent firewalld bug. Basically 'modprobe -r' has utterly broken semantics - it will recursively unload any modules that were dependancies of the one you're removing even if things still require them ! eg it'll see the 'bridge' module has refcount of 0 and remove it, even if you have bridges created on the host :-( 'rmmod modname' is what you actually want to use.
+#ifndef __VIR_KMOD_H__ +# define __VIR_KMOD_H__ + +# include "internal.h" +# include "viralloc.h"
Put the viralloc.h in the .c file, since the header doesn't do need any memory allocation APIs.
+ +char *virModprobeConfig(void); +char *virModprobeLoad(const char *) + ATTRIBUTE_NONNULL(1); +char *virModprobeUseBlacklist(const char *); + ATTRIBUTE_NONNULL(1) +char *virModprobeUnload(const char *) + ATTRIBUTE_NONNULL(1); +#endif /* __VIR_KMOD_H__ */
Nitpick on naming - general goal is to have function names match the filename. eg so we should use virKModLoad / virKModUnload 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 :|

Adding tests for new virModprobe{Config|Load|Unload|UseBlacklist} API's. Other than the virModprobeConfig() calls, the tests require some special set up and to be run from root. Signed-off-by: John Ferlan <jferlan@redhat.com> --- .gitignore | 2 + tests/Makefile.am | 5 ++ tests/virkmodtest.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+) create mode 100644 tests/virkmodtest.c diff --git a/.gitignore b/.gitignore index 96b7211..79437e9 100644 --- a/.gitignore +++ b/.gitignore @@ -70,6 +70,7 @@ /docs/libvirt-refs.xml /docs/search.php /docs/todo.html.in +/examples/domain-events/ /examples/object-events/event-test /examples/dominfo/info1 /examples/domsuspend/suspend @@ -202,6 +203,7 @@ /tests/viridentitytest /tests/virkeycodetest /tests/virkeyfiletest +/tests/virkmodtest /tests/virlockspacetest /tests/virlogtest /tests/virnet*test diff --git a/tests/Makefile.am b/tests/Makefile.am index 0930073..632c953 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -142,6 +142,7 @@ test_programs = virshtest sockettest \ sysinfotest \ virstoragetest \ virnetdevbandwidthtest \ + virkmodtest \ $(NULL) if WITH_REMOTE @@ -655,6 +656,10 @@ virnetdevbandwidthtest_SOURCES = \ virnetdevbandwidthtest.c testutils.h testutils.c virnetdevbandwidthtest_LDADD = $(LDADDS) +virkmodtest_SOURCES = \ + virkmodtest.c testutils.h testutils.c +virkmodtest_LDADD = $(LDADDS) + if WITH_LIBVIRTD libvirtdconftest_SOURCES = \ libvirtdconftest.c testutils.h testutils.c \ diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c new file mode 100644 index 0000000..8957e2c --- /dev/null +++ b/tests/virkmodtest.c @@ -0,0 +1,201 @@ +/* + * Copyright (C) 2014 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/>. + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef __linux__ + +# include <stdlib.h> +# include <stdio.h> +# include <virkmod.h> +# include <virstring.h> + +# define VIR_FROM_THIS VIR_FROM_NONE + +/* Defining MODPROBE_CONFIG_VFIO_PCI to 1 will allow this test to run + * through the module load, unload, and blacklist check API's. Doing + * so assumes that vfio-pci is present and available on your system + * and it's been placed on the blacklist. You must also run as root + * since the calls use /sbin/modprobe to load/unload a module. + * + * The load/unload tests should succeed and prove that libvirt wasn't + * checking for the blacklist. + * + * The use blacklist test follows the logic in virPCIProbeStubDriver() + * where an attempt to load a blacklist driver will result in the load + * appearing to succeed, but then finding the driver on the blacklist. + * + * To place 'vfio-pci' on the blacklist for a Fedora system create + * "/etc/modparams.d/blacklist-vfio-pci.conf" and within the file + * add the line "blacklist vfio-pci". + */ +# define MODPROBE_CONFIG_VFIO_PCI 0 + +static int +testModprobeConfig(const void *args ATTRIBUTE_UNUSED) +{ + int ret = -1; + char *outbuf = NULL; + + /* This will return the contents of a 'modprobe -c' which can differ + * from machine to machine - be happy that we get something. + */ + outbuf = virModprobeConfig(); + if (!outbuf) { + fprintf(stderr, "Failed to get config\n"); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(outbuf); + return ret; +} + + +# if MODPROBE_CONFIG_VFIO_PCI +static int +testModprobeLoad(const void *args) +{ + int ret = -1; + char *errbuf = NULL; + const char *module = args; + + errbuf = virModprobeLoad(module); + if (errbuf) { + fprintf(stderr, "Failed to load, error: %s\n", errbuf); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(errbuf); + return ret; +} + + +static int +testModprobeUnload(const void *args) +{ + int ret = -1; + char *errbuf = NULL; + const char *module = args; + + errbuf = virModprobeUnload(module); + if (errbuf) { + fprintf(stderr, "Failed to unload, error: %s\n", errbuf); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(errbuf); + return ret; +} + + +static int +testModprobeUseBlacklist(const void *args) +{ + int ret = -1; + size_t i; + char *outbuf = NULL; + char *errbuf = NULL; + char *drvblklst = NULL; + const char *module = args; + + errbuf = virModprobeUseBlacklist(module); + if (errbuf) { + fprintf(stderr, "Failed to load, error: %s\n", errbuf); + goto cleanup; + } + + /* All error path code - purpose is to determine whether the failure + * occurs because device is on blacklist in order to add an error + * message to help detect why load failed + */ + if (virAsprintf(&drvblklst, "blacklist %s", module) < 0) + goto cleanup; + + /* modprobe will convert all '-' into '_', so we need to as well */ + for (i = 0; i < drvblklst[i]; i++) + if (drvblklst[i] == '-') + drvblklst[i] = '_'; + + outbuf = virModprobeConfig(); + if (!outbuf) + goto cleanup; + + /* Find module on blacklist? */ + if (!strstr(outbuf, drvblklst)) { + fprintf(stderr, "Failed to find %s on blacklist\n", module); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(drvblklst); + VIR_FREE(outbuf); + VIR_FREE(errbuf); + if (ret == -1) { + errbuf = virModprobeUnload(module); + if (errbuf) + fprintf(stderr, "Failed to unload, error: %s\n", errbuf); + VIR_FREE(errbuf); + } + return ret; +} +# endif /* MODPROBE_CONFIG_VFIO_PCI*/ + + +static int +mymain(void) +{ + int ret = 0; +# if MODPROBE_CONFIG_VFIO_PCI + const char *module = "vfio-pci"; +# endif + + if (virtTestRun("modprobe config", testModprobeConfig, NULL) < 0) + ret = -1; +# if MODPROBE_CONFIG_VFIO_PCI + if (virtTestRun("modprobe load", testModprobeLoad, module) < 0) + ret = -1; + if (virtTestRun("modprobe unload", testModprobeUnload, module) < 0) + ret = -1; + if (virtTestRun("modprobe blacklist", testModprobeUseBlacklist, module) < 0) + ret = -1; +# endif /* MODPROBE_CONFIG_VFIO_PCI */ + + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; + +} + +VIRT_TEST_MAIN(mymain); +#else +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif -- 1.8.4.2

On Wed, Jan 29, 2014 at 07:52:44PM -0500, John Ferlan wrote:
Adding tests for new virModprobe{Config|Load|Unload|UseBlacklist} API's. Other than the virModprobeConfig() calls, the tests require some special set up and to be run from root.
The unit tests need to be isolated from the host OS entirely. For the load/unload APIs we could use the newly introduced 'virCommandSetDryRun' API to let us test them. Basically we only want to validate that the API is invoking the right set of commands / args. We don't actually want to affect our host kernel / os. Regards, 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 01/30/2014 06:58 AM, Daniel P. Berrange wrote:
On Wed, Jan 29, 2014 at 07:52:44PM -0500, John Ferlan wrote:
Adding tests for new virModprobe{Config|Load|Unload|UseBlacklist} API's. Other than the virModprobeConfig() calls, the tests require some special set up and to be run from root.
The unit tests need to be isolated from the host OS entirely. For the load/unload APIs we could use the newly introduced 'virCommandSetDryRun' API to let us test them. Basically we only want to validate that the API is invoking the right set of commands / args. We don't actually want to affect our host kernel / os.
I hadn't followed the virCommandSetDryRun() discussion. I certainly was uncomfortable with adding the load/unload tests since yes, the host would be affected, but I still wanted a way to be able to ensure that they worked in a "controlled environment". The other hurdle with using the new API is the downstream changes requiring yet another API... John

https://bugzilla.redhat.com/show_bug.cgi?id=1045124 When loading modules, libvirt does not honor the modprobe blacklist. By adding a "-b" to the modprobe command libvirt will fail to load a module if it's on the blacklist Check if the failure to load a driver was due to it being on the blacklist using the output of a "modprobe -c" searching for "blacklist <driver_Name>" where driver_Name is possibly a modified string of the input driver changing all '-' into '_' since that's what modprobe does. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..5d4168a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -42,6 +42,7 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "virkmod.h" #include "virstring.h" #include "virutil.h" @@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver) { char *drvpath = NULL; bool probed = false; + size_t i; + char *drvblklst = NULL; + char *outbuf = NULL; recheck: if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) { @@ -990,18 +994,44 @@ recheck: VIR_FREE(drvpath); if (!probed) { - const char *const probecmd[] = { MODPROBE, driver, NULL }; + char *errbuf = NULL; probed = true; - if (virRun(probecmd, NULL) < 0) { - char ebuf[1024]; - VIR_WARN("failed to load driver %s: %s", driver, - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; + if ((errbuf = virModprobeUseBlacklist(driver))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf); + VIR_FREE(errbuf); + goto cleanup; } goto recheck; } + /* All error path code - purpose is to determine whether the failure + * occurs because device is on blacklist in order to add an error + * message to help detect why load failed + */ + if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0) + goto cleanup; + + /* modprobe will convert all '-' into '_', so we need to as well */ + for (i = 0; i < drvblklst[i]; i++) + if (drvblklst[i] == '-') + drvblklst[i] = '_'; + + outbuf = virModprobeConfig(); + if (!outbuf) + goto cleanup; + + /* Find driver on blacklist? */ + if (strstr(outbuf, drvblklst)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } + +cleanup: + VIR_FREE(drvblklst); + VIR_FREE(outbuf); return -1; } @@ -1313,9 +1343,10 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *inactiveDevs) { if (virPCIProbeStubDriver(dev->stubDriver) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to load PCI stub module %s"), - dev->stubDriver); + if (virGetLastError() == NULL) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s"), + dev->stubDriver); return -1; } -- 1.8.4.2

On 01/30/2014 01:52 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1045124
When loading modules, libvirt does not honor the modprobe blacklist. By adding a "-b" to the modprobe command libvirt will fail to load a module if it's on the blacklist
Check if the failure to load a driver was due to it being on the blacklist using the output of a "modprobe -c" searching for "blacklist <driver_Name>" where driver_Name is possibly a modified string of the input driver changing all '-' into '_' since that's what modprobe does.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..5d4168a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -42,6 +42,7 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "virkmod.h" #include "virstring.h" #include "virutil.h"
@@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver) { char *drvpath = NULL; bool probed = false; + size_t i; + char *drvblklst = NULL; + char *outbuf = NULL;
recheck: if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) { @@ -990,18 +994,44 @@ recheck: VIR_FREE(drvpath);
if (!probed) { - const char *const probecmd[] = { MODPROBE, driver, NULL }; + char *errbuf = NULL; probed = true; - if (virRun(probecmd, NULL) < 0) { - char ebuf[1024]; - VIR_WARN("failed to load driver %s: %s", driver, - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; + if ((errbuf = virModprobeUseBlacklist(driver))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf); + VIR_FREE(errbuf); + goto cleanup; }
goto recheck; }
+ /* All error path code - purpose is to determine whether the failure + * occurs because device is on blacklist in order to add an error + * message to help detect why load failed + */ + if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0) + goto cleanup; + + /* modprobe will convert all '-' into '_', so we need to as well */ + for (i = 0; i < drvblklst[i]; i++) + if (drvblklst[i] == '-') + drvblklst[i] = '_'; + + outbuf = virModprobeConfig(); + if (!outbuf) + goto cleanup; + + /* Find driver on blacklist? */ + if (strstr(outbuf, drvblklst)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } +
IIUC the reason this check is here and not in virkmod.c is that 'modprobe -b <module>' on a blacklisted module returns 0 and no output. Could we check if the module was loaded successfully inside virModprobeLoad, perhaps by checking if the /sys/module/<module> directory exists? Jan

On Wed, Jan 29, 2014 at 07:52:45PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1045124
When loading modules, libvirt does not honor the modprobe blacklist. By adding a "-b" to the modprobe command libvirt will fail to load a module if it's on the blacklist
Check if the failure to load a driver was due to it being on the blacklist using the output of a "modprobe -c" searching for "blacklist <driver_Name>" where driver_Name is possibly a modified string of the input driver changing all '-' into '_' since that's what modprobe does.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..5d4168a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -42,6 +42,7 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "virkmod.h" #include "virstring.h" #include "virutil.h"
@@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver) { char *drvpath = NULL; bool probed = false; + size_t i; + char *drvblklst = NULL; + char *outbuf = NULL;
recheck: if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) { @@ -990,18 +994,44 @@ recheck: VIR_FREE(drvpath);
if (!probed) { - const char *const probecmd[] = { MODPROBE, driver, NULL }; + char *errbuf = NULL; probed = true; - if (virRun(probecmd, NULL) < 0) { - char ebuf[1024]; - VIR_WARN("failed to load driver %s: %s", driver, - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; + if ((errbuf = virModprobeUseBlacklist(driver))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf); + VIR_FREE(errbuf); + goto cleanup; }
goto recheck; }
+ /* All error path code - purpose is to determine whether the failure + * occurs because device is on blacklist in order to add an error + * message to help detect why load failed + */ + if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0) + goto cleanup; + + /* modprobe will convert all '-' into '_', so we need to as well */ + for (i = 0; i < drvblklst[i]; i++) + if (drvblklst[i] == '-') + drvblklst[i] = '_'; + + outbuf = virModprobeConfig(); + if (!outbuf) + goto cleanup; + + /* Find driver on blacklist? */ + if (strstr(outbuf, drvblklst)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + }
So when I suggested creation of virkmod.{c,h} I was really meaning that all this code would go in there. eg here you'd just invoke if (virKModIsBlacklisted(driver)) { ....report error... } so all the kmod config parsing would be isolated from this pci code. Regards, 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 01/30/2014 07:05 AM, Daniel P. Berrange wrote:
On Wed, Jan 29, 2014 at 07:52:45PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1045124
When loading modules, libvirt does not honor the modprobe blacklist. By adding a "-b" to the modprobe command libvirt will fail to load a module if it's on the blacklist
Check if the failure to load a driver was due to it being on the blacklist using the output of a "modprobe -c" searching for "blacklist <driver_Name>" where driver_Name is possibly a modified string of the input driver changing all '-' into '_' since that's what modprobe does.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..5d4168a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -42,6 +42,7 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "virkmod.h" #include "virstring.h" #include "virutil.h"
@@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver) { char *drvpath = NULL; bool probed = false; + size_t i; + char *drvblklst = NULL; + char *outbuf = NULL;
recheck: if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) { @@ -990,18 +994,44 @@ recheck: VIR_FREE(drvpath);
if (!probed) { - const char *const probecmd[] = { MODPROBE, driver, NULL }; + char *errbuf = NULL; probed = true; - if (virRun(probecmd, NULL) < 0) { - char ebuf[1024]; - VIR_WARN("failed to load driver %s: %s", driver, - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; + if ((errbuf = virModprobeUseBlacklist(driver))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf); + VIR_FREE(errbuf); + goto cleanup; }
goto recheck; }
+ /* All error path code - purpose is to determine whether the failure + * occurs because device is on blacklist in order to add an error + * message to help detect why load failed + */ + if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0) + goto cleanup; + + /* modprobe will convert all '-' into '_', so we need to as well */ + for (i = 0; i < drvblklst[i]; i++) + if (drvblklst[i] == '-') + drvblklst[i] = '_'; + + outbuf = virModprobeConfig(); + if (!outbuf) + goto cleanup; + + /* Find driver on blacklist? */ + if (strstr(outbuf, drvblklst)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + }
So when I suggested creation of virkmod.{c,h} I was really meaning that all this code would go in there. eg here you'd just invoke
if (virKModIsBlacklisted(driver)) { ....report error... }
so all the kmod config parsing would be isolated from this pci code.
So here's the rub (since Jan asked and probably figured it out) - 'modprobe -b' returns 0 and no error when it finds the module on the blacklist. I guess I had "brain lock" over removing the VIR_WARN() and already having gone through a round of testing by the bz submitter in a "real" environment with the original set of changes. This was so much simpler when all that was originally asked for was a "-b" to be added to the MODPROBE command :-)... A v3 will come soon. John
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Ján Tomko