[lxc-devel] [PATCH] tests/startone: wrong set_cgroup_item check

[Serge Hallyn]

Quoting Purcareata Bogdan-B43198 (B43198 at freescale.com):
> > -----Original Message-----
> > From: Serge Hallyn [mailto:serge.hallyn at ubuntu.com]
> > Sent: Tuesday, July 02, 2013 5:27 PM
> > To: Purcareata Bogdan-B43198
> > Cc: lxc-devel at lists.sourceforge.net
> > Subject: Re: [lxc-devel] [PATCH] tests/startone: wrong set_cgroup_item check
> > 
> > Quoting Bogdan Purcareata (bogdan.purcareata at freescale.com):
> > > Minor typo.
> > >
> > > Signed-off-by: Bogdan Purcareata <bogdan.purcareata at freescale.com>
> > > ---
> > >  src/tests/startone.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/tests/startone.c b/src/tests/startone.c
> > > index d781e75..2c1f39b 100644
> > > --- a/src/tests/startone.c
> > > +++ b/src/tests/startone.c
> > > @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
> > >
> > >  	sprintf(buf, "FROZEN");
> > >  	b = c->set_cgroup_item(c, "freezer.state", buf);
> > > -	if (!b) {
> > > +	if (b) {
> > 
> > Maybe I'm not thinking right, but why do you think setting freezer.state
> > should fail?
> 
> After looking at the lxcapi_set_cgroup_item implementation I'm a bit confused:
> 
> src/lxc/lxccontainer.c:
> 1383 static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys, const char *value)
> 1384 {
> 1385   int ret;
> 1386
> [ ... ] initial checks
> 1395
> 1396   ret = lxc_cgroup_set(c->name, subsys, value, c->config_path) == 0;
> 1397
> 1398   container_disk_unlock(c);
> 1399   return ret == 0;
> 1400 }
> 
> lxc_cgroup_set is basically a call to do_cgroup_set, which returns 0 (false) on success (both functions are in src/lxc/cgroup.c).
> 
> So calls return this:
> 
> do_cgroup_set -> 0 (false) on success

don't say "0 (false)".  It returns < 0 on error, 0 on success.

> lxc_cgroup_set -> false on success

returns 0 on success, < 0 on error.

> ret (in lxcapi_set_cgroup_item) = (false == false) = true on success

if lxc_cgroup_set() == 0, then lxc_cgroup_set() returned success.
So return (lxc_cgroup_set() == 0) returns true on success.

> lxcapi_set_cgroup_item -> (true == false) = false on success
> 
> That's why I thought the check should have been made for (b) instead of (!b) - since 1 (true) is the error case here.
> Now I'm thinking maybe there should be a bit of refactoring in the code?

But it's natural to anyone who's done kernel coding - and this is why
it's clearly spelled out in the function preambles.

The lxcapi (embodied in lxccontainer.c) is the bridge between various
userspace codes and the more kernel-like lxc implementation, which is
why it can be a bit tricky if you're not used to it.  But that's why
I went to the trouble of using bool rather than ints.  If you have an
int return value, hopefully 0 will be success.  If bool, then true will
be success.

-serge