4:24 p.m.
On 04/10/2013 04:08 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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