[libvirt] [PATCH v2 1/3] utils: Introduce functions for modprobe

[Daniel P. Berrange]

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 :|