Hi Serge,
I find some free time today so I just sent them again for review.
On Mon, Apr 15, 2013 at 2:00 PM, S.Çağlar Onur <cag...@10ur.org> wrote:
> 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>
>
--
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
