[lxc-devel] [PATCH v2] lxc-ls: check for ENOMEM and tweaking
Christian Brauner
christian.brauner at mailbox.org
Sat Jan 16 01:07:11 UTC 2016
On Sat, Jan 16, 2016 at 12:50:01AM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christian.brauner at 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.
> >
> > Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
> > ---
> > Changelog:
> > 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 | 55 ++++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 34 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c
> > index a4ab9b6..31279fb 100644
> > --- a/src/lxc/lxc_ls.c
> > +++ b/src/lxc/lxc_ls.c
> > @@ -186,7 +186,7 @@ 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[])
> > @@ -315,7 +315,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
> > if (lvl > args->ls_nesting)
> > return 0;
> >
> > - int num = 0, ret = -1;
> > + int ret = -1;
>
> > char **containers = NULL;
> > /* If we, at some level of nesting, encounter a stopped container but
> > * want to retrieve nested containers we need to build an absolute path
> > @@ -331,6 +331,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
> > if (!dir_exists(path))
> > goto out;
> >
> > + int num = 0;
>
> .... :(
>
> > /* Do not do more work than is necessary right from the start. */
> > if (args->ls_active || (args->ls_active && args->ls_frozen))
> > num = list_active_containers(path, &containers, NULL);
> > @@ -341,6 +342,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 +353,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 +399,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 +433,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 +462,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. */
> > @@ -472,12 +481,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;
> >
> > @@ -512,20 +521,24 @@ 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)
> > + size_t len = strlen(path) + strlen(name) + strlen(RUNTIME_PATH) + /* /+lxc+/+lock+/+/= */ 11 + 1;
> > + char *lock_path = malloc(len);
>
> Actually I meant keep lock_path for the duration of the function, use
> realloc here, and free at the very end.
>
> Is there an advantage to doing it as here? If so (apart from too
> much churn from variables moving around)
>
Ah, I was unsure whether that would be efficient because we should probably
check whether the realloc makes sense (i.e. that newlen > oldlen). As far as I
know it is implementation defined whether realloc() moves the memory when the
size is the same (glibc does guarantee it though). If we need to do this then my
only reason for not using realloc() is the added visual complexity. But I guess
you had something like that (taking out all cruft) in mind:
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)
{
char *lock_path = NULL, *tmp = NULL;
int check;
struct ls *l = NULL;
struct lxc_container *c = NULL;
size_t i, len = 0, newlen = 0;
for (i = 0; i < (size_t)num; i++) {
if (args->ls_nesting && running) {
} else if (args->ls_nesting && !running) {
newlen = strlen(path) + strlen(name) + strlen(RUNTIME_PATH) + /* /+lxc+/+lock+/+/= */ 11 + 1;
if (len < newlen) {
tmp = realloc(lock_path, newlen * 2);
if (!tmp)
goto put_and_next;
lock_path = tmp;
len = newlen * 2;
}
check = snprintf(lock_path, MAXPATHLEN, "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name);
if (check < 0 || check >= MAXPATHLEN)
goto put_and_next;
lxc_rmdir_onedev(lock_path, NULL);
ls_get(m, size, args, lht, newpath, l->name, lvl + 1);
lxc_rmdir_onedev(lock_path, NULL);
}
put_and_next:
lxc_container_put(c);
}
ret = 0;
out:
ls_free_arr(containers, num);
free(lock_path);
free(path);
return ret;
}
More information about the lxc-devel
mailing list