Quoting Dwight Engen (dwight.en...@oracle.com):
> On Wed, 29 May 2013 00:01:16 -0500
> Serge Hallyn <serge.hal...@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.hal...@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
------------------------------------------------------------------------------
Introducing AppDynamics Lite, a free troubleshooting tool for Java/.NET
Get 100% visibility into your production application - at no cost.
Code-level diagnostics for performance bottlenecks with <2% overhead
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap1
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
