Re: [B.A.T.M.A.N.] [PATCHv2 3/4] batman-adv: improved roaming mechanism

Andrew Lunn
2011-05-04T11:22:34Z

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

> + 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. */

+void send_roam_adv(struct bat_priv *bat_priv, uint8_t *client,
> + struct orig_node *orig_node)
> +{
> + struct neigh_node *neigh_node;
> + struct sk_buff *skb;
> + struct roam_adv_packet *roam_adv_packet;
> + struct tt_roam_node *tt_roam_node;
> + bool found = false;
> + int ret = 1;
> +
> + spin_lock_bh(&bat_priv->tt_roam_list_lock);
> + /* The new tt_req will be issued only if I'm not waiting for a
> + * reply from the same orig_node yet */
> + list_for_each_entry(tt_roam_node, &bat_priv->tt_roam_list, list) {
> + if (!compare_eth(tt_roam_node->addr, client))
> + continue;
> +
> + if (is_out_of_time(tt_roam_node->first_time,
> + ROAMING_MAX_TIME * 1000))
> + continue;
> +
> + if (!atomic_dec_not_zero(&tt_roam_node->counter))
> + /* Sorry, you roamed too many times! */
> + goto unlock;
> +
> + found = true;
> + break;
> + }
> +
> + if (!found) {
> + tt_roam_node = kmalloc(sizeof(struct tt_roam_node), GFP_ATOMIC);
> + if (!tt_roam_node)
> + goto unlock;
> +
> + tt_roam_node->first_time = jiffies;
> + atomic_set(&tt_roam_node->counter, ROAMING_MAX_COUNT - 1);
> + memcpy(tt_roam_node->addr, client, ETH_ALEN);
> +
> + list_add(&tt_roam_node->list, &bat_priv->tt_roam_list);
> + }
> + spin_unlock_bh(&bat_priv->tt_roam_list_lock);
> +
> + skb = dev_alloc_skb(sizeof(struct roam_adv_packet) + ETH_HLEN);
> + if (!skb)
> + goto free_skb;

If the allocation fails, go free 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.

1) find an existing tt_roam_node
2) Create a new tt_roam_node
3) Allocate and fill in the roam_adv_packet.
4) 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()?

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

So, mostly comments about the comments and style issues.

Andrew