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

[Michael H. Warfield]

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
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.

> thanks,
> -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/20110626/5188405c/attachment.pgp>