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