From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: simplify up_generate_addpath_all
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 12 Nov 2025 22:48:29 +0100

On Wed, Nov 12, 2025 at 07:11:42PM +0100, Theo Buehler wrote:
> On Wed, Nov 12, 2025 at 05:11:50PM +0100, Claudio Jeker wrote:
> > up_generate_addpath_all() should just call up_generate_addpath() for the
> > case where new and old are NULL.
> > Doing that removes a lot of unneeded complexity which is very helpful.
> 
> The direct call to up_generate_addpath() helps a lot and I think the
> additional mode checks are harmless/desirable.
> 
> Something isn't quite right though. You can't call up_process_prefix()
> with new == NULL. That'll explode at frompeer = prefix_peer(p); in
> up_test_update() unless I'm misreading something.
 
You are totally right. Below diff fixes this.
-- 
:wq Claudio

Index: rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
diff -u -p -r1.177 rde_update.c
--- rde_update.c	4 Nov 2025 15:01:09 -0000	1.177
+++ rde_update.c	12 Nov 2025 21:45:21 -0000
@@ -343,21 +343,15 @@ void
 up_generate_addpath_all(struct rde_peer *peer, struct rib_entry *re,
     struct prefix *new, struct prefix *old)
 {
-	struct prefix		*p, *head = NULL;
-	int			all = 0;
+	struct prefix		*p;
 
 	/*
-	 * if old and new are NULL then insert all prefixes from best,
-	 * clearing old routes in the process
+	 * If old and new are NULL then re-insert all prefixes from re,
+	 * use up_generate_addpath() for that.
 	 */
 	if (old == NULL && new == NULL) {
-		/* mark all paths as stale */
-		head = prefix_adjout_first(peer, re->prefix);
-		for (p = head; p != NULL; p = prefix_adjout_next(peer, p))
-			p->flags |= PREFIX_FLAG_STALE;
-
-		new = prefix_best(re);
-		all = 1;
+		up_generate_addpath(peer, re);
+		return;
 	}
 
 	if (new != NULL && !prefix_eligible(new)) {
@@ -366,14 +360,14 @@ up_generate_addpath_all(struct rde_peer 
 	}
 
 	if (old != NULL) {
-		/* withdraw stale paths */
+		/* withdraw old path */
 		p = prefix_adjout_get(peer, old->path_id_tx, old->pt);
 		if (p != NULL)
 			prefix_adjout_withdraw(p);
 	}
 
-	/* add new path (or multiple if all is set) */
-	while (new != NULL) {
+	if (new != NULL) {
+		/* add new path */
 		switch (up_process_prefix(peer, new, (void *)-1)) {
 		case UP_OK:
 		case UP_FILTERED:
@@ -382,22 +376,6 @@ up_generate_addpath_all(struct rde_peer 
 		case UP_ERR_LIMIT:
 			/* just give up */
 			return;
-		}
-
-		if (!all)
-			break;
-
-		/* only allow valid prefixes */
-		new = TAILQ_NEXT(new, entry.list.rib);
-		if (new == NULL || !prefix_eligible(new))
-			break;
-	}
-
-	if (all) {
-		/* withdraw stale paths */
-		for (p = head; p != NULL; p = prefix_adjout_next(peer, p)) {
-			if (p->flags & PREFIX_FLAG_STALE)
-				prefix_adjout_withdraw(p);
 		}
 	}
 }