cgroups: flatten hierarchy

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

src/lxc/cgroups/cgfsng.c

@@ -382,7 +382,6 @@ static bool cg_legacy_filter_and_set_cpus(char *path, bool am_initialized)
 			   *possmask = NULL;
 	int ret;
 	ssize_t i;
-	char oldv;
 	char *lastslash;
 	ssize_t maxisol = 0, maxoffline = 0, maxposs = 0;
 	bool bret = false, flipped_bit = false;
@@ -392,10 +391,9 @@ static bool cg_legacy_filter_and_set_cpus(char *path, bool am_initialized)
 		ERROR("Failed to detect \"/\" in \"%s\"", path);
 		return bret;
 	}
-	oldv = *lastslash;
 	*lastslash = '\0';
 	fpath = must_make_path(path, "cpuset.cpus", NULL);
-	*lastslash = oldv;
+	*lastslash = '/';
 	posscpus = read_file(fpath);
 	if (!posscpus) {
 		SYSERROR("Failed to read file \"%s\"", fpath);
@@ -516,37 +514,38 @@ copy_parent:
 static bool copy_parent_file(char *path, char *file)
 {
 	__do_free char *parent_path = NULL, *value = NULL;
-	int ret;
-	char oldv;
 	int len = 0;
 	char *lastslash = NULL;
+	int ret;
 
 	lastslash = strrchr(path, '/');
-	if (!lastslash) {
+	if (!lastslash)
-		ERROR("Failed to detect \"/\" in \"%s\"", path);
+		return log_error_errno(false, ENOENT,
-		return false;
+				       "Failed to detect \"/\" in \"%s\"", path);
-	}
+
-	oldv = *lastslash;
 	*lastslash = '\0';
 	parent_path = must_make_path(path, file, NULL);
+	*lastslash = '/';
+
 	len = lxc_read_from_file(parent_path, NULL, 0);
-	if (len <= 0) {
+	if (len <= 0)
-		SYSERROR("Failed to determine buffer size");
+		return log_error_errno(false, errno,
-		return false;
+				       "Failed to determine buffer size");
-	}
 
 	value = must_realloc(NULL, len + 1);
+	value[len] = '\0';
 	ret = lxc_read_from_file(parent_path, value, len);
-	if (ret != len) {
+	if (ret != len)
-		SYSERROR("Failed to read from parent file \"%s\"", parent_path);
+		return log_error_errno(false, errno,
-		return false;
+				       "Failed to read from parent file \"%s\"",
-	}
+				       parent_path);
 
-	*lastslash = oldv;
 	ret = lxc_write_openat(path, file, value, len);
-	if (ret < 0)
+	if (ret < 0 && errno != EACCES)
-		SYSERROR("Failed to write \"%s\" to file \"%s/%s\"", value, path, file);
+		return log_error_errno(false,
-	return ret >= 0;
+				       errno, "Failed to write \"%s\" to file \"%s/%s\"",
+				       value, path, file);
+	return true;
 }
 
 static bool is_unified_hierarchy(const struct hierarchy *h)
@@ -558,9 +557,13 @@ static bool is_unified_hierarchy(const struct hierarchy *h)
  * cgroup.clone_children so that children inherit settings. Since the
  * h->base_path is populated by init or ourselves, we know it is already
  * initialized.
+ *
+ * returns -1 on error, 0 when we didn't created a cgroup, 1 if we created a
+ * cgroup.
  */
-static bool cg_legacy_handle_cpuset_hierarchy(struct hierarchy *h, char *cgname)
+static int cg_legacy_handle_cpuset_hierarchy(struct hierarchy *h, char *cgname)
 {
+	int fret = -1;
 	__do_free char *cgpath = NULL;
 	__do_close_prot_errno int cgroup_fd = -EBADF;
 	int ret;
@@ -568,10 +571,10 @@ static bool cg_legacy_handle_cpuset_hierarchy(struct hierarchy *h, char *cgname)
 	char *slash;
 
 	if (is_unified_hierarchy(h))
-		return true;
+		return 0;
 
 	if (!string_in_list(h->controllers, "cpuset"))
-		return true;
+		return 0;
 
 	if (*cgname == '/')
 		cgname++;
@@ -583,48 +586,41 @@ static bool cg_legacy_handle_cpuset_hierarchy(struct hierarchy *h, char *cgname)
 	if (slash)
 		*slash = '/';
 
+	fret = 1;
 	ret = mkdir(cgpath, 0755);
 	if (ret < 0) {
-		if (errno != EEXIST) {
+		if (errno != EEXIST)
-			SYSERROR("Failed to create directory \"%s\"", cgpath);
+			return log_error_errno(-1, errno, "Failed to create directory \"%s\"", cgpath);
-			return false;
+
-		}
+		fret = 0;
 	}
 
 	cgroup_fd = lxc_open_dirfd(cgpath);
 	if (cgroup_fd < 0)
-		return false;
+		return -1;
 
 	ret = lxc_readat(cgroup_fd, "cgroup.clone_children", &v, 1);
-	if (ret < 0) {
+	if (ret < 0)
-		SYSERROR("Failed to read file \"%s/cgroup.clone_children\"", cgpath);
+		return log_error_errno(-1, errno, "Failed to read file \"%s/cgroup.clone_children\"", cgpath);
-		return false;
-	}
 
 	/* Make sure any isolated cpus are removed from cpuset.cpus. */
-	if (!cg_legacy_filter_and_set_cpus(cgpath, v == '1')) {
+	if (!cg_legacy_filter_and_set_cpus(cgpath, v == '1'))
-		SYSERROR("Failed to remove isolated cpus");
+		return log_error_errno(-1, errno, "Failed to remove isolated cpus");
-		return false;
-	}
 
 	/* Already set for us by someone else. */
 	if (v == '1')
 		TRACE("\"cgroup.clone_children\" was already set to \"1\"");
 
 	/* copy parent's settings */
-	if (!copy_parent_file(cgpath, "cpuset.mems")) {
+	if (!copy_parent_file(cgpath, "cpuset.mems"))
-		SYSERROR("Failed to copy \"cpuset.mems\" settings");
+		return log_error_errno(-1, errno, "Failed to copy \"cpuset.mems\" settings");
-		return false;
-	}
 
-	ret = lxc_writeat(cgroup_fd, "cgroup.clone_children", "1", 1);
-	if (ret < 0) {
 	/* Set clone_children so children inherit our settings */
-		SYSERROR("Failed to write 1 to \"%s/cgroup.clone_children\"", cgpath);
+	ret = lxc_writeat(cgroup_fd, "cgroup.clone_children", "1", 1);
-		return false;
+	if (ret < 0)
-	}
+		return log_error_errno(-1, errno, "Failed to write 1 to \"%s/cgroup.clone_children\"", cgpath);
 
-	return true;
+	return fret;
 }
 
 /* Given two null-terminated lists of strings, return true if any string is in
@@ -1152,10 +1148,9 @@ __cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
 
 	for (int i = 0; ops->hierarchies[i]; i++) {
 		__do_free char *pivot_path = NULL;
-		int ret;
+		char pivot_cgroup[] = CGROUP_PIVOT;
-		char *chop;
-		char pivot_cgroup[] = PIVOT_CGROUP;
 		struct hierarchy *h = ops->hierarchies[i];
+		int ret;
 
 		if (!h->monitor_full_path)
 			continue;
@@ -1164,25 +1159,18 @@ __cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
 			pivot_path = must_make_path(h->mountpoint,
 						    h->container_base_path,
 						    conf->cgroup_meta.dir,
-						    PIVOT_CGROUP,
+						    CGROUP_PIVOT, NULL);
-						    "cgroup.procs", NULL);
 		else
 			pivot_path = must_make_path(h->mountpoint,
 						    h->container_base_path,
-						    PIVOT_CGROUP,
+						    CGROUP_PIVOT, NULL);
-						    "cgroup.procs", NULL);
-
-		chop = strrchr(pivot_path, '/');
-		if (chop)
-			*chop = '\0';
 
 		/*
-		 * Make sure not to pass in the ro string literal PIVOT_CGROUP
+		 * Make sure not to pass in the ro string literal CGROUP_PIVOT
 		 * here.
 		 */
-		if (!cg_legacy_handle_cpuset_hierarchy(h, pivot_cgroup))
+		if (cg_legacy_handle_cpuset_hierarchy(h, pivot_cgroup) < 0)
-			log_warn_errno(continue,
+			log_warn_errno(continue, errno, "Failed to handle legacy cpuset controller");
-				       errno, "Failed to handle legacy cpuset controller");
 
 		ret = mkdir_p(pivot_path, 0755);
 		if (ret < 0 && errno != EEXIST)
@@ -1190,13 +1178,11 @@ __cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
 				       "Failed to create cgroup \"%s\"\n",
 				       pivot_path);
 
