On 04/10/2013 04:08 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> .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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list