7:50 a.m.
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(a)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