[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 06/16] Add a test suite for cgroups functionality



On 04/10/2013 04:08 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Some aspects of the cgroups setup / detection code are quite subtle
> and easy to break. It would greatly benefit from unit testing, but
> this is difficult because the test suite won't have privileges to
> play around with cgroups. The solution is to use monkey patching
> via LD_PRELOAD to override the fopen, open, mkdir, access functions
> to redirect access of cgroups files to some magic stubs in the
> test suite.
> 
> Using this we provide custom content for the /proc/cgroup and
> /proc/self/mounts files which report a fixed cgroup setup. We
> then override open/mkdir/access so that access to the cgroups
> filesystem gets redirected into files in a temporary directory
> tree in the test suite build dir.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  .gitignore            |   1 +
>  cfg.mk                |  11 +-
>  tests/Makefile.am     |  15 +-
>  tests/vircgroupmock.c | 453 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/vircgrouptest.c | 249 +++++++++++++++++++++++++++
>  5 files changed, 723 insertions(+), 6 deletions(-)
>  create mode 100644 tests/vircgroupmock.c
>  create mode 100644 tests/vircgrouptest.c

Now that I'm reading this earlier in the day, I can give the full review
that I promised...

>  
>  exclude_file_name_regexp--sc_prohibit_asprintf = \
> -  ^(bootstrap.conf$$|src/util/virutil\.c$$|examples/domain-events/events-c/event-test\.c$$)
> +  ^(bootstrap.conf$$|src/util/virutil\.c$$|examples/domain-events/events-c/event-test\.c$$|tests/vircgroupmock\.c$$)

raw asprintf - yep, all the more reason to make sure this test compiles
only on Linux.

> +++ b/tests/Makefile.am
> @@ -97,7 +97,9 @@ test_programs = virshtest sockettest \
>  	utiltest shunloadtest \
>  	virtimetest viruritest virkeyfiletest \
>  	virauthconfigtest \
> -	virbitmaptest virendiantest \
> +	virbitmaptest \
> +	vircgrouptest \
> +	virendiantest \
>  	viridentitytest \
>  	virkeycodetest \
>  	virlockspacetest \
> @@ -247,6 +249,7 @@ EXTRA_DIST += $(test_scripts)
>  
>  test_libraries = libshunload.la \
>  		libvirportallocatormock.la \
> +		vircgroupmock.la \
>  		$(NULL)

shunload.c is guarded by #ifdef linux; you should do the same for
vircgroupmock.c.

>  
> +vircgrouptest_SOURCES = \
> +	vircgrouptest.c testutils.h testutils.c
> +vircgrouptest_LDADD = $(LDADDS)
> +
> +vircgroupmock_la_SOURCES = \
> +	vircgroupmock.c
> +vircgroupmock_la_CFLAGS = $(AM_CFLAGS) -DMOCK_HELPER=1

Why do you need -DMOCK_HELPER=1?  Too much copy and past from
virportallocatortest.c, which crammed both the mock library and the test
into the same file rather than your approach here of using two files?

> +++ b/tests/vircgroupmock.c

> +#include <config.h>
> +
> +#include "internal.h"
> +
> +#include <stdio.h>
> +#include <dlfcn.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +
> +static int (*realopen)(const char *path, int flags, ...);

Don't you need <stdarg.h> to make it possible to implement your
realopen()? [1]

> +/*
> + * Intentionally missing the 'devices' mount.
> + * Co-mounting cpu & cpuacct controllers
> + * An anonymous controller for systemd

Should give good coverage of the various virCgroup actions.

> + */
> +const char *mounts =
> +    "rootfs / rootfs rw 0 0\n"
> +    "tmpfs /run tmpfs rw,seclabel,nosuid,nodev,mode=755 0 0\n"
> +    "tmpfs /not/really/sys/fs/cgroup tmpfs rw,seclabel,nosuid,nodev,noexec,mode=755 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/systemd cgroup rw,nosuid,nodev,noexec,relatime,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/cpuset cgroup rw,nosuid,nodev,noexec,relatime,cpuset 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/cpu,cpuacct cgroup rw,nosuid,nodev,noexec,relatime,cpuacct,cpu 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/freezer cgroup rw,nosuid,nodev,noexec,relatime,freezer 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/blkio cgroup rw,nosuid,nodev,noexec,relatime,blkio 0 0\n"
> +    "cgroup /not/really/sys/fs/cgroup/memory cgroup rw,nosuid,nodev,noexec,relatime,memory 0 0\n"
> +    "/dev/sda1 /boot ext4 rw,seclabel,relatime,data=ordered 0 0\n"
> +    "tmpfs /tmp tmpfs rw,seclabel,relatime,size=1024000k 0 0\n";

Resembles a real system; so far so good.

> +
> +const char *cgroups =
> +    "115:memory:/\n"
> +    "8:blkio:/\n"
> +    "6:freezer:/\n"
> +    "3:cpuacct,cpu:/system\n"
> +    "2:cpuset:/\n"
> +    "1:name=systemd:/user/berrange/123\n";

No wonder it resembles a real system :)  I'm sure you populated this
based on your own system, then tweaked it into the test.

