On 02/04/2014 06:06 AM, Ján Tomko wrote:

On 01/30/2014 06:50 PM, John Ferlan wrote:

...

This patch adds functions for various usages of modprobe

Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 6 ++ src/Makefile.am | 1 + src/libvirt_private.syms | 7 ++ src/util/virkmod.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virkmod.h | 34 +++++++++ 5 files changed, 230 insertions(+) create mode 100644 src/util/virkmod.c create mode 100644 src/util/virkmod.h

diff --git a/configure.ac b/configure.ac index 1670a41..eb11e3b 100644 --- a/configure.ac +++ b/configure.ac @@ -405,6 +405,8 @@ AC_PATH_PROG([UDEVSETTLE], [udevsettle], [], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([MODPROBE], [modprobe], [modprobe], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([RMMOD], [rmmod], [rmmod], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([SCRUB], [scrub], [scrub], @@ -433,6 +435,10 @@ if test -n "$MODPROBE"; then AC_DEFINE_UNQUOTED([MODPROBE],["$MODPROBE"], [Location or name of the modprobe program]) fi +if test -n "$RMMOD"; then + AC_DEFINE_UNQUOTED([RMMOD],["$RMMOD"], + [Location or name of the rmmod program]) +fi AC_DEFINE_UNQUOTED([SCRUB],["$SCRUB"], [Location or name of the scrub program (for wiping algorithms)])

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..372231c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1382,6 +1382,13 @@ virKeyFileLoadFile; virKeyFileNew;

+# util/virkmod.h +virKModConfig;

Do we need to export KModConfig as well?

It's called from testKModConfig() as well, so while not necessarily for this specific patch, it will be for 2/3. For me it's less churn/file changes for each patch.

...

+virKModIsBlacklisted; +virKModLoad; +virKModUnload; + + # util/virlockspace.h virLockSpaceAcquireResource; virLockSpaceCreateResource; diff --git a/src/util/virkmod.c b/src/util/virkmod.c new file mode 100644 index 0000000..d39a2e3 --- /dev/null +++ b/src/util/virkmod.c @@ -0,0 +1,182 @@ +/* + * 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 "viralloc.h" +#include "virkmod.h" +#include "vircommand.h" +#include "virstring.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; +} + +static int +doRmmod(const char *module, char **errbuf) +{ + int ret = -1; + virCommandPtr cmd = NULL; + + cmd = virCommandNewArgList(RMMOD, module, NULL); + virCommandSetErrorBuffer(cmd, errbuf); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/** + * virKModConfig: + * + * 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 * +virKModConfig(void) +{ + char *outbuf = NULL; + + if (doModprobe("-c", NULL, &outbuf, NULL) < 0) + return NULL; + + return outbuf; +} + + +/** + * virKModLoad: + * @module: Name of the module to load + * @useBlacklist: True if honoring 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 + */

It would be nicer if these functions could detect if the module was loaded and report an error.

Loading an already loaded module via modprobe doesn't produce an error. Attempting to load a blacklisted module doesn't produce an error. Loading a non existing module produces an error. So what error would you be expecting? On the unload side there is a difference between using rmmod and modprobe -r... The former will produce an error for an already unloaded module, while the latter doesn't. I'd have to add an 'lsmod' and search the output for the module name, but in then end the error becomes the same. That is you cannot unload an already unloaded module, so to that end I'm not sure what error checking buys us. Side note - modprobe does things "one way" and my experience with various OS's I've worked on is that each OS seems to have their own mechanism and rules surrounding load/unload of modules - so I'm hesitant to add too much logic in this space. Rather let the OS handle it.

...

+char * +virKModLoad(const char *module, bool useBlacklist) +{ + char *errbuf = NULL; + + if (doModprobe(useBlacklist ? "-b" : NULL, module, NULL, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} + + +/** + * virKModUnload: + * @module: Name of the module to unload + * + * Remove or unload a module. + * + * NB: Do not use 'modprobe -r' here as that code will recursively + * unload any modules that were dependancies of the one being removed + * even if things still require them. e.g. it'll see the 'bridge' + * module has refcount of 0 and remove it, even if there are bridges + * created on the host + * + * 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 * +virKModUnload(const char *module) +{ + char *errbuf = NULL; + + if (doRmmod(module, &errbuf) < 0) + return errbuf; + + VIR_FREE(errbuf); + return NULL; +} + + +/** + * virKModIsBlacklisted: + * @module: Name of the module to check for on the blacklist + * + * Search the output of the configuration data for the module being + * blacklisted. + * + * returns true when found blacklisted, false otherwise. + */ +bool +virKModIsBlacklisted(const char *module) +{ + bool retval = false; + size_t i; + char *drvblklst = NULL; + char *outbuf = NULL; + + if (virAsprintfQuiet(&drvblklst, "blacklist %s", module) < 0)

I would add a trailing '\n' here.

OK - good idea just in case there's "blacklist foo" and "blacklist foobar".

...

+ 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] = '_'; + + if (doModprobe("-c", NULL, &outbuf, NULL) < 0) + goto cleanup; + + /* Find module on blacklist? */

s/Find/Found/

Contextually it's "Did I find the module on the blacklist?" Although it's pretty obvious what's being done I think so I'll avoid the language semantics. Thanks for the review John

...

+ if (strstr(outbuf, drvblklst)) + retval = true; + +cleanup: + VIR_FREE(drvblklst); + VIR_FREE(outbuf); + return retval; +}

ACK

Jan

On 01/30/2014 12:50 PM, John Ferlan wrote:

Adding tests for new virKMod{Config|Load|Unload}() API's.

A test for virKModIsBlacklisted() would require some setup which cannot be assumed.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- .gitignore | 2 +

<...snip...>

diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c new file mode 100644 index 0000000..6225a9e --- /dev/null +++ b/tests/virkmodtest.c @@ -0,0 +1,183 @@ +/* + * 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 "vircommand.h" +# include "virkmod.h" +# include "virstring.h" + +struct testInfo { + const char *module; + const char *exp_cmd; + bool useBlacklist; +}; + +# define VIR_FROM_THIS VIR_FROM_NONE + +static int +testKModConfig(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 = virKModConfig(); + if (!outbuf) { + fprintf(stderr, "Failed to get config\n"); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(outbuf); + return ret; +} + + +static int +checkOutput(virBufferPtr buf, const char *exp_cmd) +{ + int ret = -1; + char *actual_cmd = NULL; + + if (!(actual_cmd = virBufferContentAndReset(buf))) { + int err = virBufferError(buf); + if (err) + fprintf(stderr, "buffer's in error state: %d", err); + else + fprintf(stderr, "cannot compare buffer to exp: %s", exp_cmd); + goto cleanup; + } + + if (STRNEQ(exp_cmd, actual_cmd)) { + virtTestDifference(stderr, exp_cmd, actual_cmd); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(actual_cmd); + return ret; +} + + +static int +testKModLoad(const void *args) +{ + int ret = -1; + char *errbuf = NULL; + const struct testInfo *info = args; + const char *module = info->module; + bool useBlacklist = info->useBlacklist; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCommandSetDryRun(&buf); + + errbuf = virKModLoad(module, useBlacklist); + if (errbuf) { + fprintf(stderr, "Failed to load, error: %s\n", errbuf); + goto cleanup; + } + + if (checkOutput(&buf, info->exp_cmd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandSetDryRun(NULL); + VIR_FREE(errbuf); + return ret; +} + + +static int +testKModUnload(const void *args) +{ + int ret = -1; + char *errbuf = NULL; + const struct testInfo *info = args; + const char *module = info->module; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCommandSetDryRun(&buf); + + errbuf = virKModUnload(module); + if (errbuf) { + fprintf(stderr, "Failed to unload, error: %s\n", errbuf); + goto cleanup; + } + + if (checkOutput(&buf, info->exp_cmd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandSetDryRun(NULL); + VIR_FREE(errbuf); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("config", testKModConfig, NULL) < 0) + ret = -1; + + /* Although we cannot run the command on the host, we can compare + * the output of the created command against what we'd expect to be + * created. So let's at least do that. + */ +# define DO_TEST(_name, _cb, _blkflag, _exp_cmd) \ + do { \ + struct testInfo data = {.module = "vfio-pci", \ + .exp_cmd = _exp_cmd, \ + .useBlacklist = _blkflag}; \ + if (virtTestRun(_name, _cb, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST("load", testKModLoad, false, "/sbin/modprobe vfio-pci\n"); + DO_TEST("unload", testKModUnload, false, "/sbin/rmmod vfio-pci\n"); + DO_TEST("blklist", testKModLoad, true, "/sbin/modprobe -b vfio-pci\n"); + + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; + +} + +VIRT_TEST_MAIN(mymain); +#else +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif

Considering : http://www.redhat.com/archives/libvir-list/2014-January/msg01556.html The following is squashed in: diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c index 6225a9e..c6f5a72 100644 --- a/tests/virkmodtest.c +++ b/tests/virkmodtest.c @@ -165,9 +165,9 @@ mymain(void) ret = -1; \ } while (0) - DO_TEST("load", testKModLoad, false, "/sbin/modprobe vfio-pci\n"); - DO_TEST("unload", testKModUnload, false, "/sbin/rmmod vfio-pci\n"); - DO_TEST("blklist", testKModLoad, true, "/sbin/modprobe -b vfio-pci\n"); + DO_TEST("load", testKModLoad, false, MODPROBE " vfio-pci\n"); + DO_TEST("unload", testKModUnload, false, RMMOD " vfio-pci\n"); + DO_TEST("blklist", testKModLoad, true, MODPROBE " -b vfio-pci\n"); return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; John

On 02/04/2014 06:06 AM, Ján Tomko wrote:

On 01/30/2014 06:50 PM, John Ferlan wrote:

...

Adding tests for new virKMod{Config|Load|Unload}() API's.

A test for virKModIsBlacklisted() would require some setup which cannot be assumed.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- .gitignore | 2 + tests/Makefile.am | 5 ++ tests/virkmodtest.c | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 190 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

Unrelated change.

OK - I'll send this separately (trivially). John

On 01/30/2014 06:50 PM, John Ferlan wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1045124

When loading modules, libvirt does not honor the modprobe blacklist. Use the new virKModLoad() API in order to attempt load with blacklist check. Use the new virKModIsBlacklisted() API to check if the failure to load was due to the blacklist

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..41735eb 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"

@@ -990,18 +991,26 @@ 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 = virKModLoad(driver, true))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf); + VIR_FREE(errbuf); + goto cleanup; }

goto recheck; }

+ /* If we know failure was because of blacklist, let's report that */ + if (virKModIsBlacklisted(driver)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } + +cleanup: return -1; }

