Re: [RFC] meta ematch

jamal wrote:

quoted

+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, ..).

quoted

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

quoted

+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