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

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 2515 bytes --]

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 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?

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

[...]
> -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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]