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

[Daniel P. Berrange]

On Fri, Apr 12, 2013 at 03:24:04PM -0600, Eric Blake wrote:
> On 04/10/2013 04:08 AM, Daniel P. Berrange wrote:
> 
> > +++ 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.

Yep, put  #ifdef __linux__ around the test source files

> 
> >  
> > +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?

Yes, I prviously had it all in two files, but split it when the
helper got too big

> > +#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]

Yes, though I guess something must have pulled it in indirectly.
Will add the explicit include though.

> > +{
> > +    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()?

We're lucky enough to be safe, but I switched it to use realopen()

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

Yes, I've made that change

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

The problem is that the decl on open() has to match the decl
used in <fcntl.h>. Since that uses '...' we must too.


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

Well SYSFS_PREFIX only exists in the other source file, not
this one :-)


> > +
> > +    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.

Wow, we have neglected mingw recently. I've found a tonne of pre-existing
problems :-(


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