On Friday 13 June 2014 11:15:50 Simon Wunderlich wrote:
+/**
- batadv_is_encapsulated_claim - checks if a claim frame is
encapsulated + * @bat_priv: the bat priv with all the soft interface information + * @skb: skb to be checked
- Check if this frame is a claim frame encapsulated in at least two
layers + * of VLANs.
Maybe you mention QinQ here ? 'Two layers of VLANs' isn't that obvious (without the commit message).
OK, sure.
- Returns 1 if this is an encapsulated claim frame, 0 otherwise.
Boolean functions should return bool instead of 0 / 1.
OK.
* At this point it is known that the first two protocols* are VLAN headers, so start checking at the encapsulated protocol* of the second header.The first two protocols ? Are you trying to say that at this point we already know we are having QinQ ?
Yep. I can make that more clear.
*/- headlen = VLAN_ETH_HLEN;
- do {
vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);if (!vhdr_ptr)return 0;headlen += VLAN_HLEN;- } while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));
Is there a length check somewhere in skb_header_pointer() ? We wouldn't want to read more than we have in the skb, right ?
Yes, skb_header_pointer (or also skb_copy_bits() which is called by that) checks the length, so we don't have to do that on our own.
- if (vhdr_ptr->h_vlan_encapsulated_proto != htons(ETH_P_ARP))
return 0;- if (unlikely(!pskb_may_pull(skb, headlen + arp_hdr_len(skb->dev))))
return 0;- /* pskb_may_pull() may have modified the pointers, get ethhdr again */
- ethhdr = eth_hdr(skb);
- arphdr = (struct arphdr *)((uint8_t *)ethhdr + headlen);
- /* Check whether the ARP frame carries a valid IP information */
- if (arphdr->ar_hrd != htons(ARPHRD_ETHER))
return 0;- if (arphdr->ar_pro != htons(ETH_P_IP))
return 0;- if (arphdr->ar_hln != ETH_ALEN)
return 0;- if (arphdr->ar_pln != 4)
return 0;- hw_src = (uint8_t *)arphdr + sizeof(struct arphdr);
- hw_dst = hw_src + ETH_ALEN + 4;
- bla_dst = (struct batadv_bla_claim_dst *)hw_dst;
- bla_dst_own = &bat_priv->bla.claim_dest;
Looks like copy & paste from batadv_bla_process_claim() ? How about using a shared function ?
Yeah, it's copy and pasted. The only thing which we could put into a common function are the 4 checks, the local variables are used in a different way for the further code in each function. This didn't really look worth it so I didn't bother to refactor in that patch, if you feel otherwise let me know and i'll refactor in v2.
Thanks! Simon