[Olsr-dev] Bug in function olsr_tc_update_edge ?

[Hannes Gredler]

hi eric,

your observation is correct, now asking what is the right fix ?

we are getting in the waters of system design here.

the more interesting question to ask is why does a node
emit a less than 10 percent change and flood the TC throughout
the network in spite that everyone will not trigger a SPF
calculation ?

IMO the right thing to do, sender should hold-back the information
locally and once the change is significant enough, flood it
throughout the network.

/hannes

Erik Tromp wrote:
> Hi all,
> 
> I think there may be a bug in the 0.5.5 (and also 0.5.4) version of OLSRd.
> 
> The function 'olsr_tc_update_edge' in the file 'tc_set.c' calls the function
> 'olsr_etx_significant_change' to determine whether 'the etx change is
> meaningful enough in order to trigger a SPF calculation'. The function
> 'olsr_etx_significant_change' returns true if the change is 10% or more.
> 
> To do this, it compares the current 'tc_edge->link_quality' with the value
> of 'neigh_link_quality' which is a value as received in the most recent TC
> message. If the difference is large enough, the boolean 'edge_change' is set
> to 1 to trigger the SPF calculation.
> 
> However, after that, the 'neigh_link_quality' is always copied into
> 'tc_edge->link_quality'.
> 
> What happens if the changes are very small, say 5% every time? None of the
> changes will ever trigger the SPF calculation, because all the changes are <
> 10%.
> 
> In the pre-0.5.4 era [the good old times ;-) ], the relevant value was only
> saved (in a 'saved_...' field) when the calculation was actually done. In
> this way, incremental changes were detected with respect to the values as
> used for the last calculation.
> 
> In the 0.5.3 version of tc_set.h:
> 
> struct topo_dst
> {
>   union olsr_ip_addr T_dest_addr;
>   clock_t            T_time;
>   olsr_u16_t         T_seq;
>   struct topo_dst   *next;
>   struct topo_dst   *prev;
>   double             link_quality;
>   double             inverse_link_quality;
>   double             saved_link_quality;
>   double             saved_inverse_link_quality;
> }; 
> 
> The 'saved_...' fields have gone in the 0.5.4 version of tc_set.h:
> 
> struct tc_edge_entry
> {
>   struct avl_node    edge_node; /* edge_tree node in tc_entry */
>   union olsr_ip_addr T_dest_addr; /* edge_node key */
>   struct tc_edge_entry *edge_inv; /* shortcut, used during SPF calculation
> */
>   struct tc_entry    *tc; /* backpointer to owning tc entry */
>   clock_t            T_time; /* expiration timer, timer_node key */
>   olsr_u16_t         T_seq; /* sequence number */
>   olsr_u16_t         flags; /* misc flags */
>   float              etx; /* metric used for SPF calculation */
>   float              link_quality;
>   float              inverse_link_quality;
> };
> 
> Am i missing something here?
> 
> Another point of interest: the function 'pkt_get_u32' has an error. It is
> left as an exercise to the reader to find it :-) . (It is not a problem,
> since that function is never used, unless you are writing a link-cost
> version of OLSR...)
> 
> Erik
> 
>