On Mon, Feb 28, 2022 at 02:32:07PM +0100, Theo Buehler wrote: > On Mon, Feb 28, 2022 at 12:35:09PM +0100, Claudio Jeker wrote: > > From the start bgpd had prefix_link and prefix_unlink to link all the > > various data objects together to build an actual prefix. Now prefix_move() > > tries to be smart and reimplemented prefix_link and prefix_unlink as > > inline versions (with minimal differences). Later the prefix_adjout_* > > functions were added and again similar code to prefix_link / prefix_unlink > > was added. > > > > This diff removes the code duplication and uses prefix_link and > > prefix_unlink in all those extra places. This removes a common error when > > changing the object layout because one of the many codepaths was often > > missed. > > > > To make this happen prefix_link() gained an extra argument for the > > pt_entry (the adjout has no rib_entry and so no way to get the pt from > > there). It also moves the pftable handling and prefix_evaluate() call to > > the currently only caller (prefix_add). Add an extra check in nexthop_link > > to skip prefixes in the adj-rib-out since those are not linked up. > > With this prefix_link can be used in all other places. > > > > prefix_unlink can be almost used as is only the pt_unref() needs attention > > (or actually setting p->pt to NULL). In the Adj-RIB-Out prefixes are > > unlinked but remain on the RB tree to act as withdraw requests. For this > > prefix_adjout_update() adds an extra reference to the pt_entry so that > > after calling prefix_unlink() the pt_entry is still valid. > > prefix_adjout_destroy() takes this last reference before freeing the > > prefix again. > > > > The result is a much cleaner handling of prefixes and far fewer places to > > introduce new bugs. > > It's a tricky diff to review, but it looks all correct. The result is > indeed much cleaner
Thanks for the review, I know this is tricky stuff especially since there are so many things struct prefix is tracking. -- :wq Claudio
