Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Andreas Langer <an.langer@gmx.de>]

Am Sat, 11 Sep 2010 11:26:37 +0200
schrieb Sven Eckelmann <sven.eckelmann@gmx.de>:

> Andreas Langer wrote:
> > things that changed since the last patch:
> > - add support to fragment packets on every node,
> >   if outgoing iface mtu is smaller as the packet
> > - add support to defragment packets on every node,
> >   if outgoing iface mtu is >= original size
> > - rename some functions
> 
> Is it possible to split the patch in those parts? It would make it easier to 
> read it and understand the patches later.

I think it makes no sense, it belongs together. But I can put the renaming in an
extra patch. Would that be ok ?

> 
> I haven't checked the patch in detail, but scrolled over it a little bit.
> 
> Andreas Langer wrote:
> > This patch improves the fragmentation by also fragmenting
> > packets that came in via an interface which does not need
> > fragmentation. In addition it reassembles fragmented packets
> > as soon as the interface MTU allows it.
> > 
> > Signed-off-by: Andreas Langer <an.langer at gmx.de>
> > ---
> >  batman-adv/routing.c |   64 +++++++++---------------
> >  batman-adv/unicast.c |  130
> > +++++++++++++++++++++++++++++++++----------------- batman-adv/unicast.h | 
> >  13 ++----
> >  3 files changed, 114 insertions(+), 93 deletions(-)
> > 
> > diff --git a/batman-adv/routing.c b/batman-adv/routing.c
> > index a2c64a4..e975452 100644
> > --- a/batman-adv/routing.c
> > +++ b/batman-adv/routing.c
> > @@ -1142,15 +1142,11 @@ static int route_unicast_packet(struct sk_buff
> > *skb, unsigned long flags;
> >  	struct unicast_packet *unicast_packet;
> >  	struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
> > +	int hdr_lenght = sizeof(struct unicast_packet),
> > +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_lenght;
> > 
> 
> Could you change lenght to length?

yes i can.


> 
> >  	unicast_packet = (struct unicast_packet *)skb->data;
> > 
> > -	/* packet for me */
> > -	if (is_my_mac(unicast_packet->dest)) {
> > -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> > -		return NET_RX_SUCCESS;
> > -	}
> > -
> 
> There are different parts of the patch which makes ma a little bit curious - 
> for example: why it is possible to drop that check entirely? Could that be an 
> extra patch with an explanation why that can be dropped? Is it only valid in 
> context of the new fragmentation handling? .

No it's not in context, recv_unicast_packet and recv_ucast_frag_packet check this,
so it makes no sense to call it again in route_unicast_packet. I'm not sure if it 
came from the first fragmentation patch, i put it extra.

what are the other parts which makes you curious ?

> 
> [...]
> > -static int unicast_send_frag_skb(struct sk_buff *skb, struct bat_priv
> > *bat_priv, -			  struct batman_if *batman_if, uint8_t dstaddr[],
> > -			  struct orig_node *orig_node)
> > +/* if matching fragment found return reassembled skb
> > +   otherwise buffer the fragment */
> 
> Could you change the first three spaces to a " * ". Otherwise checkpatch.pl is 
> unhappy again.
> 
> thanks,
> 	Sven

thanks, andreas