On 05/31/2013 09:13 AM, Serge Hallyn wrote: > Update the LOCKING comment. > > Take mem_lock in want_daemonize. > > convert lxcapi_destroy to not use privlock/slock by hand. > > Fix a coverity-found potential dereference of NULL c->lxc_conf. > > api_cgroup_get_item() and api_cgroup_set_item(): use disklock, > not memlock, since the values are set through the cgroup fs on > the running container. > > Signed-off-by: Serge Hallyn <serge.hal...@ubuntu.com>
Looks good.
Acked-by: Stéphane Graber <stgra...@ubuntu.com>
> ---
> src/lxc/lxccontainer.c | 56
> +++++++++++++++++++++++---------------------------
> 1 file changed, 26 insertions(+), 30 deletions(-)
>
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 9bc1caf..24b6008 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -145,11 +145,11 @@ void remove_partial(struct lxc_container *c, int fd)
> }
>
> /* LOCKING
> - * 1. c->privlock protects the struct lxc_container from multiple threads.
> - * 2. c->slock protects the on-disk container data
> - * 3. thread_mutex protects process data (ex: fd table) from multiple threads
> - * slock is an flock, which does not exclude threads. Therefore slock should
> - * always be wrapped inside privlock.
> + * 1. container_mem_lock(c) protects the struct lxc_container from multiple
> threads.
> + * 2. container_disk_lock(c) protects the on-disk container data - in
> particular the
> + * container configuration file.
> + * The container_disk_lock also takes the container_mem_lock.
> + * 3. thread_mutex protects process data (ex: fd table) from multiple
> threads.
> * NOTHING mutexes two independent programs with their own struct
> * lxc_container for the same c->name, between API calls. For instance,
> * c->config_read(); c->start(); Between those calls, data on disk
> @@ -160,7 +160,7 @@ void remove_partial(struct lxc_container *c, int fd)
> * due to hung callers. So I prefer to keep the locks only within our own
> * functions, not across functions.
> *
> - * If you're going to fork while holding a lxccontainer, increment
> + * If you're going to clone while holding a lxccontainer, increment
> * c->numthreads (under privlock) before forking. When deleting,
> * decrement numthreads under privlock, then if it hits 0 you can delete.
> * Do not ever use a lxccontainer whose numthreads you did not bump.
> @@ -406,7 +406,12 @@ static void lxcapi_want_daemonize(struct lxc_container
> *c)
> {
> if (!c)
> return;
> + if (!container_mem_lock(c)) {
> + ERROR("Error getting mem lock");
> + return;
> + }
> c->daemonize = 1;
> + container_mem_unlock(c);
> }
>
> static bool lxcapi_wait(struct lxc_container *c, const char *state, int
> timeout)
> @@ -1218,12 +1223,8 @@ static bool lxcapi_destroy(struct lxc_container *c)
> if (!c || !lxcapi_is_defined(c))
> return false;
>
> - if (lxclock(c->privlock, 0))
> - return false;
> - if (lxclock(c->slock, 0)) {
> - lxcunlock(c->privlock);
> + if (container_disk_lock(c))
> return false;
> - }
>
> if (!is_stopped(c)) {
> // we should queue some sort of error - in c->error_string?
> @@ -1231,7 +1232,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
> goto out;
> }
>
> - if (c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount)
> + if (c->lxc_conf && c->lxc_conf->rootfs.path &&
> c->lxc_conf->rootfs.mount)
> r = bdev_init(c->lxc_conf->rootfs.path,
> c->lxc_conf->rootfs.mount, NULL);
> if (r) {
> if (r->ops->destroy(r) < 0) {
> @@ -1250,8 +1251,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
> ret = true;
>
> out:
> - lxcunlock(c->privlock);
> - lxcunlock(c->slock);
> + container_disk_unlock(c);
> return ret;
> }
>
> @@ -1374,42 +1374,38 @@ err:
> static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char
> *subsys, const char *value)
> {
> int ret;
> - bool b = false;
>
> if (!c)
> return false;
>
> - if (container_mem_lock(c))
> + if (is_stopped(c))
> return false;
>
> - if (is_stopped(c))
> - goto err;
> + if (container_disk_lock(c))
> + return false;
>
> - ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
> - if (!ret)
> - b = true;
> -err:
> - container_mem_unlock(c);
> - return b;
> + ret = lxc_cgroup_set(c->name, subsys, value, c->config_path) == 0;
> +
> + container_disk_unlock(c);
> + return ret == 0;
> }
>
> static int lxcapi_get_cgroup_item(struct lxc_container *c, const char
> *subsys, char *retv, int inlen)
> {
> - int ret = -1;
> + int ret;
>
> if (!c || !c->lxc_conf)
> return -1;
>
> - if (container_mem_lock(c))
> + if (is_stopped(c))
> return -1;
>
> - if (is_stopped(c))
> - goto out;
> + if (container_disk_lock(c))
> + return -1;
>
> ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
>
> -out:
> - container_mem_unlock(c);
> + container_disk_unlock(c);
> return ret;
> }
>
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Get 100% visibility into Java/.NET code with AppDynamics Lite It's a free troubleshooting tool designed for production Get down to code-level detail for bottlenecks, with <2% overhead. Download for free and get started troubleshooting in minutes. http://p.sf.net/sfu/appdyn_d2d_ap2
_______________________________________________ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
