[lxc-devel] [PATCH 1/1] lxccontainer: don't lock around getstate and freeze/unfreeze (v2)

[Serge Hallyn]

Those go through commands.c and are already mutex'ed that way.

Also remove a unmatched container_disk_unlock in lxcapi_create.

Since is_stopped uses getstate which is no longer locked, rename
it to drop the _locked suffix.

And convert save_config to taking the disk lock.  This way the
save_ and load_config are mutexing each other, as they should.

Changelog: May 29:
   Per Dwight's comment, take the lock before opening the config
      FILE *.
   Only take disklock at load and save_config when we're using the
   container's config file, not when read/writing from/to another
   file.

Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
---
 src/lxc/lxccontainer.c | 92 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 31 deletions(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index b34b8e8..ef95033 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -287,21 +287,15 @@ out:
 
 static const char *lxcapi_state(struct lxc_container *c)
 {
-	const char *ret;
 	lxc_state_t s;
 
 	if (!c)
 		return NULL;
-	if (container_disk_lock(c))
-		return NULL;
 	s = lxc_getstate(c->name, c->config_path);
-	ret = lxc_state2str(s);
-	container_disk_unlock(c);
-
-	return ret;
+	return lxc_state2str(s);
 }
 
-static bool is_stopped_locked(struct lxc_container *c)
+static bool is_stopped(struct lxc_container *c)
 {
 	lxc_state_t s;
 	s = lxc_getstate(c->name, c->config_path);
@@ -326,10 +320,7 @@ static bool lxcapi_freeze(struct lxc_container *c)
 	if (!c)
 		return false;
 
-	if (container_disk_lock(c))
-		return false;
 	ret = lxc_freeze(c->name, c->config_path);
-	container_disk_unlock(c);
 	if (ret)
 		return false;
 	return true;
@@ -341,10 +332,7 @@ static bool lxcapi_unfreeze(struct lxc_container *c)
 	if (!c)
 		return false;
 
-	if (container_disk_lock(c))
-		return false;
 	ret = lxc_unfreeze(c->name, c->config_path);
-	container_disk_unlock(c);
 	if (ret)
 		return false;
 	return true;
@@ -369,7 +357,8 @@ static bool load_config_locked(struct lxc_container *c, const char *fname)
 
 static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
 {
-	bool ret = false;
+	bool ret = false, need_disklock = false;
+	int lret;
 	const char *fname;
 	if (!c)
 		return false;
@@ -379,10 +368,27 @@ static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
 		fname = alt_file;
 	if (!fname)
 		return false;
-	if (container_disk_lock(c))
+	/*
+	 * If we're reading something other than the container's config,
+	 * we only need to lock the in-memory container.  If loading the
+	 * container's config file, take the disk lock.
+	 */
+	if (strcmp(fname, c->configfile) == 0)
+		need_disklock = true;
+
+	if (need_disklock)
+		lret = container_disk_lock(c);
+	else
+		lret = container_mem_lock(c);
+	if (lret)
 		return false;
+
 	ret = load_config_locked(c, fname);
-	container_disk_unlock(c);
+
+	if (need_disklock)
+		container_disk_unlock(c);
+	else
+		container_mem_unlock(c);
 	return ret;
 }
 
@@ -888,7 +894,6 @@ static bool lxcapi_create(struct lxc_container *c, const char *t,
 out_unlock:
 	if (partial_fd >= 0)
 		remove_partial(c, partial_fd);
-	container_disk_unlock(c);
 out:
 	if (tpath)
 		free(tpath);
@@ -1143,30 +1148,55 @@ static int lxcapi_get_keys(struct lxc_container *c, const char *key, char *retv,
 #define LXC_DEFAULT_CONFIG "/etc/lxc/default.conf"
 static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
 {
+	FILE *fout;
+	bool ret = false, need_disklock = false;
+	int lret;
+
 	if (!alt_file)
 		alt_file = c->configfile;
 	if (!alt_file)
 		return false;  // should we write to stdout if no file is specified?
-	if (!c->lxc_conf)
+
+	// If we haven't yet loaded a config, load the stock config
+	if (!c->lxc_conf) {
 		if (!c->load_config(c, LXC_DEFAULT_CONFIG)) {
 			ERROR("Error loading default configuration file %s while saving %s\n", LXC_DEFAULT_CONFIG, c->name);
 			return false;
 		}
+	}
 
 	if (!create_container_dir(c))
 		return false;
 
-	FILE *fout = fopen(alt_file, "w");
-	if (!fout)
-		return false;
-	if (container_mem_lock(c)) {
-		fclose(fout);
+	/*
+	 * If we're writing to the container's config file, take the
+	 * disk lock.  Otherwise just take the memlock to protect the
+	 * struct lxc_container while we're traversing it.
+	 */
+	if (strcmp(c->configfile, alt_file) == 0)
+		need_disklock = true;
+
+	if (need_disklock)
+		lret = container_disk_lock(c);
+	else
+		lret = container_mem_lock(c);
+
+	if (lret)
 		return false;
-	}
+
+	fout = fopen(alt_file, "w");
+	if (!fout)
+		goto out;
 	write_config(fout, c->lxc_conf);
 	fclose(fout);
-	container_mem_unlock(c);
-	return true;
+	ret = true;
+
+out:
+	if (need_disklock)
+		container_disk_unlock(c);
+	else
+		container_mem_unlock(c);
+	return ret;
 }
 
 // do we want the api to support --force, or leave that to the caller?
@@ -1185,7 +1215,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
 		return false;
 	}
 
-	if (!is_stopped_locked(c)) {
+	if (!is_stopped(c)) {
 		// we should queue some sort of error - in c->error_string?
 		ERROR("container %s is not stopped", c->name);
 		goto out;
@@ -1342,7 +1372,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
 	if (container_mem_lock(c))
 		return false;
 
-	if (is_stopped_locked(c))
+	if (is_stopped(c))
 		goto err;
 
 	ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
@@ -1363,7 +1393,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
 	if (container_mem_lock(c))
 		return -1;
 
-	if (is_stopped_locked(c))
+	if (is_stopped(c))
 		goto out;
 
 	ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
@@ -1827,7 +1857,7 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
 	if (container_mem_lock(c))
 		return NULL;
 
-	if (!is_stopped_locked(c)) {
+	if (!is_stopped(c)) {
 		ERROR("error: Original container (%s) is running", c->name);
 		goto out;
 	}
-- 
1.8.1.2