Re: [B.A.T.M.A.N.] [PATCH] batman-adv: access h_vlan_encapsulated_proto properly

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Sun, May 12, 2013 at 03:25:10PM +0800, Marek Lindner wrote:
> > On Tuesday, May 07, 2013 19:54:02 Antonio Quartulli wrote:
> > > In the gateway code in case of VLAN tagged frame the ethhdr
> > > pointer is moved forward by 4 bytes so that the offset of
> > > h_proto in struct ethhdr matches the real
> > > h_vlan_encapsulated_proto address in the skb.
> > > 
> > > This trick is correct but the code is not easy to understand
> > > and may lead to bugs in case of re-use of ethhdr for other
> > > purposes.
> > > 
> > > Use a standalone protocol variable and assign it accordingly
> > > to the header type
> > 
> > How about adding the key word "refactoring" somewhere into the subject line or 
> > the commit message ?
> 


oky


> 
> > 
> > 
> > > @@ -626,23 +626,29 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb,
> > > unsigned int *header_len) struct iphdr *iphdr;
> > >  	struct ipv6hdr *ipv6hdr;
> > >  	struct udphdr *udphdr;
> > > +	uint16_t proto;
> > > 
> > >  	/* check for ethernet header */
> > >  	if (!pskb_may_pull(skb, *header_len + ETH_HLEN))
> > >  		return false;
> > >  	ethhdr = (struct ethhdr *)skb->data;
> > > +	proto = ntohs(ethhdr->h_proto);
> > >  	*header_len += ETH_HLEN;
> > > 
> > >  	/* check for initial vlan header */
> > > -	if (ntohs(ethhdr->h_proto) == ETH_P_8021Q) {
> > > +	if (proto == ETH_P_8021Q) {
> > > +		struct vlan_ethhdr *vhdr;
> > > +
> > 
> > While we are refactoring I suggest to not ntohs() the protocol every time but 
> > the protocol constants. The compiler will replace the constants with the 
> > appropriate number during the compilation, thus saving us from a runtime 
> > operation every time we check a packet.
> > 
> 

oh right. I did not think about it. I guess I have to use __constant_htons
instead of the plain hntons.

cheers,


 -- 
 Antonio Quartulli

 ..each of us alone is worth nothing..
 Ernesto "Che" Guevara



-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]