On Sun, 2005-01-16 at 11:32, Thomas Graf wrote:
> * jamal <1105891871.1097.647.camel@xxxxxxxxxxxxxxxx> 2005-01-16 11:11
> > On Fri, 2005-01-14 at 10:14, Thomas Graf wrote:
> >
> > > diff -Nru linux-2.6.10-bk14.orig/include/linux/tc_ematch/tc_em_meta.h
> > > linux-2.6.10-bk14/include/linux/tc_ematch/tc_em_meta.h
> > [..]
> > > +struct tcf_meta_val
> > > +{
> > > + __u16 kind;
> > > + __u8 shift;
> > > + __u8 op;
> > > +};
> > > +
> > > +#define TCF_META_TYPE_MASK (0xf << 12)
> > > +#define TCF_META_TYPE(kind) (((kind) & TCF_META_TYPE_MASK) >> 12)
> > > +#define TCF_META_ID_MASK 0x7ff
> > > +#define TCF_META_ID(kind) ((kind) & TCF_META_ID_MASK)
> >
> > Does it smell like there may be endianess issues? Probably not.
>
> Not really as long as iproute2 uses the same byte ordering. It has the
> same issues as all other rtnetlink users.
wont harm to do a quick test if you have hardware. pedit for example
still has some occasional issues some issues with big endian which i
havent had time to chase.
> > > + TCF_META_ID_DEV,
> >
> > Since filters are attached to devices - is TCF_META_ID_DEV of any value?
>
> Yes, to compare against realdev and indev.
makes sense
> > > +struct meta_value
> > > +{
> > > + struct tcf_meta_val hdr;
> > > + unsigned long val;
> > > + unsigned int len;
> >
> > Those last two look like meta_obj you defined above
>
> Yes, they once were but the code is more readable this way
> because one cannot mistake meta_obj (temporary data) with
> the data/len in meta_value (persistent).
>
fine
> > > + return (v + (FIXED_1/200)) >> FSHIFT;
> >
> > 200 has some magic connotation to it - a define somewhere perhaps?
>
> I coped this from the code for procfs ;->
know why they have that number? It must have some significance - or
maybe someone just stuck their hand in the air and measured 200? ;->
> > Theres a lot of parameters not used at all in these calls .get calls. So
> > far i have seen dst->value and some of the skb fields used. I apologize,
> > I normally dont pick on these things - so if you have future plans for
> > why you are passing those, keep them and ignore the comment.
>
> I do have plans but for more complicated meta matches and they will
> take some time and will get pushed in a second iteration.
>
np
> > > +static inline int var_dev(struct net_device *dev, struct meta_obj *dst)
> > > +{
> > > + if (unlikely(dev == NULL))
> > > + return -1;
> > > +
> > > + dst->value = (unsigned long) dev->name;
> > > + dst->len = strlen(dev->name);
> >
> > So if device dissapears ... what happens to the pointer?
>
> Not possible, the pointer is only legal to use while classifying,
> dst is on the stack of meta_match and the device cannot
> disappear while classyfing.
makes sense then
> > > +static void meta_var_apply_extras(struct meta_value *v,
> > > + struct meta_obj *dst)
> > > +{
> > > + int shift = v->hdr.shift;
> > > +
> > > + if (shift && shift < dst->len)
> > > + dst->len -= shift;
> > > +}
> >
> > whats the purpose to the extras?
>
> "eth0" shift 1 gets "eth" so we can emulate eth%. It will
> also get useful for ipv6 routing bits to implement the
> prefix.
>
makes sense - ala ppp*
> > > +static int em_meta_match(struct sk_buff *skb, struct tcf_ematch *m,
> > > + struct tcf_pkt_info *info)
> > > +{
> > > + int r;
> > > + struct meta_match *meta = (struct meta_match *) m->data;
> > > + struct meta_obj l_value, r_value;
> > > +
> > > + if (meta_get(skb, info, &meta->lvalue, &l_value) < 0 ||
> > > + meta_get(skb, info, &meta->rvalue, &r_value) < 0)
> > > + return 0;
> >
> > This is one part that confused me in my earlier email
>
> It transforms the meta data info in (l|r)value into 2 temporary
> meta objects for comparing. mgiht help if i find a better
> name.
>
> > Ok, I have to admit I am not a friend of too-friendly, which is one of
> > the faults IMO with netfilter; however, this would have
> > joenetfilterfireman sweat a little too profusely.
> > I think you could add a new metafield in 31 seconds. I could probably do
> > it in 95 seconds. Would be ideal to get average
> > janeorjoenetfilterfireman to do it in 30 minutes - I dont think you are
> > there.
>
> ideas? a step-by-step guide in Documentation/? ;->
I am not sure - probably more inlined commenting as you normally do.
cheers,
jamal
|