From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: simplify up_generate_addpath_all
To: tech@openbsd.org
Date: Wed, 12 Nov 2025 19:11:42 +0100

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.

> 
> -- 
> :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 15:38:19 -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,39 +360,21 @@ 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) {
> -		switch (up_process_prefix(peer, new, (void *)-1)) {
> -		case UP_OK:
> -		case UP_FILTERED:
> -		case UP_EXCLUDED:
> -			break;
> -		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);
> -		}
> +	/* add new path */
> +	switch (up_process_prefix(peer, new, (void *)-1)) {
> +	case UP_OK:
> +	case UP_FILTERED:
> +	case UP_EXCLUDED:
> +		break;
> +	case UP_ERR_LIMIT:
> +		/* just give up */
> +		return;
>  	}
>  }
>  
>