[Lxc-users] [PATCH 2/2] cgroups: support cgroups mounted in multiple places

[Michael H. Warfield]

On Sun, 2011-06-26 at 18:49 -0500, Serge E. Hallyn wrote: 
> Quoting Michael H. Warfield (mhw at WittsEnd.com):
> > On Sun, 2011-06-26 at 14:00 -0500, Serge E. Hallyn wrote: 
> > > Quoting Michael H. Warfield (mhw at WittsEnd.com):
> > > 
> > > Thanks, Michael, good catch.
> > > 
> > > > > Now wait a minute.  Is that a typo here:
> > > 
> > > No it's not, but:
> > > 
> > > > > char *s = index(retbuf, '.');
> > > > > 
> > > > > If you're doing, in effect, a dirname here should that be this:
> > > > > 
> > > > > char *s = index(retbuf, '/');
> > > > > 
> > > > > IAC...  That "*s = '\0';" should include a NULL check.
> > > > > 
> > > > > Adding the NULL check and lxc-info works.
> > > > > 
> > > > > Looks like that subsystem name in the call to that routine is not what
> > > > > Serge thought it was.  I threw a print above the snprintf about just for
> > > > > giggles to print out the subsystem name being passed to it and this is
> > > > > what I got back...
> > > > > 
> > > > > [mhw at forest SPECS]$ sudo lxc-info -n Alcove
> > > > > subsystem name: "freezer"
> > > > > 'Alcove' is RUNNING
> > > > > 
> > > > > No wonder "s" was null.  No dot and no /.
> > > > 
> > > > I applied this patch and it got lxc-info working.  But it was a quick
> > > > hack just to address the NULL pointer.  Is it the correct fix?
> > > 
> > > No, it's not.
> > > 
> > > For the calls to this function that come from cgroup.c itself, '.' is the
> > > right thing.  The problem is that lxc_cgroup_set() and lxc_cgroup_get()
> > > pass in things like 'devices.allow'.  I was going to make the index
> > > conditional, but all the callers of this function pass in either a filename
> > > (with a '.' in it) or NULL.
> > > 
> > > I failed to notice these:
> > > 
> > > src/lxc/freezer.c:      ret = lxc_cgroup_path_get(&nsgroup, "freezer", name);
> > > src/lxc/state.c:        err = lxc_cgroup_path_get(&nsgroup, "freezer", name);
> > > 
> > > :)
> > > 
> > > These are what you are running into.
> > 
> > > So the thing to do is leave it searching for index(s, '.') but do nothing
> > > if s is NULL.
> > 
> > And that's what I believe results with my little hack.  Only truncate if
> 
> Oops, sorry, I didn't look closely enough and assumed your patch was
> switching to checking for '/'.
> 
> > there was a hit and s was non-null.  I see now from your comments that
> > the check on '.' was correct.  I was uncertain about the inputs and
> > outputs in this routine.  Checking for the NILL condition may not be
> > "the" solution, in this case, but it is still a best common practice to
> > check pointers like that.  Never can tell what may crop up in the
> > future.
> > 
> > > Really it would be cleaner to have lxc_cgroup_{sg}et() do the index, so
> > > that lxc_cgorup_path_get() always gets a subsystem or NULL.  I'm not doing
> > > that patch right now, though, trivial as it ought to be.
> > 
> > I hear you.

> So

> Acked-by: Serge Hallyn <serge.hallyn at ubuntu.com>

> to your patch to fix my bug, and let's leave it at that for now until it
> gets more testing?

Cool.  I now have it running in 3 environments.  Two are Fedora 12 (I
know, I know, it's on my todo list) i686 systems with single cgroup
mounts while the other is my Fedora 15 x86_64 platform.  All are running
quite smoothly and lxc-info is working.

Daniel: Did you pick up my little patch from a couple of messages back?
That message did not including the -devel list.  Should I also send it
there?

> Thanks again for testing and looking into it!
> 
> -serge

Regards,
Mike
-- 
Michael H. Warfield (AI4NB) | (770) 985-6132 |  mhw at WittsEnd.com
   /\/\|=mhw=|\/\/          | (678) 463-0932 |  http://www.wittsend.com/mhw/
   NIC whois: MHW9          | An optimist believes we live in the best of all
 PGP Key: 0x674627FF        | possible worlds.  A pessimist is sure of it!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: This is a digitally signed message part
URL: <http://lists.linuxcontainers.org/pipermail/lxc-users/attachments/20110627/0ea87b02/attachment.pgp>