[lxc-devel] [PATCH 2/2] Support stopping containers concurrently
Serge Hallyn
serge.hallyn at ubuntu.com
Mon Apr 15 17:12:49 UTC 2013
Quoting S.Çağlar Onur (caglar at 10ur.org):
> From: "S.Çağlar Onur" <caglar at 10ur.org>
>
> Trying to stop multiple containers concurrently ends up with "cgroup is not mounted" errors as multiple threads corrupts the shared variables.
> Fix that stack corruption and start to use getmntent_r to support stopping multiple containers concurrently.
> ---
Thanks, yeah, you can tell we've not had multithreaded api users yet -
this is great :)
A few comments though. I can wait for a new patch, or if you prefer I
can address them myself inline. (We currently don't yet try to
guarantee bisectability, but Id like us to start keeping it in mind :)
> src/lxc/cgroup.c | 133 ++++++++++++++++++++++++++++++++++-------------------
> src/lxc/freezer.c | 14 +++++-
> src/lxc/state.c | 13 ++++--
> 3 files changed, 107 insertions(+), 53 deletions(-)
>
> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> index 812bfb8..fa85bcc 100644
> --- a/src/lxc/cgroup.c
> +++ b/src/lxc/cgroup.c
> @@ -98,29 +98,31 @@ static char *mount_has_subsystem(const struct mntent *mntent)
> */
> static int get_cgroup_mount(const char *subsystem, char *mnt)
> {
> - struct mntent *mntent;
> + struct mntent *mntent, mntent_r;
> FILE *file = NULL;
> int ret, err = -1;
>
> + char buf[MAXPATHLEN] = {0};
In the case of a bind mount, there could be two long pathnames in the
mntent plus options - is MAXPATHLEN deemed big enough? Are we better
off mallocing something like 3*MAXPATHLEN?
> +
> file = setmntent(MTAB, "r");
> if (!file) {
> SYSERROR("failed to open %s", MTAB);
> return -1;
> }
>
> - while ((mntent = getmntent(file))) {
> - if (strcmp(mntent->mnt_type, "cgroup"))
> + while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
> + if (strcmp(mntent_r.mnt_type, "cgroup") != 0)
> continue;
> -
> +
> if (subsystem) {
> - if (!hasmntopt(mntent, subsystem))
> + if (!hasmntopt(&mntent_r, subsystem))
> continue;
> } else {
> - if (!mount_has_subsystem(mntent))
> + if (!mount_has_subsystem(&mntent_r))
> continue;
> }
>
> - ret = snprintf(mnt, MAXPATHLEN, "%s", mntent->mnt_dir);
> + ret = snprintf(mnt, MAXPATHLEN, "%s", mntent_r.mnt_dir);
> if (ret < 0 || ret >= MAXPATHLEN)
> goto fail;
>
> @@ -146,22 +148,24 @@ out:
> *
> * Returns 0 on success, -1 on error.
> *
> - * The answer is written in a static char[MAXPATHLEN] in this function and
> - * should not be freed.
> */
> extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath)
> {
> - static char buf[MAXPATHLEN];
> - static char retbuf[MAXPATHLEN];
> int rc;
>
> + char *buf;
> + char *retbuf;
> +
> + buf = malloc(MAXPATHLEN * sizeof(char));
> + retbuf = malloc(MAXPATHLEN * sizeof(char));
Best to check that the allocations succeeded.
> +
> /* lxc_cgroup_set passes a state object for the subsystem,
> * so trim it to just the subsystem part */
> if (subsystem) {
> rc = snprintf(retbuf, MAXPATHLEN, "%s", subsystem);
> if (rc < 0 || rc >= MAXPATHLEN) {
> ERROR("subsystem name too long");
> - return -1;
> + goto fail;
> }
> char *s = index(retbuf, '.');
> if (s)
> @@ -170,19 +174,28 @@ extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpat
> }
> if (get_cgroup_mount(subsystem ? retbuf : NULL, buf)) {
> ERROR("cgroup is not mounted");
> - return -1;
> + goto fail;
> }
>
> rc = snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, cgpath);
> if (rc < 0 || rc >= MAXPATHLEN) {
> ERROR("name too long");
> - return -1;
> + goto fail;
> }
>
> DEBUG("%s: returning %s for subsystem %s", __func__, retbuf, subsystem);
>
> + if(buf)
> + free(buf);
> +
> *path = retbuf;
> return 0;
> +fail:
> + if (buf)
> + free(buf);
> + if (retbuf)
> + free(retbuf);
> + return -1;
> }
>
> /*
> @@ -295,15 +308,20 @@ int lxc_cgroup_set_bypath(const char *cgpath, const char *filename, const char *
>
> ret = cgroup_path_get(&dirpath, filename, cgpath);
> if (ret)
> - return -1;
> + goto fail;
>
> ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> if (ret < 0 || ret >= MAXPATHLEN) {
> ERROR("pathname too long");
> - return -1;
> + goto fail;
> }
>
> return do_cgroup_set(path, value);
> +
> +fail:
> + if(dirpath)
> + free(dirpath);
> + return -1;
> }
>
> /*
> @@ -326,15 +344,20 @@ int lxc_cgroup_set(const char *name, const char *filename, const char *value,
>
dirpath needs to be initialized to NULL at the top for this to work
out right.
> ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
> if (ret)
> - return -1;
note that in this case lxc_cgroup_path_get will not have assigned
dirpath at all so we know we won't need to free - but it's probably
more future-proof to do so.
> + goto fail;
>
> ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> if (ret < 0 || ret >= MAXPATHLEN) {
> ERROR("pathname too long");
> - return -1;
> + goto fail;
> }
>
> return do_cgroup_set(path, value);
> +
> +fail:
> + if(dirpath)
> + free(dirpath);
> + return -1;
> }
>
> /*
> @@ -367,18 +390,18 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
>
same here
> ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
> if (ret)
> - return -1;
> + goto fail;
>
> rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> if (rc < 0 || rc >= MAXPATHLEN) {
> ERROR("pathname too long");
> - return -1;
> + goto fail;
> }
>
> fd = open(path, O_RDONLY);
> if (fd < 0) {
> ERROR("open %s : %s", path, strerror(errno));
> - return -1;
> + goto fail;
> }
>
> if (!len || !value) {
> @@ -398,24 +421,28 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
>
> close(fd);
> return ret;
> +fail:
> + if(dirpath)
> + free(dirpath);
> + return -1;
> }
>
> int lxc_cgroup_nrtasks(const char *cgpath)
> {
> - char *dpath;
> + char *dirpath;
> char path[MAXPATHLEN];
> int pid, ret, count = 0;
> FILE *file;
> int rc;
>
and here.
> - ret = cgroup_path_get(&dpath, NULL, cgpath);
> + ret = cgroup_path_get(&dirpath, NULL, cgpath);
> if (ret)
> - return -1;
> + goto fail;
>
> - rc = snprintf(path, MAXPATHLEN, "%s/tasks", dpath);
> + rc = snprintf(path, MAXPATHLEN, "%s/tasks", dirpath);
> if (rc < 0 || rc >= MAXPATHLEN) {
> ERROR("pathname too long");
> - return -1;
> + goto fail;
> }
>
> file = fopen(path, "r");
> @@ -430,6 +457,10 @@ int lxc_cgroup_nrtasks(const char *cgpath)
> fclose(file);
>
> return count;
> +fail:
> + if(dirpath)
> + free(dirpath);
> + return -1;
> }
>
> /*
> @@ -470,21 +501,23 @@ static void set_clone_children(const char *mntdir)
> static int create_lxcgroups(const char *lxcgroup)
> {
> FILE *file = NULL;
> - struct mntent *mntent;
> + struct mntent *mntent, mntent_r;
> int ret, retv = -1;
> char path[MAXPATHLEN];
>
> + char buf[MAXPATHLEN] = {0};
same concern about sufficient size.
> +
> file = setmntent(MTAB, "r");
> if (!file) {
> SYSERROR("failed to open %s", MTAB);
> return -1;
> }
>
> - while ((mntent = getmntent(file))) {
> + while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
>
> - if (strcmp(mntent->mnt_type, "cgroup"))
> + if (strcmp(mntent_r.mnt_type, "cgroup"))
> continue;
> - if (!mount_has_subsystem(mntent))
> + if (!mount_has_subsystem(&mntent_r))
> continue;
>
> /*
> @@ -492,11 +525,11 @@ static int create_lxcgroups(const char *lxcgroup)
> * We probably only want to support that for /users/joe
> */
> ret = snprintf(path, MAXPATHLEN, "%s/%s",
> - mntent->mnt_dir, lxcgroup ? lxcgroup : "lxc");
> + mntent_r.mnt_dir, lxcgroup ? lxcgroup : "lxc");
> if (ret < 0 || ret >= MAXPATHLEN)
> goto fail;
> if (access(path, F_OK)) {
> - set_clone_children(mntent->mnt_dir);
> + set_clone_children(mntent_r.mnt_dir);
> ret = mkdir(path, 0755);
> if (ret == -1 && errno != EEXIST) {
> SYSERROR("failed to create '%s' directory", path);
> @@ -542,7 +575,9 @@ char *lxc_cgroup_path_create(const char *lxcgroup, const char *name)
> char *retpath, path[MAXPATHLEN];
> char tail[12];
> FILE *file = NULL;
> - struct mntent *mntent;
> + struct mntent *mntent, mntent_r;
> +
> + char buf[MAXPATHLEN] = {0};
>
> if (create_lxcgroups(lxcgroup) < 0)
> return NULL;
> @@ -559,15 +594,15 @@ again:
> else
> *tail = '\0';
>
> - while ((mntent = getmntent(file))) {
> + while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
>
> - if (strcmp(mntent->mnt_type, "cgroup"))
> + if (strcmp(mntent_r.mnt_type, "cgroup"))
> continue;
> - if (!mount_has_subsystem(mntent))
> + if (!mount_has_subsystem(&mntent_r))
> continue;
>
> /* find unused mnt_dir + lxcgroup + name + -$i */
> - ret = snprintf(path, MAXPATHLEN, "%s/%s/%s%s", mntent->mnt_dir,
> + ret = snprintf(path, MAXPATHLEN, "%s/%s/%s%s", mntent_r.mnt_dir,
> lxcgroup ? lxcgroup : "lxc", name, tail);
> if (ret < 0 || ret >= MAXPATHLEN)
> goto fail;
> @@ -607,8 +642,10 @@ int lxc_cgroup_enter(const char *cgpath, pid_t pid)
> {
> char path[MAXPATHLEN];
> FILE *file = NULL, *fout;
> - struct mntent *mntent;
> + struct mntent *mntent, mntent_r;
> int ret, retv = -1;
> + char buf[MAXPATHLEN] = {0};
> +
>
> file = setmntent(MTAB, "r");
> if (!file) {
> @@ -616,13 +653,13 @@ int lxc_cgroup_enter(const char *cgpath, pid_t pid)
> return -1;
> }
>
> - while ((mntent = getmntent(file))) {
> - if (strcmp(mntent->mnt_type, "cgroup"))
> + while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
> + if (strcmp(mntent_r.mnt_type, "cgroup"))
> continue;
> - if (!mount_has_subsystem(mntent))
> + if (!mount_has_subsystem(&mntent_r))
> continue;
> ret = snprintf(path, MAXPATHLEN, "%s/%s/tasks",
> - mntent->mnt_dir, cgpath);
> + mntent_r.mnt_dir, cgpath);
> if (ret < 0 || ret >= MAXPATHLEN) {
> ERROR("entering cgroup");
> goto out;
> @@ -714,23 +751,25 @@ static int lxc_one_cgroup_destroy(struct mntent *mntent, const char *cgpath)
> */
> int lxc_cgroup_destroy(const char *cgpath)
> {
> - struct mntent *mntent;
> + struct mntent *mntent, mntent_r;
> FILE *file = NULL;
> int err, retv = 0;
>
> + char buf[MAXPATHLEN] = {0};
> +
> file = setmntent(MTAB, "r");
> if (!file) {
> SYSERROR("failed to open %s", MTAB);
> return -1;
> }
>
> - while ((mntent = getmntent(file))) {
> - if (strcmp(mntent->mnt_type, "cgroup"))
> + while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
> + if (strcmp(mntent_r.mnt_type, "cgroup"))
> continue;
> - if (!mount_has_subsystem(mntent))
> + if (!mount_has_subsystem(&mntent_r))
> continue;
>
> - err = lxc_one_cgroup_destroy(mntent, cgpath);
> + err = lxc_one_cgroup_destroy(&mntent_r, cgpath);
> if (err) // keep trying to clean up the others
> retv = -1;
> }
> diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
> index 111bc35..ca9aa9c 100644
> --- a/src/lxc/freezer.c
> +++ b/src/lxc/freezer.c
> @@ -125,9 +125,14 @@ static int freeze_unfreeze(const char *name, int freeze, const char *lxcpath)
>
> ret = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
> if (ret)
> - return -1;
> + goto fail;
>
> return do_unfreeze(nsgroup, freeze, name, lxcpath);
> +
> +fail:
> + if (nsgroup)
> + free(nsgroup);
> + return -1;
> }
>
> int lxc_freeze(const char *name, const char *lxcpath)
> @@ -148,7 +153,12 @@ int lxc_unfreeze_bypath(const char *cgpath)
>
> ret = cgroup_path_get(&nsgroup, "freezer", cgpath);
> if (ret)
> - return -1;
> + goto fail;
>
> return do_unfreeze(nsgroup, 0, NULL, NULL);
> +
> +fail:
> + if (nsgroup)
> + free(nsgroup);
> + return -1;
> }
> diff --git a/src/lxc/state.c b/src/lxc/state.c
> index c50ef00..f83c16b 100644
> --- a/src/lxc/state.c
> +++ b/src/lxc/state.c
> @@ -76,25 +76,30 @@ static int freezer_state(const char *name, const char *lxcpath)
>
> err = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
> if (err)
> - return -1;
nsgroup needs to be initialized to NULL at top.
> + goto fail;
>
> err = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
> if (err < 0 || err >= MAXPATHLEN)
> - return -1;
> + goto fail;
>
> file = fopen(freezer, "r");
> if (!file)
> - return -1;
> + goto fail;
>
> err = fscanf(file, "%s", status);
> fclose(file);
>
> if (err == EOF) {
> SYSERROR("failed to read %s", freezer);
> - return -1;
> + goto fail;
> }
>
> return lxc_str2state(status);
> +
> +fail:
> + if (nsgroup)
> + free(nsgroup);
> + return -1;
> }
>
> static lxc_state_t __lxc_getstate(const char *name, const char *lxcpath)
> --
> 1.7.10.4
>
>
> ------------------------------------------------------------------------------
> Precog is a next-generation analytics platform capable of advanced
> analytics on semi-structured data. The platform includes APIs for building
> apps and a phenomenal toolset for data science. Developers can use
> our toolset for easy data analysis & visualization. Get a free account!
> http://www2.precog.com/precogplatform/slashdotnewsletter
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel
More information about the lxc-devel
mailing list