[Olsr-dev] [PATCH][BUILD] check_neighbor_link is wrong
Ferry Huberts
(spam-protected)
Tue Dec 11 13:15:01 CET 2012
On 11/12/12 13:05, Giovanni Di Stasi wrote:
> Il 11/12/2012 11:02, Henning Rogge ha scritto:
>> Comments inline.
>>
>> On 12/10/2012 08:34 PM, Giovanni Di Stasi wrote:
>>>> I see only two cases where this "mix up links between nodes" will happen:
>>>> a) a node use the same IPs for multiple interfaces
>>>> b) a node can see multiple interfaces of a different node
>>>>
>>>> The patch solves parts of case b) as long as no VLANs are involved. can
>>>> we use the interface index instead of the local IP?
>>> The intent of the patch was to fix case b) that may easily arise if you change channels on the interfaces.
>>> I didn't consider the VLAN case, but from our point of view using the interface index is perfectly fine.
>>> diff --git a/src/hna_set.c b/src/hna_set.c
>>> index df8a82d..711e85a 100644
>>> --- a/src/hna_set.c
>>> +++ b/src/hna_set.c
>>> @@ -362,7 +362,7 @@ olsr_print_hna_set(void)
>>> */
>>>
>>> bool
>>> -olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
>>> +olsr_input_hna(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
>>> {
>>>
>>> uint8_t olsr_msgtype;
>>> @@ -425,7 +425,7 @@ olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((un
>>> * is not in the symmetric 1-hop neighborhood of this node, the
>>> * message MUST be discarded.
>>> */
>>> - if (check_neighbor_link(from_addr) != SYM_LINK) {
>>> + if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
>>> OLSR_PRINTF(2, "Received HNA from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
>>> return false;
>>> }
>>> diff --git a/src/link_set.c b/src/link_set.c
>>> index aa6a9ea..cba1855 100644
>>> --- a/src/link_set.c
>>> +++ b/src/link_set.c
>>> @@ -184,7 +184,7 @@ get_neighbor_status(const union olsr_ip_addr *address)
>>> if (!(main_addr = mid_lookup_main_addr(address)))
>>> main_addr = address;
>>>
>>> - /* Loop trough local interfaces to check all possebilities */
>>> + /* Loop trough local interfaces to check all possbilities */
>>> for (ifs = ifnet; ifs != NULL; ifs = ifs->int_next) {
>>> struct mid_address *aliases;
>>> struct link_entry *lnk = lookup_link_entry(main_addr, NULL, ifs);
>>> @@ -216,10 +216,14 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>>> const union olsr_ip_addr *main_addr;
>>> struct link_entry *walker, *good_link, *backup_link;
>>> struct interface *tmp_if;
>>> - int curr_metric = MAX_IF_METRIC;
>>> - olsr_linkcost curr_lcost = LINK_COST_BROKEN;
>>> + int curr_metric_good = MAX_IF_METRIC;
>>> + int curr_metric_backup = MAX_IF_METRIC;
>>> + olsr_linkcost curr_lcost_good = LINK_COST_BROKEN;
>>> + olsr_linkcost curr_lcost_backup = LINK_COST_BROKEN;
>>> +
>>> olsr_linkcost tmp_lc;
>>>
>>> +
>>> /* main address lookup */
>>> main_addr = mid_lookup_main_addr(remote);
>>>
>>> @@ -234,13 +238,12 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>>>
>>> /* loop through all links that we have */
>>> OLSR_FOR_ALL_LINK_ENTRIES(walker) {
>>> -
>>> +
>>> /* if this is not a link to the neighour in question, skip */
>>> if (!ipequal(&walker->neighbor->neighbor_main_addr, main_addr))
>>> continue;
>>> -
>>> +
>>> if (olsr_cnf->lq_level == 0) {
>>> -
>>> /*
>>> * handle the non-LQ, RFC-compliant case
>>> */
>>> @@ -257,20 +260,22 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>>>
>>> /*
>>> * is this interface better than anything we had before ?
>>> - * use the requested remote interface address as a tie-breaker
>>> + * use the link interface index as a tie-breaker
>>> */
>>> - if ((tmp_if->int_metric < curr_metric) || ((tmp_if->int_metric == curr_metric) && ipequal(&walker->local_iface_addr, remote))) {
>>> -
>>> - /* memorize the interface's metric */
>>> - curr_metric = tmp_if->int_metric;
>>> -
>>> - /* prefer symmetric links over asymmetric links */
>>> - if (lookup_link_status(walker) == SYM_LINK) {
>>> - good_link = walker;
>>> - } else {
>>> - backup_link = walker;
>>> - }
>>> + if (lookup_link_status(walker) == SYM_LINK) {
>>> + if (tmp_if->int_metric < curr_metric_good ||
>>> + (tmp_if->int_metric == curr_metric_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
>>> + good_link = walker;
>>> + curr_metric_good = tmp_if->int_metric;
>>> + }
>>> + } else {
>>> + if (tmp_if->int_metric < curr_metric_backup ||
>>> + (tmp_if->int_metric == curr_metric_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
>>> + backup_link = walker;
>>> + curr_metric_backup = tmp_if->int_metric;
>>> + }
>>> }
>>> +
>>> } else {
>>>
>>> /*
>>> @@ -280,22 +285,31 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>>> /* get the link cost */
>>> tmp_lc = walker->linkcost;
>>>
>>> + /* find the interface for the link */
>>> + tmp_if = walker->if_name ? if_ifwithname(walker->if_name) : if_ifwithaddr(&walker->local_iface_addr);
>>> +
>>> + if (!tmp_if) {
>>> + continue;
>>> + }
>>> +
>>> /*
>>> * is this link better than anything we had before ?
>>> - * use the requested remote interface address as a tie-breaker.
>>> + * use the link interface index as a tie-breaker
>>> */
>>> - if ((tmp_lc < curr_lcost) || ((tmp_lc == curr_lcost) && ipequal(&walker->local_iface_addr, remote))) {
>>> -
>>> - /* memorize the link quality */
>>> - curr_lcost = tmp_lc;
>>> -
>>> - /* prefer symmetric links over asymmetric links */
>>> - if (lookup_link_status(walker) == SYM_LINK) {
>>> - good_link = walker;
>>> - } else {
>>> - backup_link = walker;
>>> - }
>>> + if (lookup_link_status(walker) == SYM_LINK) {
>>> + if (tmp_lc < curr_lcost_good ||
>>> + (tmp_lc == curr_lcost_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
>>> + good_link = walker;
>>> + curr_lcost_good=tmp_lc;
>>> + }
>>> + } else {
>>> + if (tmp_lc < curr_lcost_backup ||
>>> + (tmp_lc == curr_lcost_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
>>> + backup_link = walker;
>>> + curr_lcost_backup=tmp_lc;
>>> + }
>>> }
>>> +
>>> }
>>> }
>> I am a little bit scared to look up who use this function if it can
>> return an asymmetric link. Asymmetric links are useless for outbound
>> routing.
>>
> Hi Henning,
>
> the backup link is given back only if no symmetric link has been found, which is the same that is done now without the patch.
> Do you think we should return null in case there is an asymmetric link but no a symmetric link?
> If things stay as they are, it could be made clear that it is caller responsibility to check what kind of link is the best link given by the get_best_link function.
>
In my experience, asymmetric links are evil and cause all kinds of
unexpected, hard to diagnose, problems.
I would _very much_ like to avoid them.
--
Ferry Huberts
More information about the Olsr-dev
mailing list