On Wednesday 09 October 2013 15:05:37 Simon Wunderlich wrote:
> --- a/routing.c
> +++ b/routing.c
> @@ -407,16 +407,104 @@ static int batadv_check_unicast_packet(struct
> batadv_priv *bat_priv, struct batadv_neigh_node *
> batadv_find_router(struct batadv_priv *bat_priv,
> struct batadv_orig_node *orig_node,
> - const struct batadv_hard_iface *recv_if)
> + struct batadv_hard_iface *recv_if)
> {
> - struct batadv_neigh_node *router;
> + struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;
> + struct batadv_neigh_node *first_candidate_router = NULL;
> + struct batadv_neigh_node *next_candidate_router;
> + struct batadv_neigh_node *router, *cand_router;
> + struct batadv_orig_node_ifinfo *cand, *first_candidate = NULL;
> + struct batadv_orig_node_ifinfo *next_candidate = NULL;
> + bool last_candidate_found = false;
>
> if (!orig_node)
> return NULL;
>
> router = batadv_orig_node_get_router(orig_node, recv_if);
>
> - /* TODO: fill this later with new bonding mechanism */
> + /* only consider bonding for recv_if == NULL (first hop) and if
> + * activated.
> + */
> + if (recv_if || !atomic_read(&bat_priv->bonding) || !router)
> + return router;
> +
> + /* bonding: loop through the list of possible routers found
> + * for the various outgoing interfaces and find a candidate after
> + * the last chosen bonding candidate (next_candidate). If no such
> + * router is found, use the first candidate found (the last chosen
> + * bonding candidate might have been the last one in the list).
> + * If this can't be found either, return the previously choosen
> + * router - obviously there are no other candidates.
> + */
> + rcu_read_lock();
> + hlist_for_each_entry_rcu(cand, &orig_node->ifinfo_list, list) {
> + cand_router = rcu_dereference(cand->router);
> + if (!cand_router)
> + continue;
> +
> + if (!atomic_inc_not_zero(&cand_router->refcount))
> + continue;
> +
> + /* alternative candidate should be good enough to be
> + * considered
> + */
> + if (!bao->bat_neigh_is_equiv_or_better(cand_router,
> + cand->if_outgoing,
> + router, recv_if))
> + goto next;
> +
> + /* don't use the same router twice */
> + if (orig_node->last_bonding_candidate &&
> + (orig_node->last_bonding_candidate->router ==
> + cand_router))
> + goto next;
Again, this if-statement won't pass.
> + /* check if already passed the next candidate ... this function
> + * should the next candidate AFTER the last used bonding
> + * candidate.
> + */
> + if (!orig_node->last_bonding_candidate ||
> + last_candidate_found) {
> + next_candidate = cand;
> + next_candidate_router = cand_router;
> + break;
> + }
The comment above would benefit from additional love. :)
> + if (next_candidate) {
> + /* found a possible candidate after the last chosen bonding
> + * candidate, return it.
> + */
> + batadv_neigh_node_free_ref(router);
> + if (first_candidate)
> + batadv_neigh_node_free_ref(first_candidate_router);
> + router = next_candidate_router;
> + orig_node->last_bonding_candidate = next_candidate;
> + } else if (first_candidate) {
> + /* found no possible candidate after the last candidate, return
> + * the first candidate if available, or the already selected
> + * router otherwise.
> + */
> + batadv_neigh_node_free_ref(router);
> + router = first_candidate_router;
> + orig_node->last_bonding_candidate = first_candidate;
> + } else {
> + orig_node->last_bonding_candidate = NULL;
> + }
It is not safe to hold a pointer to first_candidate or next_candidate without
having a refcounter protecting it.
Cheers,
Marek
signature.asc
Description: This is a digitally signed message part.
