Quoting Christian Brauner (christian.brau...@mailbox.org): > - If lxc_container_new() fails we check for ENOMEM and if so goto out. If > ENOMEM is not set we will simply continue. The same goes for the call to > regcomp() but instead of checking for ENOMEM we need to check for > REG_ESPACE. > > - Tweaking: Since lxc-ls might have to gather a lot of containers and I don't > know if compilers will always optimize this, let's move *some* variable > declarations outside of the loop when it does not hinder readability. > > - Set ls_nesting to 0 initially. Otherwise users will always see nested > containers printed. > > - ls_get() gains an argument char **lockpath which is a string pointing us to > the lock we put under /run/lxc/lock/.../... so that we can remove the lock > when we no longer need it. To avoid pointless memory allocation in each new > recursion level, we share lockpath amongst all non-fork()ing recursive calls > to ls_get(). As it is not guaranteed that realloc() does not do any memory > moving when newlen == len_lockpath, we give ls_get() an additional argument > size_t len_lockpath). Every time we have a non-fork()ing recursive call to > ls_get() we check if newlen > len_lockpath and only then do we > realloc(*lockpath, newlen * 2) a reasonable chunk of memory (as the path > will > keep growing) and set len_lockpath = newlen * 2 to pass to the next > non-fork()ing recursive call to ls_get(). > To avoid keeping a variable char *lockpath in main() which serves no purpose > whatsoever and might be abused later we use a compound literal > &(char *){NULL} which gives us an anonymous pointer. This pointer we can use > for memory allocation in ls_get() for lockpath. We can conveniently free() > it > in ls_get() when the nesting level parameter lvl == 0 after exiting the > loop. > The advantage is that the variable is only accessible within ls_get() and > not > in main() while at the same time giving us an easy way to share lockpath > amongst all non-fork()ing recursive calls to ls_get(). > > Signed-off-by: Christian Brauner <christian.brau...@mailbox.org>
Thanks Acked-by: Serge E. Hallyn <serge.hal...@ubuntu.com> > --- > First v3 version of patch somehow did not make it to the mailing list so > resending. > > Changelog: > 2016-01-16T0013: > - actually reallocate newlen * 2 and not simply newlen to > avoid calling realloc() unnecessarily often > 2016-01-16T1752: > - rework: dynamically allocate lock_path instead of > lock_path[MAXPATHLEN]: > Move removing the lock to a separate function > ls_remove_lock() which we call before and after any > non-fork()ing recursive calls to ls_get() to remove the > lock > from /run/lxc/lock/.../... > - ls_get() gains arguments char **lockpath and > size_t len_lockpath. We share lockpath amongst all > non-fork()ing recursive calls to ls_get() and use > len_lockpath to check if realloc()ing makes sense or not. > Details for the implementation can be found in the commit > message. > 2016-01-16T0100: > - free() grp_tmp when ls_new() fails to prevent memory leak. > - dynamically allocate lock_path instead of > lock_path[MAXPATHLEN]: > I don't know if realloc() will gain us much here. Instead > we > simply call malloc() and free() it once the recursive > function > returns. > - set ls_nesting = 0 to prevent always listing nesting > containers > --- > src/lxc/lxc_ls.c | 108 > +++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 73 insertions(+), 35 deletions(-) > > diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c > index a4ab9b6..bfe37cb 100644 > --- a/src/lxc/lxc_ls.c > +++ b/src/lxc/lxc_ls.c > @@ -94,7 +94,7 @@ static void ls_free_arr(char **arr, size_t size); > */ > static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments > *args, > struct lengths *lht, const char *basepath, const char *parent, > - unsigned int lvl); > + unsigned int lvl, char **lockpath, size_t len_lockpath); > static char *ls_get_cgroup_item(struct lxc_container *c, const char *item); > static char *ls_get_config_item(struct lxc_container *c, const char *item, > bool running); > @@ -145,6 +145,8 @@ static void ls_print_table(struct ls *l, struct lengths > *lht, > static int ls_read_and_grow_buf(const int rpipefd, char **save_buf, > const char *id, ssize_t nbytes_id, > char **read_buf, ssize_t *read_buf_len); > +static int ls_remove_lock(const char *path, const char *name, > + char **lockpath, size_t *len_lockpath, bool recalc); > static int ls_serialize(int wpipefd, struct ls *n); > static int ls_write(const int wpipefd, const char *id, ssize_t nbytes_id, > const char *s); > @@ -186,13 +188,11 @@ Options :\n\ > -g --groups comma separated list of groups a container must have to > be displayed\n", > .options = my_longopts, > .parser = my_parser, > - .ls_nesting = MAX_NESTLVL, > + .ls_nesting = 0, > }; > > int main(int argc, char *argv[]) > { > - struct ls *ls_arr = NULL; > - size_t ls_size = 0; > int ret = EXIT_FAILURE; > /* > * The lxc parser requires that my_args.name is set. So let's satisfy > @@ -228,7 +228,12 @@ int main(int argc, char *argv[]) > .autostart_length = 9, /* AUTOSTART */ > }; > > - int status = ls_get(&ls_arr, &ls_size, &my_args, &max_len, "", NULL, 0); > + struct ls *ls_arr = NULL; > + size_t ls_size = 0; > + /* &(char *){NULL} is no magic. It's just a compound literal which > + * avoids having a pointless variable in main() that serves no purpose > + * here. */ > + int status = ls_get(&ls_arr, &ls_size, &my_args, &max_len, "", NULL, 0, > &(char *){NULL}, 0); > if (!ls_arr && status == 0) > /* We did not fail. There was just nothing to do. */ > exit(EXIT_SUCCESS); > @@ -303,7 +308,7 @@ static void ls_free_arr(char **arr, size_t size) > > static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments > *args, > struct lengths *lht, const char *basepath, const char *parent, > - unsigned int lvl) > + unsigned int lvl, char **lockpath, size_t len_lockpath) > { > /* As ls_get() is non-tail recursive we face the inherent danger of > * blowing up the stack at some level of nesting. To have at least some > @@ -341,6 +346,8 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > goto out; > } > > + char *tmp = NULL; > + int check; > struct ls *l = NULL; > struct lxc_container *c = NULL; > size_t i; > @@ -350,17 +357,23 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > /* Filter container names by regex the user gave us. */ > if (args->ls_regex) { > regex_t preg; > - if (regcomp(&preg, args->ls_regex, REG_NOSUB | > REG_EXTENDED) != 0) > + check = regcomp(&preg, args->ls_regex, REG_NOSUB | > REG_EXTENDED); > + if (check == REG_ESPACE) /* we're out of memory */ > + goto out; > + else if (check != 0) > continue; > - int rc = regexec(&preg, name, 0, NULL, 0); > + check = regexec(&preg, name, 0, NULL, 0); > regfree(&preg); > - if (rc != 0) > + if (check != 0) > continue; > } > > + errno = 0; > c = lxc_container_new(name, path); > - if (!c) > - continue; > + if ((errno == ENOMEM) && !c) > + goto out; > + else if (!c) > + continue; > > if (!c->is_defined(c)) > goto put_and_next; > @@ -390,8 +403,10 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > > /* Now it makes sense to allocate memory. */ > l = ls_new(m, size); > - if (!l) > + if (!l) { > + free(grp_tmp); > goto put_and_next; > + } > > /* How deeply nested are we? */ > l->nestlvl = lvl; > @@ -422,7 +437,7 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > if (!l->state) > goto put_and_next; > > - char *tmp = ls_get_config_item(c, "lxc.start.auto", > running); > + tmp = ls_get_config_item(c, "lxc.start.auto", running); > if (tmp) > l->autostart = atoi(tmp); > free(tmp); > @@ -451,11 +466,9 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > * all other information we need. */ > if (args->ls_nesting && running) { > struct wrapargs wargs = (struct wrapargs){.args = NULL}; > - > - /* Open a socket so that the child can communicate with > - * us. */ > - int ret = socketpair(PF_LOCAL, SOCK_STREAM | > SOCK_CLOEXEC, 0, wargs.pipefd); > - if (ret == -1) > + /* Open a socket so that the child can communicate with > us. */ > + check = socketpair(PF_LOCAL, SOCK_STREAM | > SOCK_CLOEXEC, 0, wargs.pipefd); > + if (check == -1) > goto put_and_next; > > /* Set the next nesting level. */ > @@ -470,14 +483,14 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > lxc_attach_options_t aopt = LXC_ATTACH_OPTIONS_DEFAULT; > aopt.env_policy = LXC_ATTACH_CLEAR_ENV; > > - /* fork(): Attach to the namespace of the child and run > - * ls_get() in it which is called in ls_get_wrapper(). > */ > - int status = c->attach(c, ls_get_wrapper, &wargs, > &aopt, &out); > + /* fork(): Attach to the namespace of the container and > + * run ls_get() in it which is called in * > ls_get_wrapper(). */ > + check = c->attach(c, ls_get_wrapper, &wargs, &aopt, > &out); > /* close the socket */ > close(wargs.pipefd[1]); > > /* Retrieve all information we want from the child. */ > - if (status == 0) > + if (check == 0) > if (ls_deserialize(wargs.pipefd[0], m, size) == > -1) > goto put_and_next; > > @@ -509,23 +522,17 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > if (!newpath) > goto put_and_next; > > - /* We want to remove all locks we created under > + /* We want to remove all locks we create under > * /run/lxc/lock so we create a string pointing us to > * the lock path for the current container. */ > - char lock_path[MAXPATHLEN]; > - int ret = snprintf(lock_path, MAXPATHLEN, > "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name); > - if (ret < 0 || ret >= MAXPATHLEN) > + if (ls_remove_lock(path, name, lockpath, &len_lockpath, > true) == -1) > goto put_and_next; > > - /* Remove the lock. */ > - lxc_rmdir_onedev(lock_path, NULL); > - > - ls_get(m, size, args, lht, newpath, l->name, lvl + 1); > - > - /* Remove the lock. */ > - lxc_rmdir_onedev(lock_path, NULL); > - > + ls_get(m, size, args, lht, newpath, l->name, lvl + 1, > lockpath, len_lockpath); > free(newpath); > + > + /* Remove the lock. No need to check for failure here. > */ > + ls_remove_lock(path, name, lockpath, &len_lockpath, > false) > } > > put_and_next: > @@ -536,6 +543,10 @@ put_and_next: > out: > ls_free_arr(containers, num); > free(path); > + /* lockpath is shared amongst all non-fork()ing recursive calls to > + * ls_get() so only free it on the uppermost level. */ > + if (lvl == 0) > + free(*lockpath); > > return ret; > } > @@ -932,7 +943,10 @@ static int ls_get_wrapper(void *wrap) > /* close pipe */ > close(wargs->pipefd[0]); > > - ls_get(&m, &len, wargs->args, wargs->lht, "", wargs->parent, > wargs->nestlvl); > + /* &(char *){NULL} is no magic. It's just a compound literal which > + * avoids having a pointless variable in main() that serves no purpose > + * here. */ > + ls_get(&m, &len, wargs->args, wargs->lht, "", wargs->parent, > wargs->nestlvl, &(char *){NULL}, 0); > if (!m) > goto out; > > @@ -955,6 +969,30 @@ out: > return ret; > } > > +static int ls_remove_lock(const char *path, const char *name, > + char **lockpath, size_t *len_lockpath, bool recalc) > +{ > + /* Avoid doing unnecessary work if we can. */ > + if (recalc) { > + size_t newlen = strlen(path) + strlen(name) + > strlen(RUNTIME_PATH) + /* /+lxc+/+lock+/+/= */ 11 + 1; > + if (newlen > *len_lockpath) { > + char *tmp = realloc(*lockpath, newlen * 2); > + if (!tmp) > + return -1; > + *lockpath = tmp; > + *len_lockpath = newlen * 2; > + } > + } > + > + int check = snprintf(*lockpath, *len_lockpath, "%s/lxc/lock/%s/%s", > RUNTIME_PATH, path, name); > + if (check < 0 || check >= *len_lockpath) > + return -1; > + > + lxc_rmdir_onedev(*lockpath, NULL); > + > + return 0; > +} > + > static int ls_serialize(int wpipefd, struct ls *n) > { > ssize_t nbytes = sizeof(n->ram); > -- > 2.7.0 > _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel