On Wed, Mar 30, 2022 at 02:38:54PM +0200, Claudio Jeker wrote:
> Change the code to use less goto and instead use a while loop.
> I think the result is easier to understand.
Yes this is clearer and preserves the current logic, so I'm ok with it.
Here's an alternative approach: unless I'm missing something, the only
case that actually redoes the while loop and cares about new is if the
TAILQ_NEXT() returns an eligible prefix, so I think the below diff is
equivalent to yours (regress is still happy). I also removed various
new = NULL since nothing looks at new afterward.
Index: rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
retrieving revision 1.138
diff -u -p -r1.138 rde_update.c
--- rde_update.c 22 Mar 2022 10:53:08 -0000 1.138
+++ rde_update.c 30 Mar 2022 12:54:02 -0000
@@ -117,12 +117,7 @@ up_generate_updates(struct filter_head *
prefixlen = new->pt->prefixlen;
}
-again:
- if (new == NULL) {
- /* withdraw prefix */
- if ((p = prefix_adjout_get(peer, 0, &addr, prefixlen)) != NULL)
- prefix_adjout_withdraw(p);
- } else {
+ while (new != NULL) {
need_withdraw = 0;
/*
* up_test_update() needs to run before the output filters
@@ -142,10 +137,8 @@ again:
* skip the filters.
*/
if (need_withdraw &&
- !(peer->flags & PEERFLAG_EVALUATE_ALL)) {
- new = NULL;
- goto again;
- }
+ !(peer->flags & PEERFLAG_EVALUATE_ALL))
+ break;
rde_filterstate_prep(&state, prefix_aspath(new),
prefix_communities(new), prefix_nexthop(new),
@@ -153,19 +146,19 @@ again:
if (rde_filter(rules, peer, prefix_peer(new), &addr,
prefixlen, prefix_vstate(new), &state) == ACTION_DENY) {
rde_filterstate_clean(&state);
- if (peer->flags & PEERFLAG_EVALUATE_ALL)
+ if (peer->flags & PEERFLAG_EVALUATE_ALL) {
new = TAILQ_NEXT(new, entry.list.rib);
- else
- new = NULL;
- if (new != NULL && !prefix_eligible(new))
- new = NULL;
- goto again;
+ if (new != NULL && prefix_eligible(new))
+ continue;
+ }
+ break;
}
- if (need_withdraw) {
- new = NULL;
- goto again;
- }
+ /* check if this was actually a withdraw */
+ if (need_withdraw)
+ break;
+
+ /* from here on we know this is an update */
up_prep_adjout(peer, &state, addr.aid);
prefix_adjout_update(peer, &state, &addr,
@@ -181,7 +174,13 @@ again:
rde_update_err(peer, ERR_CEASE,
ERR_CEASE_MAX_SENT_PREFIX, NULL, 0);
}
+
+ return;
}
+
+ /* withdraw prefix */
+ if ((p = prefix_adjout_get(peer, 0, &addr, prefixlen)) != NULL)
+ prefix_adjout_withdraw(p);
}
struct rib_entry *rib_add(struct rib *, struct bgpd_addr *, int);
