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.
> 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?
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