@@ -1313,9 +1322,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)

This seems to be the only caller of virPCIProbeStubDriver. You could just report the error unconditionally inside it.

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s"), + dev->stubDriver); return -1; }

Jan

On 02/04/2014 03:29 PM, John Ferlan wrote:

On 02/04/2014 06:06 AM, Ján Tomko wrote:

...

On 01/30/2014 06:50 PM, John Ferlan wrote:

...

+ VIR_FREE(errbuf); + goto cleanup; }

goto recheck; }

+ /* If we know failure was because of blacklist, let's report that */ + if (virKModIsBlacklisted(driver)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } + +cleanup: return -1; }

@@ -1313,9 +1322,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)

This seems to be the only caller of virPCIProbeStubDriver. You could just report the error unconditionally inside it.

Attempting to make the differentiation between load failed load failed because of administratively prohibited means an additional check or two in the caller.

I meant that right now virPCIProbeStubDriver is only called from here and if it did not report an error, we will report one here. It seemed cleaner not to report an error here and make virPCIProbeStubDriver report an error in all cases (not just when the module is blacklisted and/or on OOM in virPCIDriverDir).

Furthermore if something that virPCIProbeStubDriver() called provided some other error wouldn't it be better to not overwrite the message? If the virAsprintf() called by virPCIDriverDir() failed because of memory allocation, then which error message would be displayed without the virGetLastError() check? I guess I'm not 100% clear in my mind which error message would get displayed...