-		if (chop)
+		/*
-			*chop = '/';
+		 * Move ourselves into the pivot cgroup to delete our own
-
-		/* Move ourselves into the pivot cgroup to delete our own
 		 * cgroup.
 		 */
-		ret = lxc_write_to_file(pivot_path, pidstr, len, false, 0666);
+		ret = lxc_write_openat(pivot_path, "cgroup.procs", pidstr, len);
 		if (ret != 0)
 			log_warn_errno(continue, errno,
 				       "Failed to move monitor %s to \"%s\"\n",
@@ -1241,73 +1227,66 @@ static int mkdir_eexist_on_last(const char *dir, mode_t mode)
 	return 0;
 }
 
-static bool monitor_create_path_for_hierarchy(struct hierarchy *h, char *cgname)
+static bool create_cgroup_tree(struct hierarchy *h, const char *cgroup_tree,
+			       char *cgroup_leaf, bool payload)
 {
-	int ret;
+	__do_free char *path = NULL;
+	int ret, ret_cpuset;
 
-	if (!cg_legacy_handle_cpuset_hierarchy(h, cgname)) {
+	path = must_make_path(h->mountpoint, h->container_base_path, cgroup_leaf, NULL);
-		ERROR("Failed to handle legacy cpuset controller");
+	if (dir_exists(path))
-		return false;
+		return log_warn_errno(false, errno, "The %s cgroup already existed", path);
-	}
 
-	h->monitor_full_path = must_make_path(h->mountpoint, h->container_base_path, cgname, NULL);
+	ret_cpuset = cg_legacy_handle_cpuset_hierarchy(h, cgroup_leaf);
-	ret = mkdir_eexist_on_last(h->monitor_full_path, 0755);
+	if (ret_cpuset < 0)
+		return log_error_errno(false, errno, "Failed to handle legacy cpuset controller");
+
+	ret = mkdir_eexist_on_last(path, 0755);
 	if (ret < 0) {
-		ERROR("Failed to create cgroup \"%s\"", h->monitor_full_path);
+		/*
-		return false;
+		 * This is the cpuset controller and
+		 * cg_legacy_handle_cpuset_hierarchy() has created our target
+		 * directory for us to ensure correct initialization.
+		 */
+		if (ret_cpuset != 1 || cgroup_tree)
+			return log_error_errno(false, errno, "Failed to create %s cgroup", path);
 	}
 
-	return true;
+	if (payload)
-}
+		h->container_full_path = move_ptr(path);
-
-static bool container_create_path_for_hierarchy(struct hierarchy *h, char *cgname)
-{
-	int ret;
-
-	if (!cg_legacy_handle_cpuset_hierarchy(h, cgname)) {
-		ERROR("Failed to handle legacy cpuset controller");
-		return false;
-	}
-
-	h->container_full_path = must_make_path(h->mountpoint, h->container_base_path, cgname, NULL);
-	ret = mkdir_eexist_on_last(h->container_full_path, 0755);
-	if (ret < 0) {
-		ERROR("Failed to create cgroup \"%s\"", h->container_full_path);
-		return false;
-	}
-
-	return true;
-}
-
-static void remove_path_for_hierarchy(struct hierarchy *h, char *cgname, bool monitor)
-{
-	int ret;
-	char *full_path;
-
-	if (monitor)
-		full_path = h->monitor_full_path;
 	else
+		h->monitor_full_path = move_ptr(path);
+
+	return true;
+}
+
+static void cgroup_remove_leaf(struct hierarchy *h, bool payload)
+{
+	__do_free char *full_path = NULL;
+
+	if (payload)
 		full_path = h->container_full_path;
-
-	ret = rmdir(full_path);
-	if (ret < 0)
-		SYSERROR("Failed to rmdir(\"%s\") from failed creation attempt", full_path);
-
-	free(full_path);
-
-	if (monitor)
-		h->monitor_full_path = NULL;
 	else
+		full_path = h->monitor_full_path;
+
+	if (rmdir(full_path))
+		SYSWARN("Failed to rmdir(\"%s\") cgroup", full_path);
+
+	if (payload)
 		h->container_full_path = NULL;
+	else
+		h->monitor_full_path = NULL;
 }
 
 __cgfsng_ops static inline bool cgfsng_monitor_create(struct cgroup_ops *ops,
 						      struct lxc_handler *handler)
 {
 	__do_free char *monitor_cgroup = NULL;
-	char *offset, *tmp;
+	const char *cgroup_tree;
-	int i, idx = 0;
+	int idx = 0;
+	int i;
 	size_t len;
+	char *suffix;
 	struct lxc_conf *conf;
 
 	if (!ops)
@@ -1323,41 +1302,37 @@ __cgfsng_ops static inline bool cgfsng_monitor_create(struct cgroup_ops *ops,
 		return ret_set_errno(false, EINVAL);
 
 	conf = handler->conf;
+	cgroup_tree = conf->cgroup_meta.dir;
 
-	if (conf->cgroup_meta.dir)
+	if (cgroup_tree)
-		tmp = lxc_string_join("/",
+		monitor_cgroup = must_concat(&len, conf->cgroup_meta.dir, "/",
-				      (const char *[]){conf->cgroup_meta.dir,
+					     DEFAULT_MONITOR_CGROUP_PREFIX,
-						       ops->monitor_pattern,
+					     handler->name,
-						       handler->name, NULL},
+					     CGROUP_CREATE_RETRY, NULL);
-				      false);
 	else
-		tmp = must_make_path(ops->monitor_pattern, handler->name, NULL);
+		monitor_cgroup = must_concat(&len, DEFAULT_MONITOR_CGROUP_PREFIX,
-	if (!tmp)
+					     handler->name,
+					     CGROUP_CREATE_RETRY, NULL);
+	if (!monitor_cgroup)
 		return ret_set_errno(false, ENOMEM);
 
-	len = strlen(tmp) + 5; /* leave room for -NNN\0 */
+	suffix = monitor_cgroup + len - CGROUP_CREATE_RETRY_LEN;
-	monitor_cgroup = must_realloc(tmp, len);
+	*suffix = '\0';
-	offset = monitor_cgroup + len - 5;
-	*offset = 0;
-
 	do {
 		if (idx)
-			sprintf(offset, "-%d", idx);
+			sprintf(suffix, "-%d", idx);
 
 		for (i = 0; ops->hierarchies[i]; i++) {
-			if (!monitor_create_path_for_hierarchy(ops->hierarchies[i],
+			if (create_cgroup_tree(ops->hierarchies[i], cgroup_tree, monitor_cgroup, false))
-							       monitor_cgroup)) {
+				continue;
-				ERROR("Failed to create cgroup \"%s\"",
+
-				      ops->hierarchies[i]->monitor_full_path);
+			ERROR("Failed to create cgroup \"%s\"", ops->hierarchies[i]->monitor_full_path ?: "(null)");
 			for (int j = 0; j < i; j++)
-					remove_path_for_hierarchy(ops->hierarchies[j],
+				cgroup_remove_leaf(ops->hierarchies[j], false);
-								  monitor_cgroup,
-								  true);
 
 			idx++;
 			break;
 		}
-		}
 	} while (ops->hierarchies[i] && idx > 0 && idx < 1000);
 
 	if (idx == 1000)
@@ -1367,17 +1342,19 @@ __cgfsng_ops static inline bool cgfsng_monitor_create(struct cgroup_ops *ops,
 	return log_info(true, "The monitor process uses \"%s\" as cgroup", ops->monitor_cgroup);
 }
 
-/* Try to create the same cgroup in all hierarchies. Start with cgroup_pattern;
+/*
+ * Try to create the same cgroup in all hierarchies. Start with cgroup_pattern;
  * next cgroup_pattern-1, -2, ..., -999.
  */
 __cgfsng_ops static inline bool cgfsng_payload_create(struct cgroup_ops *ops,
 						      struct lxc_handler *handler)
 {
-	__do_free char *container_cgroup = NULL, *tmp = NULL;
+	__do_free char *container_cgroup = NULL;
+	const char *cgroup_tree;
 	int idx = 0;
-	int i, ret;
+	int i;
 	size_t len;
-	char *offset;
+	char *suffix;
 	struct lxc_conf *conf;
 
 	if (!ops)
@@ -1393,46 +1370,45 @@ __cgfsng_ops static inline bool cgfsng_payload_create(struct cgroup_ops *ops,
 		return ret_set_errno(false, EINVAL);
 
 	conf = handler->conf;
+	cgroup_tree = conf->cgroup_meta.dir;
 
-	if (conf->cgroup_meta.dir)
+	if (cgroup_tree)
-		tmp = lxc_string_join("/", (const char *[]){conf->cgroup_meta.dir, handler->name, NULL}, false);
+		container_cgroup = must_concat(&len, cgroup_tree, "/",
+					     DEFAULT_PAYLOAD_CGROUP_PREFIX,
+					     handler->name,
+					     CGROUP_CREATE_RETRY, NULL);
 	else
-		tmp = lxc_string_replace("%n", handler->name, ops->cgroup_pattern);
+		container_cgroup = must_concat(&len, DEFAULT_PAYLOAD_CGROUP_PREFIX,
-	if (!tmp)
+					     handler->name,
-		return log_error_errno(false, ENOMEM,
+					     CGROUP_CREATE_RETRY, NULL);
-				       "Failed expanding cgroup name pattern");
+	if (!container_cgroup)
-
+		return ret_set_errno(false, ENOMEM);
-	len = strlen(tmp) + 5; /* leave room for -NNN\0 */
-	container_cgroup = must_realloc(NULL, len);
-	(void)strlcpy(container_cgroup, tmp, len);
-	offset = container_cgroup + len - 5;
 
+	suffix = container_cgroup + len - CGROUP_CREATE_RETRY_LEN;
+	*suffix = '\0';
 	do {
 		if (idx)
-			sprintf(offset, "-%d", idx);
+			sprintf(suffix, "-%d", idx);
 
 		for (i = 0; ops->hierarchies[i]; i++) {
-			if (!container_create_path_for_hierarchy(ops->hierarchies[i],
+			if (create_cgroup_tree(ops->hierarchies[i], cgroup_tree, container_cgroup, true))
-								 container_cgroup)) {
+				continue;
-				ERROR("Failed to create cgroup \"%s\"",
+
-				      ops->hierarchies[i]->container_full_path);
+			ERROR("Failed to create cgroup \"%s\"", ops->hierarchies[i]->container_full_path ?: "(null)");
 			for (int j = 0; j < i; j++)
-					remove_path_for_hierarchy(ops->hierarchies[j],
+				cgroup_remove_leaf(ops->hierarchies[j], true);
-								  container_cgroup,
+
-								  false);
 			idx++;
 			break;
 		}
-		}
 	} while (ops->hierarchies[i] && idx > 0 && idx < 1000);
 
 	if (idx == 1000)
 		return ret_set_errno(false, ERANGE);
 
-	INFO("The container process uses \"%s\" as cgroup", container_cgroup);
-	ops->container_cgroup = move_ptr(container_cgroup);
-
 	if (ops->unified && ops->unified->container_full_path) {
+		int ret;
+
 		ret = open(ops->unified->container_full_path,
 			   O_DIRECTORY | O_RDONLY | O_CLOEXEC);
 		if (ret < 0)
@@ -1441,6 +1417,8 @@ __cgfsng_ops static inline bool cgfsng_payload_create(struct cgroup_ops *ops,
 		ops->unified_fd = ret;
 	}
 
+	ops->container_cgroup = move_ptr(container_cgroup);
+	INFO("The container process uses \"%s\" as cgroup", ops->container_cgroup);
 	return true;
 }
 
@@ -3300,7 +3278,6 @@ __cgfsng_ops static int cgfsng_data_init(struct cgroup_ops *ops)
 		return ret_set_errno(-1, ENOMEM);
 	}
 	ops->cgroup_pattern = must_copy_string(cgroup_pattern);
-	ops->monitor_pattern = MONITOR_CGROUP;
 
 	return 0;
 }

src/lxc/cgroups/cgroup.h

@@ -7,10 +7,14 @@
 #include <stddef.h>
 #include <sys/types.h>
 
+#include "macro.h"
+
 #define DEFAULT_CGROUP_MOUNTPOINT "/sys/fs/cgroup"
-#define PAYLOAD_CGROUP "lxc.payload"
+#define DEFAULT_PAYLOAD_CGROUP_PREFIX "lxc.payload."
-#define MONITOR_CGROUP "lxc.monitor"
+#define DEFAULT_MONITOR_CGROUP_PREFIX "lxc.monitor."
-#define PIVOT_CGROUP "lxc.pivot"
+#define CGROUP_CREATE_RETRY "-NNNN"
+#define CGROUP_CREATE_RETRY_LEN (STRLITERALLEN(CGROUP_CREATE_RETRY))
+#define CGROUP_PIVOT "lxc.pivot"
 
 struct lxc_handler;
 struct lxc_conf;
@@ -90,9 +94,6 @@ struct cgroup_ops {
 	char *container_cgroup;
 	char *monitor_cgroup;
 
-	/* Static memory, do not free.*/
-	const char *monitor_pattern;
-
 	/* @hierarchies
 	 * - A NULL-terminated array of struct hierarchy, one per legacy
 	 *   hierarchy. No duplicates. First sufficient, writeable mounted

src/lxc/initutils.c

@@ -84,7 +84,7 @@ const char *lxc_global_config_value(const char *option_name)
 		sprintf(user_config_path, "%s/.config/lxc/lxc.conf", user_home);
 		sprintf(user_default_config_path, "%s/.config/lxc/default.conf", user_home);
 		sprintf(user_lxc_path, "%s/.local/share/lxc/", user_home);
-		user_cgroup_pattern = strdup("lxc.payload/%n");
+		user_cgroup_pattern = strdup("%n");
 	}
 	else {
 		user_config_path = strdup(LXC_GLOBAL_CONF);

src/lxc/lsm/apparmor.c

@@ -510,7 +510,7 @@ static inline char *apparmor_dir(const char *ctname, const char *lxcpath)
 
 static inline char *apparmor_profile_full(const char *ctname, const char *lxcpath)
 {
-	return shorten_apparmor_name(must_concat("lxc-", ctname, "_<", lxcpath, ">", NULL));
+	return shorten_apparmor_name(must_concat(NULL, "lxc-", ctname, "_<", lxcpath, ">", NULL));
 }
 
 /* Like apparmor_profile_full() but with slashes replaced by hyphens */
@@ -639,7 +639,7 @@ static char *get_apparmor_profile_content(struct lxc_conf *conf, const char *lxc
 
 	profile_name_full = apparmor_profile_full(conf->name, lxcpath);
 
-	profile = must_concat(
+	profile = must_concat(NULL,
 "#include <tunables/global>\n"
 "profile \"", profile_name_full, "\" flags=(attach_disconnected,mediate_deleted) {\n",
 	                      NULL);
@@ -663,7 +663,7 @@ static char *get_apparmor_profile_content(struct lxc_conf *conf, const char *lxc
 		                  STRARRAYLEN(AA_PROFILE_STACKING_BASE));
 
 		namespace = apparmor_namespace(conf->name, lxcpath);
-		temp = must_concat("  change_profile -> \":", namespace, ":*\",\n"
+		temp = must_concat(NULL, "  change_profile -> \":", namespace, ":*\",\n"
 		                   "  change_profile -> \":", namespace, "://*\",\n",
 		                   NULL);
 		free(namespace);
@@ -682,7 +682,7 @@ static char *get_apparmor_profile_content(struct lxc_conf *conf, const char *lxc
 		if (!aa_can_stack || aa_is_stacked) {
 			char *temp;
 
-			temp = must_concat("  change_profile -> \"",
+			temp = must_concat(NULL, "  change_profile -> \"",
 			                   profile_name_full, "\",\n", NULL);
 			must_append_sized(&profile, &size, temp, strlen(temp));
 			free(temp);

src/lxc/start.c

@@ -1820,8 +1820,9 @@ static int lxc_spawn(struct lxc_handler *handler)
 		goto out_delete_net;
 	}
 
-	if (!cgroup_ops->payload_enter(cgroup_ops, handler))
+	if (!cgroup_ops->payload_enter(cgroup_ops, handler)) {
 		goto out_delete_net;
+	}
 
 	if (!cgroup_ops->payload_delegate_controllers(cgroup_ops)) {
 		ERROR("Failed to delegate controllers to payload cgroup");

src/lxc/string_utils.c

@@ -730,7 +730,7 @@ int lxc_safe_long_long(const char *numstr, long long int *converted)
 	return 0;
 }
 
-char *must_concat(const char *first, ...)
+char *must_concat(size_t *len, const char *first, ...)
 {
 	va_list args;
 	char *cur, *dest;
@@ -751,6 +751,8 @@ char *must_concat(const char *first, ...)
 	va_end(args);
 
 	dest[cur_len] = '\0';
+	if (len)
+		*len = cur_len;
 	return dest;
 }
 

src/lxc/string_utils.h

@@ -79,7 +79,7 @@ extern int parse_byte_size_string(const char *s, int64_t *converted);
  * Concatenate all passed-in strings into one path. Do not fail. If any piece
  * is not prefixed with '/', add a '/'.
  */
-__attribute__((sentinel)) extern char *must_concat(const char *first, ...);
+__attribute__((sentinel)) extern char *must_concat(size_t *len, const char *first, ...);
 __attribute__((sentinel)) extern char *must_make_path(const char *first, ...);
 __attribute__((sentinel)) extern char *must_append_path(char *first, ...);
 

src/lxc/utils.h

@@ -199,13 +199,6 @@ extern int run_command(char *buf, size_t buf_size, int (*child_fn)(void *),
 extern int run_command_status(char *buf, size_t buf_size, int (*child_fn)(void *),
 		       void *args);
 
-/* Concatenate all passed-in strings into one path. Do not fail. If any piece
- * is not prefixed with '/', add a '/'.
- */
-__attribute__((sentinel)) extern char *must_concat(const char *first, ...);
-__attribute__((sentinel)) extern char *must_make_path(const char *first, ...);
-__attribute__((sentinel)) extern char *must_append_path(char *first, ...);
-
 /* return copy of string @entry;  do not fail. */
 extern char *must_copy_string(const char *entry);
 

src/tests/cgpath.c

@@ -46,11 +46,9 @@
 /*
  * 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,
+static int test_running_container(const char *lxcpath, const char *name)
-				  const char *group, const char *name)
 {
 	int ret = -1;
 	struct lxc_container *c = NULL;
@@ -59,7 +57,7 @@ static int test_running_container(const char *lxcpath,
 	char  value[NAME_MAX], value_save[NAME_MAX];
 	struct cgroup_ops *cgroup_ops;
 
-	sprintf(relpath, "%s/%s", group ? group : "lxc.payload", name);
+	sprintf(relpath, DEFAULT_PAYLOAD_CGROUP_PREFIX "%s", name);
 
 	if ((c = lxc_container_new(name, lxcpath)) == NULL) {
 		TSTERR("container %s couldn't instantiate", name);
@@ -128,8 +126,7 @@ err1:
 	return ret;
 }
 
-static int test_container(const char *lxcpath,
+static int test_container(const char *lxcpath, const char *name,
-			  const char *group, const char *name,
 			  const char *template)
 {
 	int ret;
@@ -165,7 +162,7 @@ static int test_container(const char *lxcpath,
 		goto out3;
 	}
 
-	ret = test_running_container(lxcpath, group, name);
+	ret = test_running_container(lxcpath, name);
 
 	c->stop(c);
 out3:
@@ -195,17 +192,17 @@ int main()
 	 * the container ourselves because valgrind gets confused by lxc's
 	 * internal calls to clone.
 	 */
-	if (test_running_container(NULL, NULL, "bb01") < 0)
+	if (test_running_container(NULL, "bb01") < 0)
 		goto out;
 	printf("Running container cgroup tests...Passed\n");
 
 	#else
 
-	if (test_container(NULL, NULL, MYNAME, "busybox") < 0)
+	if (test_container(NULL, MYNAME, "busybox") < 0)
 		goto out;
 	printf("Container creation tests...Passed\n");
 
-	if (test_container("/var/lib/lxctest2", NULL, MYNAME, "busybox") < 0)
+	if (test_container("/var/lib/lxctest2", MYNAME, "busybox") < 0)
 		goto out;
 	printf("Container creation with LXCPATH tests...Passed\n");