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

[Dwight Engen]

On Wed, 29 May 2013 12:26:25 -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.
> 
> 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>

Acked-by: Dwight Engen <dwight.engen at oracle.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;
>  	}