Only the last reported error gets displayed, but both will get logged. Jan

On 02/04/2014 04:29 PM, John Ferlan wrote:

On 02/04/2014 10:10 AM, Ján Tomko wrote:

...

On 02/04/2014 03:29 PM, John Ferlan wrote:

...

On 02/04/2014 06:06 AM, Ján Tomko wrote:

...

On 01/30/2014 06:50 PM, John Ferlan wrote:

...

+ VIR_FREE(errbuf); + goto cleanup; }

goto recheck; }

+ /* If we know failure was because of blacklist, let's report that */ + if (virKModIsBlacklisted(driver)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } + +cleanup: return -1; }

@@ -1313,9 +1322,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)

This seems to be the only caller of virPCIProbeStubDriver. You could just report the error unconditionally inside it.

Attempting to make the differentiation between load failed load failed because of administratively prohibited means an additional check or two in the caller.

I meant that right now virPCIProbeStubDriver is only called from here and if it did not report an error, we will report one here.

It seemed cleaner not to report an error here and make virPCIProbeStubDriver report an error in all cases (not just when the module is blacklisted and/or on OOM in virPCIDriverDir).

oh - ah... light dawns on marblehead.

...

...

Furthermore if something that virPCIProbeStubDriver() called provided some other error wouldn't it be better to not overwrite the message? If the virAsprintf() called by virPCIDriverDir() failed because of memory allocation, then which error message would be displayed without the virGetLastError() check? I guess I'm not 100% clear in my mind which error message would get displayed...

Only the last reported error gets displayed, but both will get logged.

Jan

The "last" message is what was important to someone using virt-install

so a "git diff master src/util/virpci.c" then has the following... Basically an adjustment cleanup: label in order to handle both error conditions and removal of the error message from the caller (if you want to see a v4 I'll send it, but hopefully this is OK):

diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..c3d211f 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"

@@ -990,18 +991,32 @@ 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 = virKModLoad(driver, true))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf);

Can you do virReportError here with the contents of errbuf (and return instead of jumping to cleanup)?

+ VIR_FREE(errbuf); + goto cleanup; }

goto recheck; }

+cleanup: + /* If we know failure was because of blacklist, let's report that; + * otherwise, report a more generic failure message + */ + if (virKModIsBlacklisted(driver)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s"), + driver); + } + return -1; }

@@ -1312,12 +1327,8 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { - if (virPCIProbeStubDriver(dev->stubDriver) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to load PCI stub module %s"), - dev->stubDriver); + if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1; - }

if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR,

ACK Jan