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

[Purcareata Bogdan-B43198]

> -----Original Message-----
> From: Serge Hallyn [mailto:serge.hallyn at ubuntu.com]
> Sent: Wednesday, July 03, 2013 3:46 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 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.

So lxcapi_set_cgroup_item() shouldn't return ret instead of (ret == 0)? That was my final point - I think the return value is "flipped" right at the end.

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

Thank you, and sorry for all the noise :). I just wanted to make sure I understood how the return value propagates.

> 
> -serge