[lxc-devel] [PATCH 1/1] api locking: take process lock around open/close
Stéphane Graber
stgraber at ubuntu.com
Fri May 31 15:39:17 UTC 2013
On 05/29/2013 11:21 PM, Serge Hallyn wrote:
> And update the comment explaining the locking.
>
> Also take memlock in want_daemonize.
>
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
Is that patch still relevant?
It doesn't appear to have been reviewed/applied to staging and touches
bits that have been changed with more recent patches, so just want to
confirm that I can ignore it.
> ---
> 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");
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20130531/fcafae80/attachment.pgp>
More information about the lxc-devel
mailing list