On Wed, Mar 02, 2022 at 01:07:09PM +0100, Claudio Jeker wrote:
> On Wed, Mar 02, 2022 at 01:03:04PM +0100, Claudio Jeker wrote:
> > This diff changes prefix_adjout_withdraw() to take a prefix pointer
> > as argument. So instead of doing the lookup in the withdraw function the
> > caller may need to do it.
> >
> > With this one call to up_generate_updates() can be replaced with a direct
> > call to prefix_adjout_withdraw(). rde_up_flush_upcall() tries to withdraw
> > every prefix in the Adj-RIB-Out of a peer. The indirection via
> > up_generate_updates() makes little sense here.
>
> Forgot a hunk from the much bigger diff I wrestle with.
> There is no need to pass the peer to rde_up_flush_upcall. This also fixes
> a minor bug with rde_softreconfig_in_done() which expects arg to be
> NULL.
I'm ok with this, though I have one question.
>
> --
> :wq Claudio
>
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.538
> diff -u -p -r1.538 rde.c
> --- rde.c 28 Feb 2022 12:52:38 -0000 1.538
> +++ rde.c 2 Mar 2022 12:03:49 -0000
> @@ -3050,9 +3050,7 @@ rde_generate_updates(struct rib *rib, st
> static void
> rde_up_flush_upcall(struct prefix *p, void *ptr)
> {
> - struct rde_peer *peer = ptr;
> -
> - up_generate_updates(out_rules, peer, NULL, p);
> + prefix_adjout_withdraw(p);
> }
>
> u_char queue_buf[4096];
> @@ -3444,7 +3442,7 @@ rde_reload_done(void)
>
> if (peer->reconf_rib) {
> if (prefix_dump_new(peer, AID_UNSPEC,
> - RDE_RUNNER_ROUNDS, peer, rde_up_flush_upcall,
> + RDE_RUNNER_ROUNDS, NULL, rde_up_flush_upcall,
> rde_softreconfig_in_done, NULL) == -1)
> fatal("%s: prefix_dump_new", __func__);
> log_peer_info(&peer->conf, "flushing Adj-RIB-Out");
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.244
> diff -u -p -r1.244 rde.h
> --- rde.h 25 Feb 2022 11:36:54 -0000 1.244
> +++ rde.h 2 Mar 2022 11:49:59 -0000
> @@ -596,8 +596,7 @@ int prefix_withdraw(struct rib *, stru
> void prefix_add_eor(struct rde_peer *, uint8_t);
> int prefix_adjout_update(struct rde_peer *, struct filterstate *,
> struct bgpd_addr *, int, uint8_t);
> -int prefix_adjout_withdraw(struct rde_peer *, struct bgpd_addr *,
> - int);
> +int prefix_adjout_withdraw(struct prefix *);
> void prefix_adjout_destroy(struct prefix *p);
> void prefix_adjout_dump(struct rde_peer *, void *,
> void (*)(struct prefix *, void *));
> Index: rde_rib.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.230
> diff -u -p -r1.230 rde_rib.c
> --- rde_rib.c 1 Mar 2022 09:39:36 -0000 1.230
> +++ rde_rib.c 2 Mar 2022 11:49:39 -0000
> @@ -1252,21 +1252,19 @@ prefix_adjout_update(struct rde_peer *pe
> * the prefix in the RIB linked to the peer withdraw list.
> */
> int
> -prefix_adjout_withdraw(struct rde_peer *peer, struct bgpd_addr *prefix,
> - int prefixlen)
> +prefix_adjout_withdraw(struct prefix *p)
> {
> - struct prefix *p;
> -
> - p = prefix_adjout_get(peer, 0, prefix, prefixlen);
> - if (p == NULL) /* Got a dummy withdrawn request. */
> - return (0);
> + struct rde_peer *peer = prefix_peer(p);
>
> if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
>
> - /* already a withdraw, error */
> - if (p->flags & PREFIX_FLAG_WITHDRAW)
> - log_warnx("%s: prefix already withdrawed", __func__);
> + /* already a withdraw, shortcut */
> + if (p->flags & PREFIX_FLAG_WITHDRAW) {
> + p->lastchange = getmonotime();
> + p->flags &= ~PREFIX_FLAG_STALE;
> + return (0);
> + }
I'm a bit confused by this part. You changed this to an error a few days
ago, now you change it back to a shortcut. Why?
> /* pending update just got withdrawn */
> if (p->flags & PREFIX_FLAG_UPDATE)
> RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
> @@ -1279,7 +1277,7 @@ prefix_adjout_withdraw(struct rde_peer *
> p->lastchange = getmonotime();
>
> p->flags |= PREFIX_FLAG_WITHDRAW;
> - if (RB_INSERT(prefix_tree, &peer->withdraws[prefix->aid], p) != NULL)
> + if (RB_INSERT(prefix_tree, &peer->withdraws[p->pt->aid], p) != NULL)
> fatalx("%s: RB tree invariant violated", __func__);
> return (1);
> }
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 rde_update.c
> --- rde_update.c 1 Mar 2022 09:53:42 -0000 1.134
> +++ rde_update.c 2 Mar 2022 11:49:39 -0000
> @@ -102,6 +102,7 @@ up_generate_updates(struct filter_head *
> {
> struct filterstate state;
> struct bgpd_addr addr;
> + struct prefix *p;
> int need_withdraw;
> uint8_t prefixlen;
>
> @@ -119,7 +120,9 @@ up_generate_updates(struct filter_head *
> again:
> if (new == NULL) {
> /* withdraw prefix */
> - if (prefix_adjout_withdraw(peer, &addr, prefixlen) == 1) {
> + if ((p = prefix_adjout_get(peer, 0, &addr, prefixlen)) == NULL)
> + return;
> + if (prefix_adjout_withdraw(p) == 1) {
> peer->prefix_out_cnt--;
> peer->up_wcnt++;
> }
>
