Hello Oliver;
On Tue, 2012-06-26 at 22:07 +0200, Oliver Hartkopp wrote:
> I found some time for a review. See details inline ...
>
I agree with quite everything except for following...
> > + match = true;
>
>
> match = 1;
>
egrep -r "= true;" ./linux-source | wc -l
returns 6770 -- why don't you like "= true"?
> > + break;
> > + }
> > + }
> > + } else { /* SFF */
> > + can_id &= CAN_SFF_MASK;
> > + match = test_bit(can_id, cm->match_sff);
> > + }
> > +
>
>
> return match;
>
match() function must return 1 or 0, however (from my experience)
test_bit() returns 0 and non-0 (strictly speaking, in my case, 0 and
-1).
> > + &conf[i],
> > + sizeof(struct can_filter));
> > +
> > + cm->eff_rules_count++;
> > + } else {
> > + continue;
> > + }
>
>
> omit { } around continue
>
http://lxr.linux.no/#linux+v3.4.4/Documentation/CodingStyle#L169
> > + }
> > +
> > + /* Process SFF frame rules */
> > + for (i = 0; i < cm->rules_count; i++) {
> > + if ((conf[i].can_id & CAN_EFF_FLAG) &&
> > + (conf[i].can_mask & CAN_EFF_FLAG)) {
>
>
> What if CAN_EFF_FLAG is set in can_id but not in can_mask ?
>
There were small misunderstanding from my side -- this will be rewritten
in the way that EFF_FLAG in mask will determine if we care about the
value of EFF_FLAG in an identifier -- i.e. when EFF_FLAG is set in the
mask, the rule will be added as SFF or EFF only depending on EFF_FLAG
value in the identifier. If EFF_FLAG is 0 in the mask, the rule will be
added as both SFF and EFF.
Regards;
Rostislav
--
To unsubscribe from this list: send the line "unsubscribe lartc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
