locking: update per Dwight's comment

Create three pairs of functions:
	int process_lock(void);
	void process_unlock(void);
	int container_mem_lock(struct lxc_container *c)
	void container_mem_unlock(struct lxc_container *c)
	int container_disk_lock(struct lxc_container *c);
	void container_disk_unlock(struct lxc_container *c);

and use those in lxccontainer.c

process_lock() is to protect the process state among multiple threads.
container_mem_lock() is to protect a struct container among multiple
threads.  container_disk_lock is to protect a container on disk.

Also remove the lock in lxcapi_init_pid() as Dwight suggested.

Fix a typo (s/container/contain) spotted by Dwight.

More locking fixes are needed, but let's first the the fundamentals
right.  How close does this get us?

Changelog: v2:
	fix lxclock compile

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Dwight Engen <dwight.engen@oracle.com>

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 0ec23e8f3e5ee5607d7764228b89e551b5501268..2a45c1fe059b6c856b8ad28d7d52a256bc6e186d 100644 (file)
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -43,8 +43,6 @@
 #include <arpa/inet.h>
 #include <ifaddrs.h>
 
-extern pthread_mutex_t thread_mutex;
-
 lxc_log_define(lxc_container, lxc);
 
 /* LOCKING
@@ -138,7 +136,7 @@ int lxc_container_get(struct lxc_container *c)
        if (c->numthreads < 1)
                return 0;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return 0;
        if (c->numthreads < 1) {
                // bail without trying to unlock, bc the privlock is now probably
@@ -146,7 +144,7 @@ int lxc_container_get(struct lxc_container *c)
                return 0;
        }
        c->numthreads++;
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return 1;
 }
 
@@ -154,14 +152,14 @@ int lxc_container_put(struct lxc_container *c)
 {
        if (!c)
                return -1;
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return -1;
        if (--c->numthreads < 1) {
-               lxcunlock(c->privlock);
+               container_mem_unlock(c);
                lxc_container_free(c);
                return 1;
        }
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return 0;
 }
 
@@ -181,7 +179,7 @@ static bool lxcapi_is_defined(struct lxc_container *c)
        if (!c)
                return false;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return false;
        if (!c->configfile)
                goto out;
@@ -191,7 +189,7 @@ static bool lxcapi_is_defined(struct lxc_container *c)
        ret = true;
 
 out:
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return ret;
 }
 
@@ -202,15 +200,11 @@ static const char *lxcapi_state(struct lxc_container *c)
 
        if (!c)
                return NULL;
-       if (lxclock(c->privlock, 0))
+       if (container_disk_lock(c))
                return NULL;
-       if (lxclock(c->slock, 0))
-               goto bad;
        s = lxc_getstate(c->name, c->config_path);
        ret = lxc_state2str(s);
-       lxcunlock(c->slock);
-bad:
-       lxcunlock(c->privlock);
+       container_disk_unlock(c);
 
        return ret;
 }
@@ -240,15 +234,10 @@ static bool lxcapi_freeze(struct lxc_container *c)
        if (!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;
-       }
        ret = lxc_freeze(c->name, c->config_path);
-       lxcunlock(c->slock);
-       lxcunlock(c->privlock);
+       container_disk_unlock(c);
        if (ret)
                return false;
        return true;
@@ -260,15 +249,10 @@ static bool lxcapi_unfreeze(struct lxc_container *c)
        if (!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;
-       }
        ret = lxc_unfreeze(c->name, c->config_path);
-       lxcunlock(c->slock);
-       lxcunlock(c->privlock);
+       container_disk_unlock(c);
        if (ret)
                return false;
        return true;
@@ -276,20 +260,10 @@ static bool lxcapi_unfreeze(struct lxc_container *c)
 
 static pid_t lxcapi_init_pid(struct lxc_container *c)
 {
-       pid_t ret;
        if (!c)
                return -1;
 
-       if (lxclock(c->privlock, 0))
-               return -1;
-       if (lxclock(c->slock, 0)) {
-               lxcunlock(c->privlock);
-               return -1;
-       }
-       ret = lxc_cmd_get_init_pid(c->name, c->config_path);
-       lxcunlock(c->slock);
-       lxcunlock(c->privlock);
-       return ret;
+       return lxc_cmd_get_init_pid(c->name, c->config_path);
 }
 
 static bool load_config_locked(struct lxc_container *c, const char *fname)
@@ -313,15 +287,10 @@ static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
                fname = alt_file;
        if (!fname)
                return false;
-       if (lxclock(c->privlock, 0))
-               return false;
-       if (lxclock(c->slock, 0)) {
-               lxcunlock(c->privlock);
+       if (container_disk_lock(c))
                return false;
-       }
        ret = load_config_locked(c, fname);
-       lxcunlock(c->slock);
-       lxcunlock(c->privlock);
+       container_disk_unlock(c);
        return ret;
 }
 
@@ -384,11 +353,11 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
        if (useinit && !argv)
                return false;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return false;
        conf = c->lxc_conf;
        daemonize = c->daemonize;
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
 
        if (useinit) {
                ret = lxc_execute(c->name, argv, 1, conf, c->config_path);
@@ -409,23 +378,20 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
                        return false;
                lxc_monitord_spawn(c->config_path);
 
-               ret = pthread_mutex_lock(&thread_mutex);
-               if (ret != 0) {
-                       ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+               if (process_lock())
                        return false;
-               }
                pid_t pid = fork();
                if (pid < 0) {
                        lxc_container_put(c);
-                       pthread_mutex_unlock(&thread_mutex);
+                       process_unlock();
                        return false;
                }
                if (pid != 0) {
                        ret = wait_on_daemonized_start(c);
-                       pthread_mutex_unlock(&thread_mutex);
+                       process_unlock();
                        return ret;
                }
-               pthread_mutex_unlock(&thread_mutex);
+               process_unlock();
                /* second fork to be reparented by init */
                pid = fork();
                if (pid < 0) {
@@ -613,13 +579,8 @@ static bool lxcapi_create(struct lxc_container *c, const char *t, char *const ar
        /* we're going to fork.  but since we'll wait for our child, we
         * don't need to lxc_container_get */
 
-       if (lxclock(c->privlock, 0))
+       if (container_disk_lock(c))
                goto out;
-       if (lxclock(c->slock, 0)) {
-               ERROR("failed to grab global container lock for %s\n", c->name);
-               lxcunlock(c->privlock);
-               goto out_unlock1;
-       }
 
        pid = fork();
        if (pid < 0) {
@@ -698,9 +659,7 @@ static bool lxcapi_create(struct lxc_container *c, const char *t, char *const ar
        bret = load_config_locked(c, c->configfile);
 
 out_unlock:
-       lxcunlock(c->slock);
-out_unlock1:
-       lxcunlock(c->privlock);
+       container_disk_unlock(c);
 out:
        if (tpath)
                free(tpath);
@@ -778,11 +737,10 @@ static bool lxcapi_clear_config_item(struct lxc_container *c, const char *key)
 
        if (!c || !c->lxc_conf)
                return false;
-       if (lxclock(c->privlock, 0)) {
+       if (container_mem_lock(c))
                return false;
-       }
        ret = lxc_clear_config_item(c->lxc_conf, key);
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return ret == 0;
 }
 
@@ -904,11 +862,10 @@ static int lxcapi_get_config_item(struct lxc_container *c, const char *key, char
 
        if (!c || !c->lxc_conf)
                return -1;
-       if (lxclock(c->privlock, 0)) {
+       if (container_mem_lock(c))
                return -1;
-       }
        ret = lxc_get_config_item(c->lxc_conf, key, retv, inlen);
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return ret;
 }
 
@@ -923,12 +880,12 @@ static int lxcapi_get_keys(struct lxc_container *c, const char *key, char *retv,
         */
        if (!c || !c->lxc_conf)
                return -1;
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return -1;
        int ret = -1;
        if (strncmp(key, "lxc.network.", 12) == 0)
                ret =  lxc_list_nicconfigs(c->lxc_conf, key, retv, inlen);
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return ret;
 }
 
@@ -953,13 +910,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 (lxclock(c->privlock, 0)) {
+       if (container_mem_lock(c)) {
                fclose(fout);
                return false;
        }
        write_config(fout, c->lxc_conf);
        fclose(fout);
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return true;
 }
 
@@ -1000,7 +957,7 @@ static bool lxcapi_set_config_item(struct lxc_container *c, const char *key, con
        if (!c)
                return false;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return false;
 
        if (!c->lxc_conf)
@@ -1015,7 +972,7 @@ static bool lxcapi_set_config_item(struct lxc_container *c, const char *key, con
                b = true;
 
 err:
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return b;
 }
 
@@ -1076,7 +1033,7 @@ static bool lxcapi_set_config_path(struct lxc_container *c, const char *path)
        if (!c)
                return b;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return b;
 
        p = strdup(path);
@@ -1102,7 +1059,7 @@ static bool lxcapi_set_config_path(struct lxc_container *c, const char *path)
 err:
        if (oldpath)
                free(oldpath);
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return b;
 }
 
@@ -1115,7 +1072,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
        if (!c)
                return false;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return false;
 
        if (is_stopped_nolock(c))
@@ -1125,7 +1082,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
        if (!ret)
                b = true;
 err:
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return b;
 }
 
@@ -1136,7 +1093,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
        if (!c || !c->lxc_conf)
                return -1;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return -1;
 
        if (is_stopped_nolock(c))
@@ -1145,7 +1102,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
        ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
 
 out:
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return ret;
 }
 
@@ -1600,7 +1557,7 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
        if (!c || !c->is_defined(c))
                return NULL;
 
-       if (lxclock(c->privlock, 0))
+       if (container_mem_lock(c))
                return NULL;
 
        if (c->is_running(c)) {
@@ -1682,11 +1639,11 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
                goto out;
 
        // TODO: update c's lxc.snapshot = count
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        return c2;
 
 out:
-       lxcunlock(c->privlock);
+       container_mem_unlock(c);
        if (c2) {
                c2->destroy(c2);
                lxc_container_put(c2);

diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
index bddd1e9e80e8445b272563a0e017198c3ee9b575..4bbe873be8e26ba7a81e516f135893484d562bb7 100644 (file)
--- a/src/lxc/lxclock.c
+++ b/src/lxc/lxclock.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <lxc/utils.h>
 #include <lxc/log.h>
+#include <lxc/lxccontainer.h>
 
 #define OFLAG (O_CREAT | O_RDWR)
 #define SEMMODE 0660
@@ -48,7 +49,10 @@ static char *lxclock_name(const char *p, const char *n)
                free(dest);
                return NULL;
        }
-       if (mkdir_p(dest, 0755) < 0) {
+       process_lock();
+       ret = mkdir_p(dest, 0755);
+       process_unlock();
+       if (ret < 0) {
                free(dest);
                return NULL;
        }
@@ -80,11 +84,6 @@ static sem_t *lxc_new_unnamed_sem(void)
 struct lxc_lock *lxc_newlock(const char *lxcpath, const char *name)
 {
        struct lxc_lock *l;
-       int ret = pthread_mutex_lock(&thread_mutex);
-       if (ret != 0) {
-               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
-               return NULL;
-       }
 
        l = malloc(sizeof(*l));
        if (!l)
@@ -106,18 +105,12 @@ struct lxc_lock *lxc_newlock(const char *lxcpath, const char *name)
        l->u.f.fd = -1;
 
 out:
-       pthread_mutex_unlock(&thread_mutex);
        return l;
 }
 
 int lxclock(struct lxc_lock *l, int timeout)
 {
-       int saved_errno = errno;
-       int ret = pthread_mutex_lock(&thread_mutex);
-       if (ret != 0) {
-               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
-               return ret;
-       }
+       int ret = -1, saved_errno = errno;
 
        switch(l->type) {
        case LXC_LOCK_ANON_SEM:
@@ -149,35 +142,31 @@ int lxclock(struct lxc_lock *l, int timeout)
                        ret = -2;
                        goto out;
                }
+               process_lock();
                if (l->u.f.fd == -1) {
                        l->u.f.fd = open(l->u.f.fname, O_RDWR|O_CREAT,
                                        S_IWUSR | S_IRUSR);
                        if (l->u.f.fd == -1) {
+                               process_unlock();
                                ERROR("Error opening %s", l->u.f.fname);
                                goto out;
                        }
                }
                ret = flock(l->u.f.fd, LOCK_EX);
+               process_unlock();
                if (ret == -1)
                        saved_errno = errno;
                break;
        }
 
 out:
-       pthread_mutex_unlock(&thread_mutex);
        errno = saved_errno;
        return ret;
 }
 
 int lxcunlock(struct lxc_lock *l)
 {
-       int saved_errno = errno;
-       int ret = pthread_mutex_lock(&thread_mutex);
-
-       if (ret != 0) {
-               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
-               return ret;
-       }
+       int ret = 0, saved_errno = errno;
 
        switch(l->type) {
        case LXC_LOCK_ANON_SEM:
@@ -188,6 +177,7 @@ int lxcunlock(struct lxc_lock *l)
                        saved_errno = errno;
                break;
        case LXC_LOCK_FLOCK:
+               process_lock();
                if (l->u.f.fd != -1) {
                        if ((ret = flock(l->u.f.fd, LOCK_UN)) < 0)
                                saved_errno = errno;
@@ -195,40 +185,84 @@ int lxcunlock(struct lxc_lock *l)
                        l->u.f.fd = -1;
                } else
                        ret = -2;
+               process_unlock();
                break;
        }
 
-       pthread_mutex_unlock(&thread_mutex);
        errno = saved_errno;
        return ret;
 }
 
+/*
+ * lxc_putlock() is only called when a container_new() fails,
+ * or during container_put(), which is already guaranteed to
+ * only be done by one task.
+ * So the only exclusion we need to provide here is for regular
+ * thread safety (i.e. file descriptor table changes).
+ */
 void lxc_putlock(struct lxc_lock *l)
 {
-       int ret = pthread_mutex_lock(&thread_mutex);
-       if (ret != 0) {
-               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
-               return;
-       }
-
        if (!l)
-               goto out;
+               return;
        switch(l->type) {
        case LXC_LOCK_ANON_SEM:
                if (l->u.sem)
                        sem_close(l->u.sem);
                break;
        case LXC_LOCK_FLOCK:
+               process_lock();
                if (l->u.f.fd != -1) {
                        close(l->u.f.fd);
                        l->u.f.fd = -1;
                }
+               process_unlock();
                if (l->u.f.fname) {
                        free(l->u.f.fname);
                        l->u.f.fname = NULL;
                }
                break;
        }
-out:
+}
+
+int process_lock(void)
+{
+       int ret;
+       ret = pthread_mutex_lock(&thread_mutex);
+       if (ret != 0)
+               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+       return ret;
+}
+
+void process_unlock(void)
+{
        pthread_mutex_unlock(&thread_mutex);
 }
+
+int container_mem_lock(struct lxc_container *c)
+{
+       return lxclock(c->privlock, 0);
+}
+
+void container_mem_unlock(struct lxc_container *c)
+{
+       lxcunlock(c->privlock);
+}
+
+int container_disk_lock(struct lxc_container *c)
+{
+       int ret;
+
+       if ((ret = lxclock(c->privlock, 0)))
+               return ret;
+       if ((ret = lxclock(c->slock, 0))) {
+               lxcunlock(c->privlock);
+               return ret;
+       }
+       return 0;
+}
+
+void container_disk_unlock(struct lxc_container *c)
+{
+       lxcunlock(c->slock);
+       lxcunlock(c->privlock);
+}

diff --git a/src/lxc/lxclock.h b/src/lxc/lxclock.h
index 61e7bbb9bc0005a0596c7aa6b4a1d25d61105432..0438353ef957cbf9a0053f511ffdee519803a5a7 100644 (file)
--- a/src/lxc/lxclock.h
+++ b/src/lxc/lxclock.h
@@ -1,3 +1,5 @@
+#ifndef __LXCLOCK_H
+#define __LXCLOCK_H
 /* liblxcapi
  *
  * Copyright © 2012 Serge Hallyn <serge.hallyn@ubuntu.com>.
@@ -53,7 +55,7 @@ struct lxc_lock {
  * We use that to protect the containers as represented on disk.
  * lxc_newlock() for the named lock only allocates the pathname in
  * memory so we can quickly open+lock it at lxclock.
- * l->u.f.fname will container the malloc'ed name (which must be
+ * l->u.f.fname will contain the malloc'ed name (which must be
  * freed when the container is freed), and u.f.fd = -1.
  *
  * return lxclock on success, NULL on failure.
@@ -81,3 +83,12 @@ extern int lxclock(struct lxc_lock *lock, int timeout);
 extern int lxcunlock(struct lxc_lock *lock);
 
 extern void lxc_putlock(struct lxc_lock *l);
+
+extern int process_lock(void);
+extern void process_unlock(void);
+struct lxc_container;
+extern int container_mem_lock(struct lxc_container *c);
+extern void container_mem_unlock(struct lxc_container *c);
+extern int container_disk_lock(struct lxc_container *c);
+extern void container_disk_unlock(struct lxc_container *c);
+#endif