Hi Serge,
Thanks for the review. If you are not in a hurry I can send another round
that includes your suggestions in couple of days.
Best,
On Mon, Apr 15, 2013 at 1:12 PM, Serge Hallyn <serge.hal...@ubuntu.com>wrote:
> Quoting S.Çağlar Onur (cag...@10ur.org):
> > From: "S.Çağlar Onur" <cag...@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@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/lxc-devel
>
--
S.Çağlar Onur <cag...@10ur.org>
------------------------------------------------------------------------------
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@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
