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

[Serge E. Hallyn]

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?

Thanks again for testing and looking into it!

-serge