[lxc-devel] [PATCH 1/1] api locking: take process lock around open/close
Serge Hallyn
serge.hallyn at ubuntu.com
Thu May 30 03:21:22 UTC 2013
And update the comment explaining the locking.
Also take memlock in want_daemonize.
Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
---
src/lxc/lxccontainer.c | 153 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 115 insertions(+), 38 deletions(-)
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 9bc1caf..145ed7c 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -131,7 +131,12 @@ void remove_partial(struct lxc_container *c, int fd)
char *path = alloca(len);
int ret;
+ if (process_lock()) {
+ ERROR("Failed getting process lock");
+ return;
+ }
close(fd);
+ process_unlock();
ret = snprintf(path, len, "%s/%s/partial", c->config_path, c->name);
if (ret < 0 || ret >= len) {
ERROR("Error writing partial pathname");
@@ -145,11 +150,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
+ * 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
- * slock is an flock, which does not exclude threads. Therefore slock should
- * always be wrapped inside privlock.
* 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 +165,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.
@@ -358,11 +363,14 @@ static pid_t lxcapi_init_pid(struct lxc_container *c)
static bool load_config_locked(struct lxc_container *c, const char *fname)
{
+ bool ret = false;
if (!c->lxc_conf)
c->lxc_conf = lxc_conf_init();
+ process_lock();
if (c->lxc_conf && !lxc_config_read(fname, c->lxc_conf))
- return true;
- return false;
+ ret = true;
+ process_unlock();
+ return ret;
}
static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
@@ -406,7 +414,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)
@@ -512,6 +525,7 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
process_unlock();
return ret;
}
+ // In a forked task, no longer threaded
process_unlock();
/* second fork to be reparented by init */
pid = fork();
@@ -1023,7 +1037,9 @@ char** lxcapi_get_ips(struct lxc_container *c, char* interface, char* family, in
goto out;
/* Save reference to old netns */
+ process_lock();
old_netns = open("/proc/self/ns/net", O_RDONLY);
+ process_unlock();
if (old_netns < 0) {
SYSERROR("failed to open /proc/self/ns/net");
goto out;
@@ -1034,7 +1050,9 @@ char** lxcapi_get_ips(struct lxc_container *c, char* interface, char* family, in
if (ret < 0 || ret >= MAXPATHLEN)
goto out;
+ process_lock();
new_netns = open(new_netns_path, O_RDONLY);
+ process_unlock();
if (new_netns < 0) {
SYSERROR("failed to open %s", new_netns_path);
goto out;
@@ -1097,10 +1115,12 @@ out:
/* Switch back to original netns */
if (old_netns >= 0 && setns(old_netns, CLONE_NEWNET))
SYSERROR("failed to setns");
+ process_lock();
if (new_netns >= 0)
close(new_netns);
if (old_netns >= 0)
close(old_netns);
+ process_unlock();
/* Append NULL to the array */
if (count) {
@@ -1194,11 +1214,15 @@ static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
if (lret)
return false;
+ process_lock();
fout = fopen(alt_file, "w");
+ process_unlock();
if (!fout)
goto out;
write_config(fout, c->lxc_conf);
+ process_lock();
fclose(fout);
+ process_unlock();
ret = true;
out:
@@ -1214,16 +1238,13 @@ static bool lxcapi_destroy(struct lxc_container *c)
{
struct bdev *r = NULL;
bool ret = false;
+ int v;
if (!c || !lxcapi_is_defined(c))
return false;
- if (lxclock(c->privlock, 0))
+ if (container_disk_lock(c))
return false;
- if (lxclock(c->slock, 0)) {
- lxcunlock(c->privlock);
- return false;
- }
if (!is_stopped(c)) {
// we should queue some sort of error - in c->error_string?
@@ -1231,10 +1252,16 @@ 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->rootfs.path && c->lxc_conf->rootfs.mount) {
+ process_lock();
r = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
+ process_unlock();
+ }
if (r) {
- if (r->ops->destroy(r) < 0) {
+ process_lock();
+ v = r->ops->destroy(r);
+ process_unlock();
+ if (v < 0) {
ERROR("Error destroying rootfs for %s", c->name);
goto out;
}
@@ -1243,15 +1270,17 @@ static bool lxcapi_destroy(struct lxc_container *c)
const char *p1 = lxcapi_get_config_path(c);
char *path = alloca(strlen(p1) + strlen(c->name) + 2);
sprintf(path, "%s/%s", p1, c->name);
- if (lxc_rmdir_onedev(path) < 0) {
+ process_lock();
+ v = lxc_rmdir_onedev(path);
+ process_unlock();
+ if (v < 0) {
ERROR("Error destroying container directory for %s", c->name);
goto out;
}
ret = true;
out:
- lxcunlock(c->privlock);
- lxcunlock(c->slock);
+ container_disk_unlock(c);
return ret;
}
@@ -1374,23 +1403,17 @@ 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))
- return false;
-
if (is_stopped(c))
- goto err;
+ return false;
+ process_lock();
ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
- if (!ret)
- b = true;
-err:
- container_mem_unlock(c);
- return b;
+ process_unlock();
+ return ret == 0;
}
static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, char *retv, int inlen)
@@ -1400,32 +1423,41 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
if (!c || !c->lxc_conf)
return -1;
- if (container_mem_lock(c))
- return -1;
-
if (is_stopped(c))
- goto out;
+ return -1;
+ process_lock();
ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
+ process_unlock();
-out:
- container_mem_unlock(c);
return ret;
}
const char *lxc_get_default_config_path(void)
{
- return default_lxc_path();
+ const char *ret;
+ process_lock();
+ ret = default_lxc_path();
+ process_unlock();
+ return ret;
}
const char *lxc_get_default_lvm_vg(void)
{
- return default_lvm_vg();
+ const char *ret;
+ process_lock();
+ ret = default_lvm_vg();
+ process_unlock();
+ return ret;
}
const char *lxc_get_default_zfs_root(void)
{
- return default_zfs_root();
+ const char *ret;
+ process_lock();
+ ret = default_zfs_root();
+ process_unlock();
+ return ret;
}
const char *lxc_get_version(void)
@@ -1450,12 +1482,16 @@ static int copy_file(char *old, char *new)
return -1;
}
+ process_lock();
in = open(old, O_RDONLY);
+ process_unlock();
if (in < 0) {
SYSERROR("opening original file %s", old);
return -1;
}
+ process_lock();
out = open(new, O_CREAT | O_EXCL | O_WRONLY, 0644);
+ process_unlock();
if (out < 0) {
SYSERROR("opening new file %s", new);
close(in);
@@ -1476,8 +1512,10 @@ static int copy_file(char *old, char *new)
goto err;
}
}
+ process_lock();
close(in);
close(out);
+ process_unlock();
// we set mode, but not owner/group
ret = chmod(new, sbuf.st_mode);
@@ -1489,8 +1527,10 @@ static int copy_file(char *old, char *new)
return 0;
err:
+ process_lock();
close(in);
close(out);
+ process_unlock();
return -1;
}
@@ -1547,48 +1587,67 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
const char *p0, *p1, *p2, *end;
const char *oldpath = oldc->get_config_path(oldc);
const char *oldname = oldc->name;
+ int ret;
+ process_lock();
f = fopen(path, "r");
+ process_unlock();
if (!f) {
SYSERROR("opening old config");
return -1;
}
if (fseek(f, 0, SEEK_END) < 0) {
SYSERROR("seeking to end of old config");
+ process_lock();
fclose(f);
+ process_unlock();
return -1;
}
flen = ftell(f);
if (flen < 0) {
+ process_lock();
fclose(f);
+ process_unlock();
SYSERROR("telling size of old config");
return -1;
}
if (fseek(f, 0, SEEK_SET) < 0) {
+ process_lock();
fclose(f);
+ process_unlock();
SYSERROR("rewinding old config");
return -1;
}
contents = malloc(flen+1);
if (!contents) {
SYSERROR("out of memory");
+ process_lock();
fclose(f);
+ process_unlock();
return -1;
}
if (fread(contents, 1, flen, f) != flen) {
free(contents);
+ process_lock();
fclose(f);
+ process_unlock();
SYSERROR("reading old config");
return -1;
}
contents[flen] = '\0';
- if (fclose(f) < 0) {
+
+ process_lock();
+ ret = fclose(f);
+ process_unlock();
+ if (ret < 0) {
free(contents);
SYSERROR("closing old config");
return -1;
}
+ process_lock();
f = fopen(path, "w");
+ process_unlock();
if (!f) {
SYSERROR("reopening config");
free(contents);
@@ -1605,11 +1664,15 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
if (fwrite(p0, 1, (end-p0), f) != (end-p0)) {
SYSERROR("writing new config");
free(contents);
+ process_lock();
fclose(f);
+ process_unlock();
return -1;
}
free(contents);
+ process_lock();
fclose(f);
+ process_unlock();
// success
return 0;
} else {
@@ -1618,7 +1681,9 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
if (fwrite(p0, 1, (p-p0), f) != (p-p0)) {
SYSERROR("writing new config");
free(contents);
+ process_lock();
fclose(f);
+ process_unlock();
return -1;
}
p0 = p;
@@ -1626,7 +1691,9 @@ static int update_name_and_paths(const char *path, struct lxc_container *oldc,
if (fwrite(new, 1, strlen(new), f) != strlen(new)) {
SYSERROR("writing new name or path in new config");
free(contents);
+ process_lock();
fclose(f);
+ process_unlock();
return -1;
}
p0 += (p == p2) ? strlen(oldname) : strlen(oldpath);
@@ -1671,13 +1738,19 @@ static int copyhooks(struct lxc_container *oldc, struct lxc_container *c)
static void new_hwaddr(char *hwaddr)
{
- FILE *f = fopen("/dev/urandom", "r");
+ FILE *f;
+
+ process_lock();
+ f = fopen("/dev/urandom", "r");
+ process_unlock();
if (f) {
unsigned int seed;
int ret = fread(&seed, sizeof(seed), 1, f);
if (ret != 1)
seed = time(NULL);
+ process_lock();
fclose(f);
+ process_unlock();
srand(seed);
} else
srand(time(NULL));
@@ -1891,13 +1964,17 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
}
// copy the configuration, tweak it as needed,
+ process_lock();
fout = fopen(newpath, "w");
+ process_unlock();
if (!fout) {
SYSERROR("open %s", newpath);
goto out;
}
write_config(fout, c->lxc_conf);
+ process_lock();
fclose(fout);
+ process_unlock();
if (update_name_and_paths(newpath, c, n, l) < 0) {
ERROR("Error updating name in cloned config");
--
1.8.1.2
More information about the lxc-devel
mailing list