Hi Daniel
On Mon, Nov 23, 2015 at 10:08 PM, Daniel Walton <[email protected] > wrote: > > > On Mon, Nov 23, 2015 at 9:29 AM, Balaji Gurudoss <[email protected]> > wrote: > >> Hi Daniel >> >> Thanks a lot for the comments. >> >>> >>>> >>>> diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c >>>> index be9b480..38f5029 100644 >>>> --- a/bgpd/bgp_advertise.c >>>> +++ b/bgpd/bgp_advertise.c >>>> @@ -264,6 +264,7 @@ bgp_adj_out_set (struct bgp_node *rn, struct peer >>>> *peer, struct prefix *p, >>>> bgp_advertise_add (adv->baa, adv); >>>> >>>> FIFO_ADD (&peer->sync[afi][safi]->update, &adv->fifo); >>>> + peer->table_version++; >>>> } >>>> >>>> void >>>> @@ -299,6 +300,7 @@ bgp_adj_out_unset (struct bgp_node *rn, struct peer >>>> *peer, struct prefix *p, >>>> /* Add to synchronization entry for withdraw announcement. */ >>>> FIFO_ADD (&peer->sync[afi][safi]->withdraw, &adv->fifo); >>>> >>>> + peer->table_version++; >>>> /* Schedule packet write. */ >>>> BGP_WRITE_ON (peer->t_write, bgp_write, peer->fd); >>>> } >>>> >>> >>> How the table_version in maintained for the peer structure looks a >>> little odd. Typically if a peer's tblver is 57 you know that you have >>> converged that peer for all prefixes whose tblver is <= 57. What you have >>> above is just a counter for how many times something has been added/removed >>> from the peer's adj_out. >>> >> >> >>> >>> You could instead set the peer's tblver based on the max between the >>> peer's current tblver and the tblver of the prefix being advertised. >>> >> >> I am sorry i didn't get your point. >> > > With your patch the peer's tbl_ver is just a counter for how many times an > adj has been added/removed from the peer's adj_out. That would be a very > different meaning for peer tblver vs. what it means in other > implemenations...I think this would cause confusion for the user. > Yeah i get your point. I increment the peer table_Version in the following scenarios 1. When the best path gets advertised 2. When the best path gets withdrawn 3. When the peer gets to Established State (gets initialized to 1) > > > >> diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c >>>> index 316fa5a..8753c02 100644 >>>> --- a/bgpd/bgp_route.c >>>> +++ b/bgpd/bgp_route.c >>>> @@ -1515,7 +1515,6 @@ bgp_process_announce_selected (struct peer *peer, >>>> struct bgp_info *selected, >>>> bgp_adj_out_unset (rn, peer, p, afi, safi); >>>> break; >>>> } >>>> - >>>> return 0; >>>> } >>>> >>>> @@ -1625,7 +1624,11 @@ bgp_process_main (struct work_queue *wq, void >>>> *data) >>>> } >>>> >>>> if (old_select) >>>> - bgp_info_unset_flag (rn, old_select, BGP_INFO_SELECTED); >>>> + { >>>> + bgp_info_unset_flag (rn, old_select, BGP_INFO_SELECTED); >>>> + bgp->table_version++; >>>> + old_select->peer->table_version++; >>>> + } >>>> if (new_select) >>>> { >>>> bgp_info_set_flag (rn, new_select, BGP_INFO_SELECTED); >>>> >>> The flag is set here ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > so no need to check to see if it is set below. Why not bump the > table_verison inside this if? > > Ack. Yeah that could be done and move the flag check inside the new_select as you say. Will address that in v2 Thanks for your time and patience in reviewing this. Thanks, - Balaji > Daniel > > > >> @@ -1633,6 +1636,11 @@ bgp_process_main (struct work_queue *wq, void >>>> *data) >>>> UNSET_FLAG (new_select->flags, BGP_INFO_MULTIPATH_CHG); >>>> } >>>> >>>> + if (new_select && CHECK_FLAG (new_select->flags, BGP_INFO_SELECTED)) >>>> + { >>>> + bgp->table_version++; >>>> + new_select->peer->table_version++; >>>> + } >>>> >>> >>> Why not just bump the table_versions in the "if (new_select)" that is >>> right after the "if (old_select)" up above? >>> >> >> The table version actually gets incremented when the changes happen for >> any of the best paths available. Hence i check if the flag is set before i >> update the version. >> > > >> >> >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
