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

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

Hi Andrew, answers see below

> > +struct frag_packet_list_entry *search_frag_packet(struct list_head *head,
> > +	struct unicast_frag_packet *up) {
> > +
> > +	struct frag_packet_list_entry *tfp;
> > +	struct unicast_frag_packet *tmp_up = NULL;
> > +
> > +	list_for_each_entry(tfp, head, list) {
> > +
> > +		if (!tfp->skb)
> > +			continue;
> > +
> > +		if (tfp->seqno == ntohs(up->seqno))
> > +			goto mov_tail;
> 
> Please could you explain this last bit? If we have received a
> retransmit we move it to the tail of the list?

If we receive a packet with the same seqno we move the list entry to the tail then return null.

	tmp_frag_entry =
			search_frag_packet(&orig_node->frag_list,
			unicast_packet);

	if (!tmp_frag_entry) {
		create_frag_entry(&orig_node->frag_list, skb);


In routing.c it creates now a new entry, create_frag_entry takes the last list entry. So the old
packet with the same seqno will be now overwritten. I hope it is clear, do you see any problems with
this ?



> > +
> > +		tmp_up = (struct unicast_frag_packet *) tfp->skb->data;
> > +
> > +		if (up->flags & UNI_FRAG_HEAD) {
> > +
> > +			if (tfp->seqno == ntohs(up->seqno)+1) {
> > +				if (tmp_up->flags & UNI_FRAG_HEAD)
> > +					goto mov_tail;
> > +				else
> > +					goto ret_tfp;
> > +			}
> > +		} else {
> > +
> > +			if (tfp->seqno == ntohs(up->seqno)-1) {
> > +				if (tmp_up->flags & UNI_FRAG_HEAD)
> > +					goto ret_tfp;
> > +				else
> > +					goto mov_tail;
> > +			}
> > +		}
> 
> How about simplifying this loop a little. Not tested, probably broken,
> but i hope expresses the idea:
> 
> 	if (up->flags & UNI_FRAG_HEAD) {
> 	      search_seqno = ntohs(up->seqno)+1
> 	} else {
> 	      search_seqno = ntohs(up->seqno)-1
> 	}
>     
> 	list_for_each_entry(tfp, head, list) {
> 		if (!tfp->skb)
> 			continue;
> 		
> 		/* Found the other fragment? */
> 		if (tfp->seqno == search_seqno)
> 		    /* Head and a tail? */
> 		    if ((tmp_up->flags & UNI_FRAG_HEAD) != 
>  		        (tfp->flags & UNI_FRAG_HEAD)
> 		       return tfp;
> 		    } else {
> 		      /* Two heads or tails. Move to tail of list so it will 
> 		         get recycled */
> 		      list_move_tail(&tfp->list, head);
> 		      return NULL;
>                     }
>         }
> 	return NULL;
> 
> Moving the ntohs() out of the loop should save a few cycles and it is
> hopefully a little bit more readable.

Thanks for the hint, it is more comprehensible.

> 
> > +	}
> > +	goto ret_null;
> > +
> > +ret_tfp:
> > +	return tfp;
> > +mov_tail:
> > +	list_move_tail(&tfp->list, head);
> > +ret_null:
> > +	return NULL;
> 
> Assuming you don't like my re-write, could we remove ret_tfp and
> ret_null. I don't think they aid the understandability of the
> code. They are just return statements.

I like your re-write.

> > +}
> > +
> > +void frag_list_free(struct list_head *head)
> > +{
> > +
> > +	struct frag_packet_list_entry *pf, *tmp_pf;
> > +
> > +	if (!list_empty(head)) {
> > +
> > +		list_for_each_entry_safe(pf, tmp_pf, head, list) {
> > +			if (pf->skb)
> > +				kfree_skb(pf->skb);
> 
> kfree_skb() is happy to take a NULL pointer. So there is no need to do
> the comparison here. It will get repeated inside kfree_skb(). This
> happens in a few places.

Thanks for the hint.
 
> > +			list_del(&pf->list);
> > +			kfree(pf);
> > +		}
> > +	}
> > +	return;
> 
> 
> > +int recv_unicast_packet(struct sk_buff *skb, struct batman_if *recv_if)
> > +{
> > +	struct unicast_packet *unicast_packet;
> > +	struct ethhdr *ethhdr;
> > +	int hdr_size = sizeof(struct unicast_packet);
> > +
> > +	/* drop packet if it has not necessary minimum size */
> > +	if (skb_headlen(skb) < hdr_size)
> > +		return NET_RX_DROP;
> > +
> > +	ethhdr = (struct ethhdr *) skb_mac_header(skb);
> > +
> > +	/* packet with unicast indication but broadcast recipient */
> > +	if (is_bcast(ethhdr->h_dest))
> > +		return NET_RX_DROP;
> > +
> > +	/* packet with broadcast sender address */
> > +	if (is_bcast(ethhdr->h_source))
> > +		return NET_RX_DROP;
> > +
> > +	/* not for me */
> > +	if (!is_my_mac(ethhdr->h_dest))
> > +		return NET_RX_DROP;
> > +
> > +	unicast_packet = (struct unicast_packet *) skb->data;
> > +
> > +	/* packet for me */
> > +	if (is_my_mac(unicast_packet->dest)) {
> > +		interface_rx(skb, hdr_size);
> > +		return NET_RX_SUCCESS;
> > +	}
> > +
> > +	return route_unicast_packet(skb, recv_if, hdr_size);
> > +}
> > +
> > +int recv_ucast_frag_packet(struct sk_buff *skb, struct batman_if *recv_if)
> > +{
> > +	struct unicast_frag_packet *unicast_packet;
> > +	struct orig_node *orig_node;
> > +	struct ethhdr *ethhdr;
> > +	struct frag_packet_list_entry *tmp_frag_entry;
> > +	int hdr_size = sizeof(struct unicast_frag_packet);
> > +	unsigned long flags;
> > +
> > +	/* drop packet if it has not necessary minimum size */
> > +	if (skb_headlen(skb) < hdr_size)
> > +		return NET_RX_DROP;
> > +
> > +	ethhdr = (struct ethhdr *) skb_mac_header(skb);
> > +
> > +	/* packet with unicast indication but broadcast recipient */
> > +	if (is_bcast(ethhdr->h_dest))
> > +		return NET_RX_DROP;
> > +
> > +	/* packet with broadcast sender address */
> > +	if (is_bcast(ethhdr->h_source))
> > +		return NET_RX_DROP;
> > +
> > +	/* not for me */
> > +	if (!is_my_mac(ethhdr->h_dest))
> > +		return NET_RX_DROP;
> > +
> 
> There is some code here which is identical to
> recv_unicast_packet(). Could it be refactored into a helper function?

Yes it could, i already thought about it.
 
> 
>        Andrew

Thanks again, i will send a new patch.

regards,
Andreas