On Wed, May 04, 2011 at 01:22:34PM +0200, Andrew Lunn wrote:
+struct roam_adv_packet {
- uint8_t packet_type;
- uint8_t version;
- uint8_t dst[6];
- uint8_t ttl;
- uint8_t src[6];
- uint8_t client[6];
+} __packed;
Maybe put ttl at the end, to help with alignment?
As I did for the tt_query packet, the initial four fields are the same as the unicast_packet so that I can exploit route_unicast_packet() instead of writing routing function.
Is that a major issue?
- tt_global_add(bat_priv, orig_node, roam_adv_packet->client,
atomic_read(&orig_node->last_ttvn) + 1, true);- /* Roaming phase starts: I have a new information but the ttvn has been
* incremented yet. This flag will make me check all the incoming* packets for the correct destination. */The grammar in that comment could be better:
/* Roaming phase starts: I have new information but the ttvn has not * been incremented yet. This flag will make me check all the incoming * packets for the correct destination. */
Thanks and sorry for my poor grammar :)
- skb = dev_alloc_skb(sizeof(struct roam_adv_packet) + ETH_HLEN);
- if (!skb)
goto free_skb;If the allocation fails, go free it ?
It's a matter of label. I'll correct it
- skb_reserve(skb, ETH_HLEN);
- roam_adv_packet = (struct roam_adv_packet *)skb_put(skb,
sizeof(struct roam_adv_packet));- roam_adv_packet->packet_type = BAT_ROAM_ADV;
- roam_adv_packet->version = COMPAT_VERSION;
- roam_adv_packet->ttl = TTL;
- memcpy(roam_adv_packet->src,
bat_priv->primary_if->net_dev->dev_addr, ETH_ALEN);- memcpy(roam_adv_packet->dst, orig_node->orig, ETH_ALEN);
- memcpy(roam_adv_packet->client, client, ETH_ALEN);
- neigh_node = find_router(bat_priv, orig_node, NULL);
- if (!neigh_node)
goto free_skb;- if (neigh_node->if_incoming->if_status != IF_ACTIVE)
goto free_neigh;- bat_dbg(DBG_ROUTES, bat_priv,
"Sending ROAMING_ADV to %pM (client %pM) via %pM\n",orig_node->orig, client, neigh_node->addr);- send_skb_packet(skb, neigh_node->if_incoming, neigh_node->addr);
- ret = 0;
+free_neigh:
- if (neigh_node)
neigh_node_free_ref(neigh_node);+free_skb:
- if (ret)
kfree_skb(skb);- return;
+unlock:
- spin_unlock_bh(&bat_priv->tt_roam_list_lock);
}
All these different goto's makes me think of BASIC. How about breaking this function up into a number of functions.
- find an existing tt_roam_node
- Create a new tt_roam_node
- Allocate and fill in the roam_adv_packet.
- Find the neigh_node and send the packet.
You then end up with something like
void send_roam_adv(struct bat_priv *bat_priv, uint8_t *client, struct orig_node *orig_node) { spin_lock_bh(&bat_priv->tt_roam_list_lock);
tt_roam_node = find_tt_roam_node(client);
if (!tt_roam_node) { tt_roam_node = new_tt_roam_node(client, bat_priv); } spin_unlock_bh(&bat_priv->tt_roam_list_lock);
if (tt_roam_node) roam_pkt = build_roam_pkt(bat_priv, orig_node, client); if (roan_pkt) send_roam_pkt(roam_pkt, orig_node, client); }
No goto's and easier to understand. It also makes it clear that tt_roam_node is not actually used while sending the packet, so maybe it does not belong inside send_roam_adv()?
Ok..I got the point. Maybe I will not be so drastic but I will follow your suggestion
- bool tt_poss_change; /* this flag is needed to detect an ongoing
* roaming event. If it is true, it means that* in the last OGM interval I sent a Roaming_adv,* so I have to check every packet going to it* whether the destination is still a client of* its or not, it will be reset as soon as I'll* receive a new TTVN from it */Too many it/its. I have a hard time understanding what it is.
You are definitely right. I'll rewrite the comment.
So, mostly comments about the comments and style issues.
Andrew
Thank you very much Andrew!
Regards,