[lxc-devel] [PATCH] lxc-ls: check for ENOMEM and tweaking

[Christian Brauner]

On Fri, Jan 15, 2016 at 08:40:19PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christian.brauner at 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.
alloca()?

> 
> > Signed-off-by: Christian Brauner <christian.brauner at 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'.
Thanks!

> 
> >  
> > @@ -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)
Are you mocking my vim skills? :D

I'll just group the ls_nesting = 0 patch and this patch together and send again.