From: Theo Buehler Subject: Re: bgpd: adjust prefix_evaluate for clearer rde_generate_updates logic To: tech@openbsd.org Date: Wed, 20 May 2026 12:08:33 +0200 On Tue, May 19, 2026 at 03:17:18PM +0200, Claudio Jeker wrote: > rde_generate_updates is called with new and old (old_pathid_tx) to > identify the paths that were added or removed. I always get lost in what > combinations work or may show up. So lets have a look: It's a super tricky beast... > There are some limitations when it comes to new and old passed to > prefix_evaluate(). > First of all, new->path_id_tx == old->path_id_tx unless new or old are > NULL. There is no way that a path is replaced with a different path. > Also in most cases if new != NULL then old == NULL and vice versa (but > there is also a case where both are set but then new != old). > > For rde_generate_updates() the situation is similar. Either new is valid or > old is valid. If both are invalid then there is no need to call the > function. If both are valid only new matters (or better make old_pathid_tx > be 0). > > prefix_evaluate_nexthop() behaves similar. If old was valid then new is > invalid but if old was invalid then new may still be invalid (because for > example the prefix may be filtered). Again if new is valid then there is > no need to pass old. > > The diff below tries to put this into code. This should make it easier for > the next step which extends rde_generate_updates() to include a queue of > updates. It ensures that rde_generate_updates() is called only with these > options: > new != NULL, old_pathid_tx = 0 > new == NULL, old_pathid_tx != 0 > > new == NULL, old_pathid_tx = 0 and new != NULL old_pathid_tx != 0 are both > invalid now. This all makes sense and I agree that this makes semantics clearer and should help make things more robust. Let's see how this works out in the next step and then in practice. ok tb. > -- > :wq Claudio > > Index: rde_decide.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v > diff -u -p -r1.106 rde_decide.c > --- rde_decide.c 1 Dec 2025 13:07:28 -0000 1.106 > +++ rde_decide.c 19 May 2026 13:09:23 -0000 > @@ -524,9 +524,11 @@ prefix_best(struct rib_entry *re) > /* > * Find the correct place to insert the prefix in the prefix list. > * If the active prefix has changed we need to send an update also special > - * treatment is needed if 'rde evaluate all' is used on some peers. > - * To re-evaluate a prefix just call prefix_evaluate with old and new pointing > - * to the same prefix. > + * treatment is needed if 'rde evaluate all' or add-path is used on some peers. > + * To re-evaluate a prefix it is best to first call prefix_evaluate with > + * new = NULL, old = prefix, adjust the prefix and then call prefix_evaluate > + * with new = prefix, old = NULL. This ensures proper evaluation in case > + * the prefix change influences prefix_eligible() or MED handling. > */ > void > prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old) > @@ -552,8 +554,13 @@ prefix_evaluate(struct rib_entry *re, st > prefix_remove(old, re); > old_pathid_tx = old->path_id_tx; > } > - if (new != NULL) > + if (new != NULL) { > prefix_insert(new, NULL, re); > + if (!prefix_eligible(new)) > + new = NULL; > + else > + old_pathid_tx = 0; > + } > newbest = prefix_best(re); > > /* > @@ -578,10 +585,10 @@ prefix_evaluate(struct rib_entry *re, st > * rde_generate_updates() will then take care of distribution. > */ > if (rde_evaluate_all()) { > - if (new != NULL && !prefix_eligible(new)) > - new = NULL; > - if (new != NULL || old != NULL) > - rde_generate_updates(re, new, old_pathid_tx, EVAL_ALL); > + /* no old path to remove and path is ineligible, skip rest */ > + if (old_pathid_tx == 0 && new == NULL) > + return; > + rde_generate_updates(re, new, old_pathid_tx, EVAL_ALL); > } > } > > @@ -592,7 +599,7 @@ prefix_evaluate_nexthop(struct prefix *p > struct rib_entry *re = prefix_re(p); > struct prefix *newbest, *oldbest, *new, *old; > struct rib *rib; > - uint32_t old_pathid_tx; > + uint32_t old_pathid_tx = 0; > > /* Skip non local-RIBs or RIBs that are flagged as noeval. */ > rib = re_rib(re); > @@ -625,7 +632,8 @@ prefix_evaluate_nexthop(struct prefix *p > > old = p; > prefix_remove(old, re); > - old_pathid_tx = old->path_id_tx; > + if (prefix_eligible(old)) > + old_pathid_tx = old->path_id_tx; > > if (state == NEXTHOP_REACH) > p->nhflags |= NEXTHOP_VALID; > @@ -636,6 +644,15 @@ prefix_evaluate_nexthop(struct prefix *p > prefix_insert(new, NULL, re); > newbest = prefix_best(re); > > + if (!prefix_eligible(new)) > + new = NULL; > + else > + old_pathid_tx = 0; > + > + /* path was and still is ineligible, skip rest */ > + if (old_pathid_tx == 0 && new == NULL) > + return; > + > /* > * If the active prefix changed or the active prefix was removed > * and added again then generate an update. > @@ -657,9 +674,6 @@ prefix_evaluate_nexthop(struct prefix *p > * to be passed on (not only a change of the best prefix). > * rde_generate_updates() will then take care of distribution. > */ > - if (rde_evaluate_all()) { > - if (!prefix_eligible(new)) > - new = NULL; > + if (rde_evaluate_all()) > rde_generate_updates(re, new, old_pathid_tx, EVAL_ALL); > - } > } >