netdev
[Top] [All Lists]

Re: [RFC] meta ematch

To: hadi@xxxxxxxxxx
Subject: Re: [RFC] meta ematch
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Sun, 16 Jan 2005 17:32:32 +0100
Cc: Thomas Graf <tgraf@xxxxxxx>, netdev@xxxxxxxxxxx
In-reply-to: <1105891871.1097.647.camel@xxxxxxxxxxxxxxxx>
References: <20050106194102.GW26856@xxxxxxxxxxxxxx> <1105105511.1046.77.camel@xxxxxxxxxxxxxxxx> <20050108145457.GZ26856@xxxxxxxxxxxxxx> <1105363582.1041.162.camel@xxxxxxxxxxxxxxxx> <20050110211747.GA26856@xxxxxxxxxxxxxx> <1105394738.1085.63.camel@xxxxxxxxxxxxxxxx> <20050113174111.GP26856@xxxxxxxxxxxxxx> <41E6C3E5.2020908@xxxxxxxxx> <20050113192047.GQ26856@xxxxxxxxxxxxxx> <41E71CC4.3020102@xxxxxxxxx> <20050114151407.GR26856@xxxxxxxxxxxxxx> <1105891871.1097.647.camel@xxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.3) Gecko/20041008 Debian/1.7.3-5
jamal wrote:

+static int meta_int_loadavg_2(struct sk_buff *skb, struct tcf_pkt_info *info,
+                             struct meta_value *v, struct meta_obj *dst)
+{
+       dst->value = fixed_loadavg(avenrun[2]);
+       return 0;
+}


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.
BTW, it would probably be useful to return some mnemonic instead of 0.

Returning 0 for success and negative error codes is perfectly fine as long
as you don't need any magic numbers (1, 2, ..).

+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?
Devices don't disappear during packet processing.

+static int meta_int_compare(struct meta_obj *a, struct meta_obj *b)
+{
+       /* Let gcc optimize it, the unlikely is not really based on
+        * some numbers but jump free code for missmatches seems
+        * more logical.
+        */
+       if (unlikely(a == b))
+               return 0;
+       else if (a < b)
+               return -1;
+       else
+               return 1;
+}

Would be very useful to return mnemonics for readability.
Same as for above, everyone knows what to expect from a *_compare function.
Returning stuff like CMP_LT, CMP_BT, .. is just ugly.

Regards
Patrick


<Prev in Thread] Current Thread [Next in Thread>