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

Reply via email to