[lxc-devel] [PATCH] cgmanager get/set: clean up child

[Dwight Engen]

On Fri, 15 Aug 2014 16:20:06 +0000
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> Quoting Dwight Engen (dwight.engen at oracle.com):
> > On Thu, 14 Aug 2014 21:56:04 +0000
> > Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > 
> > > Make sure we reap our child at cgm_{s,g}et.
> > > 
> > > Not doing this resulted in a defunct child of Tycho's
> > > lxc-restore.
> > > 
> > > Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> > 
> > Acked-by: Dwight Engen <dwight.engen at oracle.com>
> > (with one question below)
> > 
> > > ---
> > >  src/lxc/cgmanager.c | 29 ++++++++++++++++++++---------
> > >  1 file changed, 20 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/src/lxc/cgmanager.c b/src/lxc/cgmanager.c
> > > index c4f48e9..c46f80d 100644
> > > --- a/src/lxc/cgmanager.c
> > > +++ b/src/lxc/cgmanager.c
> > > @@ -794,32 +794,37 @@ static int cgm_get(const char *filename,
> > > char *value, size_t len, const char *na close(p[1]);
> > >  		return -1;
> > >  	}
> > > -	if (!pid)
> > > +	if (!pid) // do_cgm_get exits
> > >  		do_cgm_get(name, lxcpath, filename, p[1], len &&
> > > value); close(p[1]);
> > >  	ret = read(p[0], &newlen, sizeof(newlen));
> > >  	if (ret != sizeof(newlen)) {
> > >  		close(p[0]);
> > > -		return -1;
> > > +		ret = -1;
> > > +		goto out;
> > >  	}
> > >  	if (!len || !value) {
> > >  		close(p[0]);
> > > -		return newlen;
> > > +		ret = newlen;
> > > +		goto out;
> > >  	}
> > >  	memset(value, 0, len);
> > > +	ret = -1;
> > >  	if (newlen < 0) { // child is reporting an error
> > >  		close(p[0]);
> > > -		return -1;
> > > +		goto out;
> > >  	}
> > >  	if (newlen == 0) { // empty read
> > >  		close(p[0]);
> > > -		return 0;
> > > +		goto out;
> > 
> > So this is a bug fix here because it looks like it used to return 0
> > and now it will return -1?
> 
> D'oh, no I'm not sure about that.  For instance, cpuset.mems could be
> empty, which could cause an empty read.  (Right?)

Makes sense to me. Maybe you just wanna set ret inside each block.
 
> > >  	}
> > >  	readlen = newlen > len ? len : newlen;
> > >  	ret = read(p[0], value, readlen);
> > >  	close(p[0]);
> > > -	if (ret != readlen)
> > > -		return -1;
> > > +	if (ret != readlen) {
> > > +		ret = -1;
> > > +		goto out;
> > > +	}
> > >  	if (newlen >= len) {
> > >  		value[len-1] = '\0';
> > >  		newlen = len-1;
> > > @@ -828,7 +833,11 @@ static int cgm_get(const char *filename, char
> > > *value, size_t len, const char *na value[newlen++] = '\n';
> > >  		value[newlen] = '\0';
> > >  	}
> > > -	return newlen;
> > > +	ret = newlen;
> > > +out:
> > > +	if (wait_for_pid(pid))
> > > +		WARN("do_cgm_get exited with error");
> > > +	return ret;
> > >  }
> > >  
> > >  static void do_cgm_set(const char *name, const char *lxcpath,
> > > const char *filename, const char *value, int outp) @@ -920,11
> > > +929,13 @@ static int cgm_set(const char *filename, const char
> > > *value, const char *name, co close(p[0]); return -1;
> > >  	}
> > > -	if (!pid)
> > > +	if (!pid) // do_cgm_set exits
> > >  		do_cgm_set(name, lxcpath, filename, value, p[1]);
> > >  	close(p[1]);
> > >  	ret = read(p[0], &v, sizeof(v));
> > >  	close(p[0]);
> > > +	if (wait_for_pid(pid))
> > > +		WARN("do_cgm_set exited with error");
> > >  	if (ret != sizeof(v) || !v)
> > >  		return -1;
> > >  	return 0;
> > 
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel