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;elsegoto ret_tfp;}} else {if (tfp->seqno == ntohs(up->seqno)-1) {if (tmp_up->flags & UNI_FRAG_HEAD)goto ret_tfp;elsegoto 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