Messages in this thread | | | Date | Mon, 3 Dec 2012 09:44:14 -0800 | From | Tejun Heo <> | Subject | Re: [PATCH 4/5] device_cgroup: make may_access() stronger |
| |
Hello,
On Tue, Nov 27, 2012 at 02:35:05PM -0500, Aristeu Rozanski wrote: > @@ -375,22 +376,33 @@ > continue; > if (refex->access & (~ex->access)) > continue; > - match = true; > + match = 1; > break; > } > > /* > - * In two cases we'll consider this new exception valid: > - * - the dev cgroup has its default policy to allow + exception list: > - * the new exception should *not* match any of the exceptions > - * (behavior == DEVCG_DEFAULT_ALLOW, !match) > - * - the dev cgroup has its default policy to deny + exception list: > - * the new exception *should* match the exceptions > - * (behavior == DEVCG_DEFAULT_DENY, match) > + * The only three possibilities are: > + * devcg->behavior == ALLOW, rule behavior == ALLOW > + * devcg->behavior == ALLOW, rule behavior == DENY > + * devcg->behavior == DENY, rule behavior == DENY > + * the remaining > + * devcg->behavior == DENY, rule behavior == ALLOW > + * won't be possible by hierarchy > + * > + * Since we want to simplify the code, here're the possibilites to > + * make easier to understand: > + * > + * devcg behavior rule behavior match result > + * allow allow 1 0 > + * allow allow 0 1 > + * allow deny 1 0 > + * allow deny 0 1 > + * deny deny 1 1 > + * deny deny 0 0 > */ > - if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match) > - return 1; > - return 0; > + if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW) > + return !match; > + return match;
I kinda dislike this. This isn't a performanc critical path where we must try our best to shave off a few condition checks. There's no reason to encode the test like this. Please just spell the conditions out in code rather than trying to build a magic series of equality tests which somehow ends up spewing out the correct results.
Thanks.
-- tejun
| |