> +
> +static int make_file(const char *path,
> +                      const char *name,
> +                      const char *value)

Indentation is off.

> +{
> +    int fd = -1;
> +    int ret = -1;
> +    char *filepath = NULL;
> +
> +    if (asprintf(&filepath, "%s/%s", path, name) < 0)
> +        return -1;
> +
> +    if ((fd = open(filepath, O_CREAT|O_WRONLY, 0600)) < 0)

Is it okay if this calls our open() instead of realopen()?

> +        goto cleanup;
> +
> +    if (write(fd, value, strlen(value)) != strlen(value))

Can't embed any NUL in your fake files, but that's probably okay.

> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    if (fd != -1 &&close(fd) < 0)

Spacing.

> +        ret = -1;
> +    free(filepath);
> +
> +    return ret;
> +}
> +
> +static int make_controller(const char *path, mode_t mode)
> +{
> +    int ret = -1;
> +    const char *controller;
> +
> +    if (!STRPREFIX(path, fakesysfsdir)) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +    controller = path + strlen(fakesysfsdir) + 1;
> +
> +    if (STREQ(controller, "cpu")) {
> +        if (symlink("cpu,cpuacct", path) < 0)
> +            return -1;
> +        return -0;

-0 is cute.  Couldn't this just be:

return symlink("cpu,cpuacct", path);

> +    }
> +    if (STREQ(controller, "cpuacct")) {
> +        if (symlink("cpu,cpuacct", path) < 0)
> +            return -1;
> +        return 0;
> +    }
> +
> +    if (realmkdir(path, mode) < 0)
> +        goto cleanup;
> +
> +#define MAKE_FILE(name, value)                  \
> +    do {                                        \
> +        if (make_file(path, name, value) < 0)   \
> +            goto cleanup;                       \
> +    } while (0)
> +
> +    if (STRPREFIX(controller, "cpu,cpuacct")) {
> +        MAKE_FILE("cpu.cfs_period_us", "100000\n");

Seems useful; again, the set of files and their contents was probably
copied right off your system, even if newer kernels later on add more
files, this test will still be a reasonable action of at least one
kernel in time that libvirt must deal with.

...
> +        MAKE_FILE("blkio.weight_device", "");
> +
> +    } else {
> +        errno = EINVAL;
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +static void init_syms(void)
> +{
> +    if (realfopen)
> +        return;
> +
> +#define LOAD_SYM(name)                                                  \
> +    do {                                                                \
> +        if (!(real ## name = dlsym(RTLD_NEXT, #name))) {                \
> +            fprintf(stderr, "Cannot find real '%s' symbol\n", #name);   \
> +            abort();                                                    \
> +        }                                                               \
> +    } while (0)
> +
> +    LOAD_SYM(fopen);
> +    LOAD_SYM(access);
> +    LOAD_SYM(mkdir);
> +    LOAD_SYM(open);

Looks correct for the stubs we are overriding.

> +}
> +
> +static void init_sysfs(void)
> +{
> +    if (fakesysfsdir)
> +        return;
> +
> +    if (!(fakesysfsdir = getenv("LIBVIRT_FAKE_SYSFS_DIR"))) {
> +        fprintf(stderr, "Missing LIBVIRT_FAKE_SYSFS_DIR env variable\n");
> +        abort();
> +    }
> +
> +#define MAKE_CONTROLLER(subpath) \
> +    do { \
> +        char *path; \
> +        if (asprintf(&path,"%s/%s", fakesysfsdir, subpath) < 0) \
> +            abort();                                           \
> +        if (make_controller(path, 0755) < 0) {                 \
> +            fprintf(stderr, "Cannot initialize %s\n", path);   \
> +            abort();                                           \
> +        }                                                      \

Odd alignment of \ (half aligned, half not)

> +    } while (0)
> +
> +    MAKE_CONTROLLER("cpu");
> +    MAKE_CONTROLLER("cpuacct");
> +    MAKE_CONTROLLER("cpu,cpuacct");
> +    MAKE_CONTROLLER("cpu,cpuacct/system");
> +    MAKE_CONTROLLER("cpuset");
> +    MAKE_CONTROLLER("blkio");
> +    MAKE_CONTROLLER("memory");
> +    MAKE_CONTROLLER("freezer");
> +}
> +
> +
> +FILE *fopen(const char *path, const char *mode)
> +{
> +    init_syms();
> +
> +    if (STREQ(path, "/proc/mounts")) {
> +        if (STREQ(mode, "r")) {
> +            return fmemopen((void *)mounts, strlen(mounts), mode);

fmemopen() - fun stuff.  Glad you're fixing this up to be Linux only :)

Is the (void*) cast really needed?  Ah yes, to cast away const.

> +        } else {
> +            errno = EACCES;
> +            return NULL;
> +        }
> +    }
> +    if (STREQ(path, "/proc/self/cgroup")) {
> +        if (STREQ(mode, "r")) {
> +            return fmemopen((void *)cgroups, strlen(cgroups), mode);
> +        } else {
> +            errno = EACCES;
> +            return NULL;
> +        }
> +    }

And these just happen to be the two files we fopen in libvirt.

> +
> +    return realfopen(path, mode);
> +}
> +
> +int access(const char *path, int mode)
> +{
> +    int ret;
> +
> +    init_syms();
> +
> +    if (STRPREFIX(path, SYSFS_PREFIX)) {
> +        init_sysfs();
> +        char *newpath;
> +        if (asprintf(&newpath, "%s/%s",
> +                     fakesysfsdir,
> +                     path + strlen(SYSFS_PREFIX)) < 0) {
> +            errno = ENOMEM;
> +            return -1;
> +        }
> +        ret = realaccess(newpath, mode);
> +        free(newpath);
> +    } else {
> +        ret = realaccess(path, mode);
> +    }
> +    return ret;
> +}
> +
> +int mkdir(const char *path, mode_t mode)
> +{
> +    int ret;
> +
> +    init_syms();
> +
> +    if (STRPREFIX(path, SYSFS_PREFIX)) {
> +        init_sysfs();
> +        char *newpath;
> +        if (asprintf(&newpath, "%s/%s",
> +                     fakesysfsdir,
> +                     path + strlen(SYSFS_PREFIX)) < 0) {
> +            errno = ENOMEM;
> +            return -1;
> +        }
> +        ret = make_controller(newpath, mode);
> +        free(newpath);
> +    } else {
> +        ret = realmkdir(path, mode);
> +    }
> +    return ret;
> +}

Looks fine.

> +
> +int open(const char *path, int flags, ...)
> +{
> +    int ret;
> +    char *newpath = NULL;
> +
> +    init_syms();
> +
> +    if (STRPREFIX(path, SYSFS_PREFIX)) {
> +        init_sysfs();
> +        if (asprintf(&newpath, "%s/%s",
> +                     fakesysfsdir,
> +                     path + strlen(SYSFS_PREFIX)) < 0) {
> +            errno = ENOMEM;
> +            return -1;
> +        }
> +    }
> +    if (flags & O_CREAT) {
> +        va_list ap;

[1] Yep, you DO need to fix the includes to use <stdarg.h>.

> +        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);
> +    }

You know, wouldn't it just work to write the simpler:

int open(const char *path, int flags, mode_t mode)
{
    ...
    ret = realopen(newpath ? newpath : path, flags, mode);
    ...
}

without any regard to the presence or absence of O_CREAT?  That is, are
there any platforms that support Linux but where var-arg passing would
do the wrong thing if our LD_PRELOAD is specified as a three-argument
function instead of a var-arg function, whether or not the calling app
passed 2 or 3 args?

> +    free(newpath);
> +    return ret;
> +}
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> new file mode 100644
> index 0000000..a68aa88
> --- /dev/null
> +++ b/tests/vircgrouptest.c
> @@ -0,0 +1,249 @@

...
> +
> +static int validateCgroup(virCgroupPtr cgroup,
> +                          const char *expectPath,
> +                          const char **expectMountPoint,
> +                          const char **expectPlacement)
> +{
> +    int i;
> +
> +    if (STRNEQ(cgroup->path, expectPath)) {
> +        fprintf(stderr, "Wrong path '%s', expected '%s'\n",
> +                cgroup->path, expectPath);
> +        return -1;
> +    }

> +
> +const char *mountsSmall[VIR_CGROUP_CONTROLLER_LAST] = {
> +    [VIR_CGROUP_CONTROLLER_CPU] = "/not/really/sys/fs/cgroup/cpu,cpuacct",

Is it worth spelling this:
 SYSFS_PREFIX "/cgroup/cpu,cpuacct"

> +
> +#define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX"
> +
> +
> +
> +static int
> +mymain(void)
> +{
> +    int ret = 0;
> +    char *fakesysfsdir;
> +
> +    if (!(fakesysfsdir = strdup(FAKESYSFSDIRTEMPLATE))) {
> +        fprintf(stderr, "Out of memory\n");
> +        abort();
> +    }
> +
> +    if (!mkdtemp(fakesysfsdir)) {
> +        fprintf(stderr, "Cannot create fakesysfsdir");
> +        abort();
> +    }
> +
> +    setenv("LIBVIRT_FAKE_SYSFS_DIR", fakesysfsdir, 1);
> +
> +    if (virtTestRun("New cgroup for self", 1, testCgroupNewForSelf, NULL) < 0)
> +        ret = -1;
> +
> +    if (virtTestRun("New cgroup for driver", 1, testCgroupNewForDriver, NULL) < 0)
> +        ret = -1;
> +
> +    if (virtTestRun("New cgroup for domain", 1, testCgroupNewForDomain, NULL) < 0)
> +        ret = -1;
> +
> +    if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
> +        virFileDeleteTree(fakesysfsdir);

Worth calling VIR_FREE(fakesysfsdir) here, to keep valgrind happy?
(That is, supposing that valgrind can even deal with our mock LD_PRELOAD
tests)

> +
> +    return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libvircgroupmock.so")
> 

I know that you have access to a mingw cross-compilation environment -
ACK if you fix the issues I mentioned above and check that things still
compile on mingw.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]