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

[Serge Hallyn]

Quoting Dwight Engen (dwight.engen at oracle.com):
> 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?

Hm, yes.  Also, we should probably not take the lock if alt_file is
provided (and != c->configfile).  Same as load_config.

thanks!

-serge