[lxc-devel] [PATCH] fix memory leaks in cgroup functions

Dwight Engen dwight.engen at oracle.com
Thu May 23 19:44:39 UTC 2013


There were several memory leaks in the cgroup functions, notably in the
success cases.

The cgpath test program was refactored and additional tests added to it.
It was used in various modes under valgrind to test that the leaks were
fixed.

Simplify lxc_cgroup_path_get() and cgroup_path_get by having them return a
char * instead of an int and an output char * argument. The only return
values ever used were -1 and 0, which are now handled with NULL and non-NULL
returns respectively.

Use consistent variable names of cgabspath when refering to an absolute path
to a cgroup subsystem or file, and cgrelpath when refering to a container
"group/name" within the cgroup heirarchy.

Remove unused subsystem argument to lxc_cmd_get_cgroup_path().

Remove unused #define MAXPRIOLEN

Make template arg to lxcapi_create() const

Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
---
 src/lxc/cgroup.c       | 270 ++++++++++++++++++-----------------
 src/lxc/cgroup.h       |   6 +-
 src/lxc/commands.c     |   4 +-
 src/lxc/commands.h     |   3 +-
 src/lxc/freezer.c      |  36 ++---
 src/lxc/lxccontainer.c |   6 +-
 src/lxc/lxccontainer.h |   4 +-
 src/lxc/state.c        |  38 ++---
 src/tests/cgpath.c     | 372 +++++++++++++++++++++++++++++++++++++------------
 9 files changed, 463 insertions(+), 276 deletions(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index bb1268b..f04d59a 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -147,102 +147,113 @@ out:
 }
 
 /*
- * cgroup_path_get: Calculate the full path for a particular subsystem, plus
- * a passed-in (to be appended) relative cgpath for a container.
- * @path: a char** into which a pointer to the answer is copied
- * @subsystem: subsystem of interest (i.e. freezer).
- * @cgpath: a container's (relative) cgroup path, i.e. "/lxc/c1".
+ * cgroup_path_get: Get the absolute path to a particular subsystem,
+ * plus a passed-in (to be appended) relative cgpath for a container.
  *
- * Returns 0 on success, -1 on error.
+ * @subsystem : subsystem of interest (e.g. "freezer")
+ * @cgrelpath : a container's relative cgroup path (e.g. "lxc/c1")
+ *
+ * Returns absolute path on success, NULL on error. The caller must free()
+ * the returned path.
  *
+ * Note that @subsystem may be the name of an item (e.g. "freezer.state")
+ * in which case the subsystem will be determined by taking the string up
+ * to the first '.'
  */
-extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath)
+char *cgroup_path_get(const char *subsystem, const char *cgrelpath)
 {
 	int rc;
 
 	char *buf = NULL;
-	char *retbuf = NULL;
+	char *cgabspath = NULL;
 
 	buf = malloc(MAXPATHLEN * sizeof(char));
 	if (!buf) {
 		ERROR("malloc failed");
-		goto fail;
+		goto out1;
 	}
 
-	retbuf = malloc(MAXPATHLEN * sizeof(char));
-	if (!retbuf) {
+	cgabspath = malloc(MAXPATHLEN * sizeof(char));
+	if (!cgabspath) {
 		ERROR("malloc failed");
-		goto fail;
+		goto out2;
 	}
 
 	/* lxc_cgroup_set passes a state object for the subsystem,
 	 * so trim it to just the subsystem part */
 	if (subsystem) {
-		rc = snprintf(retbuf, MAXPATHLEN, "%s", subsystem);
+		rc = snprintf(cgabspath, MAXPATHLEN, "%s", subsystem);
 		if (rc < 0 || rc >= MAXPATHLEN) {
 			ERROR("subsystem name too long");
-			goto fail;
+			goto err3;
 		}
-		char *s = index(retbuf, '.');
+		char *s = index(cgabspath, '.');
 		if (s)
 			*s = '\0';
-		DEBUG("%s: called for subsys %s name %s\n", __func__, retbuf, cgpath);
+		DEBUG("%s: called for subsys %s name %s\n", __func__,
+		      subsystem, cgrelpath);
 	}
-	if (get_cgroup_mount(subsystem ? retbuf : NULL, buf)) {
+	if (get_cgroup_mount(subsystem ? cgabspath : NULL, buf)) {
 		ERROR("cgroup is not mounted");
-		goto fail;
+		goto err3;
 	}
 
-	rc = snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, cgpath);
+	rc = snprintf(cgabspath, MAXPATHLEN, "%s/%s", buf, cgrelpath);
 	if (rc < 0 || rc >= MAXPATHLEN) {
 		ERROR("name too long");
-		goto fail;
+		goto err3;
 	}
 
-	DEBUG("%s: returning %s for subsystem %s", __func__, retbuf, subsystem);
+	DEBUG("%s: returning %s for subsystem %s relpath %s", __func__,
+		cgabspath, subsystem, cgrelpath);
+	goto out2;
 
-	if(buf)
-		free(buf);
-
-	*path = retbuf;
-	return 0;
-fail:
-	if (buf)
-		free(buf);
-	if (retbuf)
-		free(retbuf);
-	return -1;
+err3:
+	free(cgabspath);
+	cgabspath = NULL;
+out2:
+	free(buf);
+out1:
+	return cgabspath;
 }
 
 /*
- * lxc_cgroup_path_get: determine full pathname for a cgroup
- * file for a specific container.
- * @path: char ** used to return the answer.
- * @subsystem: cgroup subsystem (i.e. "freezer") for which to
- * return an answer.  If NULL, then the first cgroup entry in
- * mtab will be used.
+ * lxc_cgroup_path_get: Get the absolute pathname for a cgroup
+ * file for a running container.
+ *
+ * @subsystem : subsystem of interest (e.g. "freezer"). If NULL, then
+ *              the first cgroup entry in mtab will be used.
+ * @name      : name of container to connect to
+ * @lxcpath   : the lxcpath in which the container is running
  *
  * This is the exported function, which determines cgpath from the
- * monitor running in lxcpath.
+ * lxc-start of the @name container running in @lxcpath.
  *
- * Returns 0 on success, < 0 on error.
+ * Returns path on success, NULL on error. The caller must free()
+ * the returned path.
  */
-int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name, const char *lxcpath)
+char *lxc_cgroup_path_get(const char *subsystem, const char *name,
+			  const char *lxcpath)
 {
-	int ret;
-	char *cgpath;
+	char *cgabspath;
+	char *cgrelpath;
 
-	cgpath = lxc_cmd_get_cgroup_path(subsystem, name, lxcpath);
-	if (!cgpath)
-		return -1;
+	cgrelpath = lxc_cmd_get_cgroup_path(name, lxcpath);
+	if (!cgrelpath)
+		return NULL;
 
-	ret = cgroup_path_get(path, subsystem, cgpath);
-	free(cgpath);
-	return ret;
+	cgabspath = cgroup_path_get(subsystem, cgrelpath);
+	free(cgrelpath);
+	return cgabspath;
 }
 
 /*
- * small helper which simply write a value into a (cgroup) file
+ * do_cgroup_set: Write a value into a cgroup file
+ *
+ * @path      : absolute path to cgroup file
+ * @value     : value to write into file
+ *
+ * Returns 0 on success, < 0 on error.
  */
 static int do_cgroup_set(const char *path, const char *value)
 {
@@ -267,87 +278,86 @@ static int do_cgroup_set(const char *path, const char *value)
 }
 
 /*
- * small helper to write a value into a file in a particular directory.
- * @cgpath: the directory in which to find the file
- * @filename: the file (under cgpath) to which to write
- * @value: what to write
+ * lxc_cgroup_set_bypath: Write a value into a cgroup file
+ *
+ * @cgrelpath : a container's relative cgroup path (e.g. "lxc/c1")
+ * @filename  : the cgroup file to write (e.g. "freezer.state")
+ * @value     : value to write into file
  *
  * Returns 0 on success, < 0 on error.
  */
