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

[Dwight Engen]

On Wed, 29 May 2013 00:01:16 -0500
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> 
> 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;
>  	}

This isn't new with your change, but shouldn't we try to get the
disk_lock before doing the fopen() since fopen() is going to truncate
the file and if the getting the lock fails now we have an empty config
file?

>  	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;
>  	}