[Olsr-dev] [PATCH][BUILD] check_neighbor_link is wrong
Giovanni Di Stasi
(spam-protected)
Tue Dec 11 13:05:41 CET 2012
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.
>> OLSR_FOR_ALL_LINK_ENTRIES_END(walker);
>> @@ -327,7 +341,7 @@ set_loss_link_multiplier(struct link_entry *entry)
>> }
>> }
>> assert(cfg_inter);
>> -
>> +
>> /* create a null address for comparison */
>> memset(&null_addr, 0, sizeof(union olsr_ip_addr));
>>
>> @@ -627,12 +641,12 @@ add_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *remote
>> * @return 1 of the link is symmertic 0 if not
>> */
>> int
>> -check_neighbor_link(const union olsr_ip_addr *int_addr)
>> +check_neighbor_link(const union olsr_ip_addr * local, const union olsr_ip_addr *remote)
>> {
>> struct link_entry *link;
>>
>> OLSR_FOR_ALL_LINK_ENTRIES(link) {
>> - if (ipequal(int_addr, &link->neighbor_iface_addr)) {
>> + if (ipequal(remote, &link->neighbor_iface_addr) && ipequal(local, &link->local_iface_addr)) {
> I was talking about using the interface index (instead of the local IP)
> here to detect the "right" link.
>
Ok then, I will modify the patch to use the interface index instead.
Best regards,
Giovanni
More information about the Olsr-dev
mailing list