-int lxc_cgroup_set_bypath(const char *cgpath, const char *filename, const char *value)
+int lxc_cgroup_set_bypath(const char *cgrelpath, const char *filename, const char *value)
 {
 	int ret;
-	char *dirpath = NULL;
+	char *cgabspath;
 	char path[MAXPATHLEN];
 
-	ret = cgroup_path_get(&dirpath, filename, cgpath);
-	if (ret)
-		goto fail;
+	cgabspath = cgroup_path_get(filename, cgrelpath);
+	if (!cgabspath)
+		return -1;
 
-	ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
 	if (ret < 0 || ret >= MAXPATHLEN) {
 		ERROR("pathname too long");
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
-	return do_cgroup_set(path, value);
+	ret = do_cgroup_set(path, value);
 
-fail:
-	if(dirpath)
-		free(dirpath);
-	return -1;
+out:
+	free(cgabspath);
+	return ret;
 }
 
 /*
- * set a cgroup value for a container
+ * lxc_cgroup_set: Write a value into a cgroup file
  *
- * @name: name of the container
- * @filename: the cgroup file (i.e. freezer.state) whose value to change
- * @value: the value to write to the file
- * @lxcpath: the lxcpath under which the container is running.
+ * @name      : name of container to connect to
+ * @filename  : the cgroup file to write (e.g. "freezer.state")
+ * @value     : value to write into file
+ * @lxcpath   : the lxcpath in which the container is running
  *
  * Returns 0 on success, < 0 on error.
  */
-
 int lxc_cgroup_set(const char *name, const char *filename, const char *value,
 		   const char *lxcpath)
 {
 	int ret;
-	char *dirpath = NULL;
+	char *cgabspath;
 	char path[MAXPATHLEN];
 
-	ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
-	if (ret)
-		goto fail;
+	cgabspath = lxc_cgroup_path_get(filename, name, lxcpath);
+	if (!cgabspath)
+		return -1;
 
-	ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
 	if (ret < 0 || ret >= MAXPATHLEN) {
 		ERROR("pathname too long");
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
-	return do_cgroup_set(path, value);
+	ret = do_cgroup_set(path, value);
 
-fail:
-	if(dirpath)
-		free(dirpath);
-	return -1;
+out:
+	free(cgabspath);
+	return ret;
 }
 
 /*
- * Get value of a cgroup setting for a container.
+ * lxc_cgroup_get: Read value from a cgroup file
  *
- * @name: name of the container
- * @filename: the cgroup file to read (i.e. 'freezer.state')
- * @value: a preallocated char* into which to copy the answer
- * @len: the length of pre-allocated @value
- * @lxcpath: the lxcpath in which the container is running (i.e.
- * /var/lib/lxc)
+ * @name      : name of container to connect to
+ * @filename  : the cgroup file to read (e.g. "freezer.state")
+ * @value     : a pre-allocated buffer to copy the answer into
+ * @len       : the length of pre-allocated @value
+ * @lxcpath   : the lxcpath in which the container is running
  *
- * Returns < 0 on error, or the number of bytes read.
+ * Returns the number of bytes read on success, < 0 on error
  *
- * If you pass in NULL value or 0 len, then you are asking for the size of the
- * file.
+ * If you pass in NULL value or 0 len, the return value will be the size of
+ * the file, and @value will not contain the contents.
  *
  * Note that we can't get the file size quickly through stat or lseek.
  * Therefore if you pass in len > 0 but less than the file size, your only
@@ -357,25 +367,26 @@ fail:
 int lxc_cgroup_get(const char *name, const char *filename, char *value,
 		   size_t len, const char *lxcpath)
 {
-	int fd, ret = -1;
-	char *dirpath = NULL;
+	int fd, ret;
+	char *cgabspath;
 	char path[MAXPATHLEN];
-	int rc;
 
-	ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
-	if (ret)
-		goto fail;
+	cgabspath = lxc_cgroup_path_get(filename, name, lxcpath);
+	if (!cgabspath)
+		return -1;
 
-	rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
-	if (rc < 0 || rc >= MAXPATHLEN) {
+	ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
+	if (ret < 0 || ret >= MAXPATHLEN) {
 		ERROR("pathname too long");
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		ERROR("open %s : %s", path, strerror(errno));
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
 	if (!len || !value) {
@@ -394,47 +405,45 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
 		ERROR("read %s : %s", path, strerror(errno));
 
 	close(fd);
+out:
+	free(cgabspath);
 	return ret;
-fail:
-	if(dirpath)
-		free(dirpath);
-	return -1;
 }
 
-int lxc_cgroup_nrtasks(const char *cgpath)
+int lxc_cgroup_nrtasks(const char *cgrelpath)
 {
-	char *dirpath = NULL;
+	char *cgabspath = NULL;
 	char path[MAXPATHLEN];
-	int pid, ret, count = 0;
+	int pid, ret;
 	FILE *file;
-	int rc;
 
-	ret = cgroup_path_get(&dirpath, NULL, cgpath);
-	if (ret)
-		goto fail;
+	cgabspath = cgroup_path_get(NULL, cgrelpath);
+	if (!cgabspath)
+		return -1;
 
-	rc = snprintf(path, MAXPATHLEN, "%s/tasks", dirpath);
-	if (rc < 0 || rc >= MAXPATHLEN) {
+	ret = snprintf(path, MAXPATHLEN, "%s/tasks", cgabspath);
+	if (ret < 0 || ret >= MAXPATHLEN) {
 		ERROR("pathname too long");
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
 	file = fopen(path, "r");
 	if (!file) {
 		SYSERROR("fopen '%s' failed", path);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
+	ret = 0;
 	while (fscanf(file, "%d", &pid) != EOF)
-		count++;
+		ret++;
 
 	fclose(file);
 
-	return count;
-fail:
-	if(dirpath)
-		free(dirpath);
-	return -1;
+out:
+	free(cgabspath);
+	return ret;
 }
 
 /*
@@ -654,12 +663,12 @@ char *lxc_cgroup_path_create(const char *lxcgroup, const char *name)
 
 	char buf[LARGE_MAXPATHLEN] = {0};
 
-	if (create_lxcgroups(lxcgroup) < 0)
-		return NULL;
-
 	if (!allcgroups)
 		return NULL;
 
+	if (create_lxcgroups(lxcgroup) < 0)
+		goto err1;
+
 again:
 	if (visited) {
 		/* we're checking for a new name, so start over with all cgroup
@@ -670,9 +679,7 @@ again:
 	file = setmntent(MTAB, "r");
 	if (!file) {
 		SYSERROR("failed to open %s", MTAB);
-		if (allcgroups)
-			free(allcgroups);
-		return NULL;
+		goto err1;
 	}
 
 	if (i)
@@ -730,6 +737,7 @@ next:
 
 fail:
 	endmntent(file);
+err1:
 	free(allcgroups);
 	if (visited)
 		free(visited);
@@ -880,7 +888,7 @@ int lxc_cgroup_attach(pid_t pid, const char *name, const char *lxcpath)
 	int ret;
 	char *dirpath;
 
-	dirpath = lxc_cmd_get_cgroup_path(NULL, name, lxcpath);
+	dirpath = lxc_cmd_get_cgroup_path(name, lxcpath);
 	if (!dirpath) {
 		ERROR("Error getting cgroup for container %s: %s", lxcpath, name);
 		return -1;
diff --git a/src/lxc/cgroup.h b/src/lxc/cgroup.h
index 971311e..747ff5c 100644
--- a/src/lxc/cgroup.h
+++ b/src/lxc/cgroup.h
@@ -23,15 +23,13 @@
 #ifndef _cgroup_h
 #define _cgroup_h
 
-#define MAXPRIOLEN 24
-
 struct lxc_handler;
 extern int lxc_cgroup_destroy(const char *cgpath);
-extern int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name,
+extern char *lxc_cgroup_path_get(const char *subsystem, const char *name,
 			      const char *lxcpath);
 extern int lxc_cgroup_nrtasks(const char *cgpath);
 extern char *lxc_cgroup_path_create(const char *lxcgroup, const char *name);
 extern int lxc_cgroup_enter(const char *cgpath, pid_t pid);
 extern int lxc_cgroup_attach(pid_t pid, const char *name, const char *lxcpath);
-extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath);
+extern char *cgroup_path_get(const char *subsystem, const char *cgpath);
 #endif
diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index 3f21488..169914e 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -335,15 +335,13 @@ static int lxc_cmd_get_clone_flags_callback(int fd, struct lxc_cmd_req *req,
  * particular subsystem. This is the cgroup path relative to the root
  * of the cgroup filesystem.
  *
- * @subsystem : the cgroup subsystem of interest (i.e. freezer)
  * @name      : name of container to connect to
  * @lxcpath   : the lxcpath in which the container is running
  *
  * Returns the path on success, NULL on failure. The caller must free() the
  * returned path.
  */
-char *lxc_cmd_get_cgroup_path(const char *subsystem, const char *name,
-			      const char *lxcpath)
+char *lxc_cmd_get_cgroup_path(const char *name, const char *lxcpath)
 {
 	int ret, stopped = 0;
 	struct lxc_cmd_rr cmd = {
diff --git a/src/lxc/commands.h b/src/lxc/commands.h
index b5b4788..08bde9c 100644
--- a/src/lxc/commands.h
+++ b/src/lxc/commands.h
@@ -67,8 +67,7 @@ struct lxc_cmd_console_rsp_data {
 
 extern int lxc_cmd_console(const char *name, int *ttynum, int *fd,
 			   const char *lxcpath);
-extern char *lxc_cmd_get_cgroup_path(const char *subsystem,
-				     const char *name, const char *lxcpath);
+extern char *lxc_cmd_get_cgroup_path(const char *name, const char *lxcpath);
 extern int lxc_cmd_get_clone_flags(const char *name, const char *lxcpath);
 extern char *lxc_cmd_get_config_item(const char *name, const char *item, const char *lxcpath);
 extern pid_t lxc_cmd_get_init_pid(const char *name, const char *lxcpath);
diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
index 35bf3a7..37a07fd 100644
--- a/src/lxc/freezer.c
+++ b/src/lxc/freezer.c
@@ -120,19 +120,16 @@ out:
 
 static int freeze_unfreeze(const char *name, int freeze, const char *lxcpath)
 {
-	char *nsgroup = NULL;
+	char *cgabspath;
 	int ret;
-	
-	ret = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
-	if (ret)
-		goto fail;
 
-	return do_unfreeze(nsgroup, freeze, name, lxcpath);
+	cgabspath = lxc_cgroup_path_get("freezer", name, lxcpath);
+	if (!cgabspath)
+		return -1;
 
-fail:
-	if (nsgroup)
-		free(nsgroup);
-	return -1;
+	ret = do_unfreeze(cgabspath, freeze, name, lxcpath);
+	free(cgabspath);
+	return ret;
 }
 
 int lxc_freeze(const char *name, const char *lxcpath)
@@ -146,19 +143,16 @@ int lxc_unfreeze(const char *name, const char *lxcpath)
 	return freeze_unfreeze(name, 0, lxcpath);
 }
 
-int lxc_unfreeze_bypath(const char *cgpath)
+int lxc_unfreeze_bypath(const char *cgrelpath)
 {
-	char *nsgroup = NULL;
+	char *cgabspath;
 	int ret;
-	
-	ret = cgroup_path_get(&nsgroup, "freezer", cgpath);
-	if (ret)
-		goto fail;
 
-	return do_unfreeze(nsgroup, 0, NULL, NULL);
+	cgabspath = cgroup_path_get("freezer", cgrelpath);
+	if (!cgabspath)
+		return -1;
 
-fail:
-	if (nsgroup)
-		free(nsgroup);
-	return -1;
+	ret = do_unfreeze(cgabspath, 0, NULL, NULL);
+	free(cgabspath);
+	return ret;
 }
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 2934afa..b2e5e36 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -570,7 +570,7 @@ static bool create_container_dir(struct lxc_container *c)
  * for ->create, argv contains the arguments to pass to the template,
  * terminated by NULL.  If no arguments, you can just pass NULL.
  */
-static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
+static bool lxcapi_create(struct lxc_container *c, const char *t, char *const argv[])
 {
 	bool bret = false;
 	pid_t pid;
@@ -636,7 +636,7 @@ static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
 		newargv = malloc(nargs * sizeof(*newargv));
 		if (!newargv)
 			exit(1);
-		newargv[0] = t;
+		newargv[0] = (char *)t;
 
 		len = strlen(c->config_path) + strlen(c->name) + strlen("--path=") + 2;
 		patharg = malloc(len);
@@ -717,7 +717,7 @@ static bool lxcapi_shutdown(struct lxc_container *c, int timeout)
 	return retv;
 }
 
-static bool lxcapi_createl(struct lxc_container *c, char *t, ...)
+static bool lxcapi_createl(struct lxc_container *c, const char *t, ...)
 {
 	bool bret = false;
 	char **args = NULL, **temp;
diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
index 22ccb0b..1c464fe 100644
--- a/src/lxc/lxccontainer.h
+++ b/src/lxc/lxccontainer.h
@@ -48,8 +48,8 @@ struct lxc_container {
 	bool (*set_config_item)(struct lxc_container *c, const char *key, const char *value);
 	bool (*destroy)(struct lxc_container *c);
 	bool (*save_config)(struct lxc_container *c, const char *alt_file);
-	bool (*create)(struct lxc_container *c, char *t, char *const argv[]);
-	bool (*createl)(struct lxc_container *c, char *t, ...);
+	bool (*create)(struct lxc_container *c, const char *t, char *const argv[]);
+	bool (*createl)(struct lxc_container *c, const char *t, ...);
 	/* send SIGPWR.  if timeout is not 0 or -1, do a hard stop after timeout seconds */
 	bool (*shutdown)(struct lxc_container *c, int timeout);
 	/* clear all network or capability items in the in-memory configuration */
diff --git a/src/lxc/state.c b/src/lxc/state.c
index ed59535..347477c 100644
--- a/src/lxc/state.c
+++ b/src/lxc/state.c
@@ -69,38 +69,40 @@ lxc_state_t lxc_str2state(const char *state)
 
 static lxc_state_t freezer_state(const char *name, const char *lxcpath)
 {
-	char *nsgroup = NULL;
+	char *cgabspath = NULL;
 	char freezer[MAXPATHLEN];
 	char status[MAXPATHLEN];
 	FILE *file;
-	int err;
+	int ret;
 
-	err = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
-	if (err)
-		goto fail;
+	cgabspath = lxc_cgroup_path_get("freezer", name, lxcpath);
+	if (!cgabspath)
+		return -1;
 
-	err = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
-	if (err < 0 || err >= MAXPATHLEN)
-		goto fail;
+	ret = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", cgabspath);
+	if (ret < 0 || ret >= MAXPATHLEN)
+		goto out;
 
 	file = fopen(freezer, "r");
-	if (!file)
-		goto fail;
+	if (!file) {
+		ret = -1;
+		goto out;
+	}
 
-	err = fscanf(file, "%s", status);
+	ret = fscanf(file, "%s", status);
 	fclose(file);
 
-	if (err == EOF) {
+	if (ret == EOF) {
 		SYSERROR("failed to read %s", freezer);
-		goto fail;
+		ret = -1;
+		goto out;
 	}
 
-	return lxc_str2state(status);
+	ret = lxc_str2state(status);
 
-fail:
-	if (nsgroup)
-		free(nsgroup);
-	return -1;
+out:
+	free(cgabspath);
+	return ret;
 }
 
 lxc_state_t lxc_getstate(const char *name, const char *lxcpath)
diff --git a/src/tests/cgpath.c b/src/tests/cgpath.c
index 2497e9e..5fb1d31 100644
--- a/src/tests/cgpath.c
+++ b/src/tests/cgpath.c
@@ -18,6 +18,7 @@
  */
 #include "../lxc/lxccontainer.h"
 
+#include <limits.h>
 #include <unistd.h>
 #include <signal.h>
 #include <stdio.h>
@@ -30,137 +31,324 @@
 #include "../lxc/commands.h"
 
 #define MYNAME "lxctest1"
-#define MYNAME2 "lxctest2"
 
-#define TSTERR(x) do { \
-	fprintf(stderr, "%d: %s\n", __LINE__, x); \
+#define TSTERR(fmt, ...) do { \
+	fprintf(stderr, "%s:%d " fmt "\n", __FILE__, __LINE__, ##__VA_ARGS__); \
 } while (0)
 
-int main()
+/*
+ * test_same_name: Create the same named container within a group
+ *
+ * @group : name of the container group or NULL for default "lxc"
+ * @name  : name of the container
+ *
+ * Note, lxc will append a -<nr> to duplicate container names. This is what
+ * is tested here.
+ *
+ * Returns 0 on success, < 0 on failure
+ */
+static int test_same_name(const char *group, const char *name)
 {
-	struct lxc_container *c = NULL, *c2 = NULL;
-	char *path;
-	char *dirpath = NULL;
-	int len;
-	int ret, retv = -1;
+	int ret = -1;
+	char *cgrelpath1;
+	char *cgrelpath2;
+	char  relpath[PATH_MAX+1];
 
-	/* won't require privilege necessarily once users are classified by
-	 * pam_cgroup */
-	if (geteuid() != 0) {
-		TSTERR("requires privilege");
-		exit(0);
+	sprintf(relpath, "%s/%s-1", group ? group : "lxc", name);
+
+	cgrelpath1 = lxc_cgroup_path_create(group, name);
+	if (!cgrelpath1) {
+		TSTERR("lxc_cgroup_path_create returned NULL");
+		goto err1;
+	}
+	cgrelpath2 = lxc_cgroup_path_create(group, name);
+	if (!cgrelpath2) {
+		TSTERR("lxc_cgroup_path_create returned NULL");
+		goto err2;
+	}
+	if (strcmp(cgrelpath2, relpath)) {
+		TSTERR("unexpected name for duplicate %s", cgrelpath2);
+		goto err3;
+	}
+
+	ret = 0;
+err3:
+	lxc_cgroup_destroy(cgrelpath2);
+	free(cgrelpath2);
+err2:
+	lxc_cgroup_destroy(cgrelpath1);
+	free(cgrelpath1);
+err1:
+	return ret;
+}
+
+/*
+ * test_basic: Test cgroup functions that don't require a running container
+ *
+ * @group : name of the container group or NULL for default "lxc"
+ * @name  : name of the container
+ *
+ * Returns 0 on success, < 0 on failure
+ */
+static int test_basic(const char *group, const char *name)
+{
+	int ret = -1;
+	char *cgabspath;
+	char *cgrelpath;
+	char  relpath[PATH_MAX+1];
+
+	sprintf(relpath, "%s/%s", group ? group : "lxc", name);
+
+	cgrelpath = lxc_cgroup_path_create(group, name);
+	if (!cgrelpath) {
+		TSTERR("lxc_cgroup_path_create returned NULL");
+		goto err1;
+	}
+	if (!strstr(cgrelpath, relpath)) {
+		TSTERR("lxc_cgroup_path_create %s not in %s", relpath, cgrelpath);
+		goto err2;
+	}
+
+	cgabspath = cgroup_path_get("freezer", cgrelpath);
+	if (!cgabspath) {
+		TSTERR("cgroup_path_get returned NULL");
+		goto err2;
+	}
+	if (!strstr(cgabspath, relpath)) {
+		TSTERR("cgroup_path_get %s not in %s", relpath, cgabspath);
+		goto err3;
+	}
+
+	ret = lxc_cgroup_destroy(cgrelpath);
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_destroy failed");
+		goto err3;
+	}
+err3:
+	free(cgabspath);
+err2:
+	free(cgrelpath);
+err1:
+	return ret;
+}
+
+/*
+ * test_running_container: test cgroup functions against a running container
+ *
+ * @group : name of the container group or NULL for default "lxc"
+ * @name  : name of the container
+ */
+static int test_running_container(const char *lxcpath,
+				  const char *group, const char *name)
+{
+	int nrtasks, ret = -1;
+	struct lxc_container *c = NULL;
+	char *cgrelpath;
+	char *cgabspath;
+	char  relpath[PATH_MAX+1];
+	char  abspath[PATH_MAX+1];
+	char  value[NAME_MAX], value_save[NAME_MAX];
+
+	sprintf(relpath, "%s/%s", group ? group : "lxc", name);
+
+	if ((c = lxc_container_new(name, lxcpath)) == NULL) {
+		TSTERR("container %s couldn't instantiate", name);
+		goto err1;
+	}
+	if (!c->is_defined(c)) {
+		TSTERR("container %s does not exist", name);
+		goto err2;
+	}
+
+	cgrelpath = lxc_cmd_get_cgroup_path(c->name, c->config_path);
+	if (!cgrelpath) {
+		TSTERR("lxc_cmd_get_cgroup_path returned NULL");
+		goto err2;
+	}
+	if (!strstr(cgrelpath, relpath)) {
+		TSTERR("lxc_cmd_get_cgroup_path %s not in %s", relpath, cgrelpath);
+		goto err3;
+	}
+
+	/* test get/set value using memory.swappiness file */
+	ret = lxc_cgroup_get(c->name, "memory.swappiness", value,
+			     sizeof(value), c->config_path);
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_get failed");
+		goto err3;
 	}
+	strcpy(value_save, value);
 
-	printf("Basic cgroup path tests...\n");
-	path = lxc_cgroup_path_create(NULL, MYNAME);
-	if (!path || !(len = strlen(path))) {
-		TSTERR("zero result from lxc_cgroup_path_create");
-		exit(1);
+	ret = lxc_cgroup_set_bypath(cgrelpath, "memory.swappiness", "100");
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_set_bypath failed");
+		goto err3;
 	}
-	if (!strstr(path, "lxc/" MYNAME)) {
-		TSTERR("lxc_cgroup_path_create NULL lxctest1");
-		exit(1);
+	ret = lxc_cgroup_get(c->name, "memory.swappiness", value,
+			     sizeof(value), c->config_path);
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_get failed");
+		goto err3;
+	}
+	if (strcmp(value, "100\n")) {
+		TSTERR("lxc_cgroup_set_bypath failed to set value >%s<", value);
+		goto err3;
+	}
+
+	/* restore original value */
+	ret = lxc_cgroup_set(c->name, "memory.swappiness", value_save,
+			     c->config_path);
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_set failed");
+		goto err3;
+	}
+	ret = lxc_cgroup_get(c->name, "memory.swappiness", value,
+			     sizeof(value), c->config_path);
+	if (ret < 0) {
+		TSTERR("lxc_cgroup_get failed");
+		goto err3;
+	}
+	if (strcmp(value, value_save)) {
+		TSTERR("lxc_cgroup_set failed to set value >%s<", value);
+		goto err3;
 	}
-	free(path);
 
-	path = lxc_cgroup_path_create("ab", MYNAME);
-	if (!path || !(len = strlen(path))) {
-		TSTERR("zero result from lxc_cgroup_path_create");
-		exit(1);
+	nrtasks = lxc_cgroup_nrtasks(cgrelpath);
+	if (nrtasks < 1) {
+		TSTERR("failed getting nrtasks");
+		goto err3;
 	}
-	if (!strstr(path, "ab/" MYNAME)) {
-		TSTERR("lxc_cgroup_path_create ab lxctest1");
-		exit(1);
+
+	cgabspath = lxc_cgroup_path_get("freezer", c->name, c->config_path);
+	if (!cgabspath) {
+		TSTERR("lxc_cgroup_path_get returned NULL");
+		goto err3;
 	}
-	free(path);
-	printf("... passed\n");
+	sprintf(abspath, "%s/%s/%s", "freezer", group ? group : "lxc", c->name);
+	if (!strstr(cgabspath, abspath)) {
+		TSTERR("lxc_cgroup_path_get %s not in %s", abspath, cgabspath);
+		goto err4;
+	}
+
+	free(cgabspath);
+	cgabspath = lxc_cgroup_path_get("freezer.state", c->name, c->config_path);
+	if (!cgabspath) {
+		TSTERR("lxc_cgroup_path_get returned NULL");
+		goto err3;
+	}
+	sprintf(abspath, "%s/%s/%s", "freezer", group ? group : "lxc", c->name);
+	if (!strstr(cgabspath, abspath)) {
+		TSTERR("lxc_cgroup_path_get %s not in %s", abspath, cgabspath);
+		goto err4;
+	}
+
+	ret = 0;
+err4:
+	free(cgabspath);
+err3:
+	free(cgrelpath);
+err2:
+	lxc_container_put(c);
+err1:
+	return ret;
+}
+
+static int test_container(const char *lxcpath,
+			  const char *group, const char *name,
+			  const char *template)
+{
+	int ret;
+	struct lxc_container *c = NULL;
 
-	printf("Container creation tests...\n");
+	if (lxcpath) {
+		ret = mkdir(lxcpath, 0755);
+		if (ret < 0 && errno != EEXIST) {
+			TSTERR("failed to mkdir %s %s", lxcpath, strerror(errno));
+			goto out1;
+		}
+	}
+	ret = -1;
 
-	if ((c = lxc_container_new(MYNAME, NULL)) == NULL) {
-		TSTERR("instantiating first container");
-		exit(1);
+	if ((c = lxc_container_new(name, lxcpath)) == NULL) {
+		TSTERR("instantiating container %s", name);
+		goto out1;
 	}
 	if (c->is_defined(c)) {
 		c->stop(c);
 		c->destroy(c);
-		c = lxc_container_new(MYNAME, NULL);
+		c = lxc_container_new(name, lxcpath);
 	}
 	c->set_config_item(c, "lxc.network.type", "empty");
-	if (!c->createl(c, "ubuntu", NULL)) {
-		TSTERR("creating first container");
-		goto out;
+	if (!c->createl(c, template, NULL)) {
+		TSTERR("creating container %s", name);
+		goto out2;
 	}
 	c->load_config(c, NULL);
 	c->want_daemonize(c);
 	if (!c->startl(c, 0, NULL)) {
-		TSTERR("starting first container");
-		goto out;
+		TSTERR("starting container %s", name);
+		goto out3;
 	}
-	printf("first container passed.  Now two containers...\n");
 
-	char *nsgroup;
-#define ALTBASE "/var/lib/lxctest2"
-	ret = mkdir(ALTBASE, 0755);
+	ret = test_running_container(lxcpath, group, name);
 
-	ret = lxc_cgroup_path_get(&nsgroup, "freezer", MYNAME, c->get_config_path(c));
-	if (ret < 0 || !strstr(nsgroup, "lxc/" MYNAME)) {
-		TSTERR("getting first cgroup path from lxc_command");
-		goto out;
+	c->stop(c);
+out3:
+	c->destroy(c);
+out2:
+	lxc_container_put(c);
+out1:
+	return ret;
+}
+
+int main()
+{
+	int ret = EXIT_FAILURE;
+
+	/* won't require privilege necessarily once users are classified by
+	 * pam_cgroup */
+	if (geteuid() != 0) {
+		TSTERR("requires privilege");
+		exit(0);
 	}
 
-	/* start second container */
-	if ((c2 = lxc_container_new(MYNAME2, ALTBASE)) == NULL) {
-		TSTERR("instantiating second container");
+	if (test_basic(NULL, MYNAME) < 0)
 		goto out;
-	}
-	if (c2->is_defined(c2)) {
-		c2->stop(c2);
-		c2->destroy(c2);
-		c2 = lxc_container_new(MYNAME2, ALTBASE);
-	}
-	c2->set_config_item(c2, "lxc.network.type", "empty");
-	if (!c2->createl(c2, "ubuntu", NULL)) {
-		TSTERR("creating second container");
+	if (test_basic("ab", MYNAME) < 0)
 		goto out;
-	}
+	printf("Basic cgroup path tests...Passed\n");
 
-	c2->load_config(c2, NULL);
-	c2->want_daemonize(c2);
-	if (!c2->startl(c2, 0, NULL)) {
-		TSTERR("starting second container");
+	if (test_same_name(NULL, MYNAME) < 0)
 		goto out;
-	}
+	if (test_same_name("ab", MYNAME) < 0)
+		goto out;
+	printf("Same name tests...Passed\n");
 
-	ret = lxc_cgroup_path_get(&nsgroup, "freezer", MYNAME2, c2->get_config_path(c2));
-	if (ret < 0 || !strstr(nsgroup, "lxc/" MYNAME2)) {
-		TSTERR("getting second cgroup path from lxc_command");
+	#if TEST_ALREADY_RUNNING_CT
+
+	/*
+	 * This is useful for running with valgrind to test for memory
+	 * leaks. The container should already be running, we can't start
+	 * the container ourselves because valgrind gets confused by lxc's
+	 * internal calls to clone.
+	 */
+	if (test_running_container(NULL, NULL, "bb01") < 0)
 		goto out;
-	}
+	printf("Running container cgroup tests...Passed\n");
+
+	#else
 
-	dirpath = lxc_cmd_get_cgroup_path(NULL, c2->name, c2->config_path);
-	if (!dirpath) {
-		TSTERR("getting second container's cgpath");
+	if (test_container(NULL, NULL, MYNAME, "busybox") < 0)
 		goto out;
-	}
+	printf("Container creation tests...Passed\n");
 
-	if (lxc_cgroup_nrtasks(dirpath) < 1) {
-		TSTERR("getting nrtasks");
+	if (test_container("/var/lib/lxctest2", NULL, MYNAME, "busybox") < 0)
 		goto out;
-	}
-	printf("...passed\n");
+	printf("Container creation with LXCPATH tests...Passed\n");
 
-	retv = 0;
+	#endif
+
+	ret = EXIT_SUCCESS;
 out:
-	if (dirpath)
-		free(dirpath);
-	if (c2) {
-		c2->stop(c2);
-		c2->destroy(c2);
-	}
-	if (c) {
-		c->stop(c);
-		c->destroy(c);
-	}
-	return retv;
+	return ret;
 }
-- 
1.8.1.4





More information about the lxc-devel mailing list