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

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

Currently (after this patch) disk_lock is only taken around
save_config and load_config.  I think we will want to tighten
some of this back down, but really we can't lock out external
editors acting on config files anyway - so it's more important
(after ensuring we don't have deadlocks) to make sure the API is
thread-safe, which means taking the process_lock() around open/close
etc.  So I'll focus on that in the next bit.  Then work on a
fn by fn locking rationale.

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

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index b34b8e8..9eb431c 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;
@@ -888,7 +876,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);
@@ -1159,13 +1146,13 @@ static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
 	FILE *fout = fopen(alt_file, "w");
 	if (!fout)
 		return false;
-	if (container_mem_lock(c)) {
+	if (container_disk_lock(c)) {
 		fclose(fout);
 		return false;
 	}
 	write_config(fout, c->lxc_conf);
 	fclose(fout);
-	container_mem_unlock(c);
+	container_disk_unlock(c);
 	return true;
 }
 
@@ -1185,7 +1172,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 +1329,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 +1350,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 +1814,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