On Wed, Mar 30, 2022 at 03:10:58PM +0200, Theo Buehler wrote:
> 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.
Agreed, this is even better. OK claudio@
> 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);
>
--
:wq Claudio
