Quoting Christian Brauner (christian.brau...@mailbox.org): > If lxc_container_new() fails we check for ENOMEN and 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 all variable > declarations outside of the loop. They should still be fairly self > explanatory.
It's just a stack pointer move so it's not really needed, so I think readability should win out. Now the biggest penalty to this would be the lock_path[MAXPATHLEN], but that is *always* already declared before the recursive ls_get anyway, so we always take the hit. > Signed-off-by: Christian Brauner <christian.brau...@mailbox.org> > --- > src/lxc/lxc_ls.c | 57 > ++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 33 insertions(+), 24 deletions(-) > > diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c > index a4ab9b6..7d46426 100644 > --- a/src/lxc/lxc_ls.c > +++ b/src/lxc/lxc_ls.c > @@ -341,25 +341,37 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > goto out; > } > > + char lock_path[MAXPATHLEN]; > + char *curr_path, *name, *newpath, *tmp; > + const char *state_tmp; > + int check = 0; > + size_t i; > + bool running; > struct ls *l = NULL; > struct lxc_container *c = NULL; > - size_t i; > + struct wrapargs wargs = (struct wrapargs){.args = NULL}; > for (i = 0; i < (size_t)num; i++) { > - char *name = containers[i]; > + name = containers[i]; > > /* 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) > + if ((errno == ENOMEM) && !c) > + goto out; > + else if (!c) > continue; > > if (!c->is_defined(c)) > @@ -367,7 +379,7 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > > /* This does not allocate memory so no worries about freeing it > * when we goto next or out. */ > - const char *state_tmp = c->state(c); > + state_tmp = c->state(c); > if (!state_tmp) > state_tmp = "UNKNOWN"; > > @@ -380,11 +392,11 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > if (args->ls_stopped && strcmp(state_tmp, "STOPPED")) > goto put_and_next; > > - bool running = c->is_running(c); > + running = c->is_running(c); > > - char *grp_tmp = ls_get_groups(c, running); > - if (!ls_has_all_grps(grp_tmp, args->groups)) { > - free(grp_tmp); > + tmp = ls_get_groups(c, running); > + if (!ls_has_all_grps(tmp, args->groups)) { > + free(tmp); > goto put_and_next; > } Unrelated to this patch, but I just noticed that if ls_new() fails you'll leak the 'tmp'. > > @@ -396,7 +408,7 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > /* How deeply nested are we? */ > l->nestlvl = lvl; > > - l->groups = grp_tmp; > + l->groups = tmp; > > l->running = running; > > @@ -422,7 +434,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); > @@ -450,12 +462,10 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > /* Get nested containers: Only do this after we have gathered > * 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) > + check = socketpair(PF_LOCAL, SOCK_STREAM | > SOCK_CLOEXEC, 0, wargs.pipefd); > + if (check == -1) > goto put_and_next; > > /* Set the next nesting level. */ > @@ -472,12 +482,12 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > > /* 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); > + 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; > > @@ -495,7 +505,7 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > * need a path-extractor function. We face the same > * problem with the ovl_mkdir() function in > * lxcoverlay.{c,h}. */ > - char *curr_path = ls_get_config_item(c, "lxc.rootfs", > running); > + curr_path = ls_get_config_item(c, "lxc.rootfs", > running); > if (!curr_path) > goto put_and_next; > > @@ -504,7 +514,7 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > * nested containers. What we do is simply create a > * growing path which will lead us into the rootfs of > * the next container where it stores its containers. */ > - char *newpath = lxc_append_paths(basepath, curr_path); > + newpath = lxc_append_paths(basepath, curr_path); > free(curr_path); > if (!newpath) > goto put_and_next; > @@ -512,9 +522,8 @@ static int ls_get(struct ls **m, size_t *size, const > struct lxc_arguments *args, > /* We want to remove all locks we created 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) > + check = snprintf(lock_path, MAXPATHLEN, > "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name); > + if (check < 0 || ret >= MAXPATHLEN) You need to add a /g at the end of that :%s :) (^ ret) > goto put_and_next; > > /* Remove the lock. */ > -- > 2.7.0 > > _______________________________________________ > lxc-devel mailing list > lxc-devel@lists.linuxcontainers.org > http://lists.linuxcontainers.org/listinfo/lxc-devel _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel