Re: bgpd rework how prefixes are written

[Claudio Jeker]

On Tue, Mar 28, 2023 at 05:02:26PM +0200, Theo Buehler wrote:
> On Tue, Mar 28, 2023 at 03:35:46PM +0200, Claudio Jeker wrote:
> > This diff moves prefix_write to rde_prefix.c and renames it to pt_write.
> > The function now takes a struct pt_entry * as argument and with this the
> > extra indirection via pt_getaddr() falls away.
> 
> I'm ok with this, although it's not entirely obvious to me why this is
> better.

I want to remove most usage of pt_getaddr() and pt_fill() mainly to have
less dependency on struct bgpd_addr. The problem is that adding flowspec
to struct bgpd_addr does not really work. So using struct pt_entry
pointers works better since the representation is then purely internal.

Right now my goal is to get most of the Adj-RIB-Out handling cleaned up
making it possible to announce flowspec rules from OpenBGPD.

> I found this diff hard to review. Perhaps you could land it with an
> intermediate step that only moves the two functions without doing
> anything else. Once I did this, it was a lot easier to see what
> happened.

Sorry about that. Will try to remember for next time. 
I already split all the pt_entry churn into multible diffs but guess N + 1
is the rule :)

-- 
:wq Claudio

Re: bgpd rework how prefixes are written

[Theo Buehler]

On Tue, Mar 28, 2023 at 03:35:46PM +0200, Claudio Jeker wrote:
> This diff moves prefix_write to rde_prefix.c and renames it to pt_write.
> The function now takes a struct pt_entry * as argument and with this the
> extra indirection via pt_getaddr() falls away.

I'm ok with this, although it's not entirely obvious to me why this is
better.

I found this diff hard to review. Perhaps you could land it with an
intermediate step that only moves the two functions without doing
anything else. Once I did this, it was a lot easier to see what
happened.

bgpd rework how prefixes are written

[Claudio Jeker]

This diff moves prefix_write to rde_prefix.c and renames it to pt_write.
The function now takes a struct pt_entry * as argument and with this the
extra indirection via pt_getaddr() falls away.

-- 
:wq Claudio

Index: mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.112
diff -u -p -r1.112 mrt.c
--- mrt.c   28 Mar 2023 09:52:08 -  1.112
+++ mrt.c   28 Mar 2023 13:33:41 -
@@ -387,7 +387,7 @@ mrt_dump_entry_mp(struct mrt *mrt, struc
 {
struct ibuf *buf, *hbuf = NULL, *h2buf = NULL;
struct nexthop  *n;
-   struct bgpd_addr addr, nexthop, *nh;
+   struct bgpd_addr nexthop, *nh;
uint16_t len;
uint8_t  aid;
 
@@ -445,17 +445,15 @@ mrt_dump_entry_mp(struct mrt *mrt, struc
/* originated timestamp */
DUMP_LONG(h2buf, time(NULL) - (getmonotime() - p->lastchange));
 
-   pt_getaddr(p->pt, );
-
n = prefix_nexthop(p);
if (n == NULL) {
memset(, 0, sizeof(struct bgpd_addr));
-   nexthop.aid = addr.aid;
+   nexthop.aid = p->pt->aid;
nh = 
} else
nh = >exit_nexthop;
 
-   switch (addr.aid) {
+   switch (p->pt->aid) {
case AID_INET:
DUMP_SHORT(h2buf, AFI_IPv4);/* afi */
DUMP_BYTE(h2buf, SAFI_UNICAST); /* safi */
@@ -495,8 +493,8 @@ mrt_dump_entry_mp(struct mrt *mrt, struc
goto fail;
}
 
-   if (prefix_writebuf(h2buf, , p->pt->prefixlen) == -1) {
-   log_warnx("%s: prefix_writebuf error", __func__);
+   if (pt_writebuf(h2buf, p->pt) == -1) {
+   log_warnx("%s: pt_writebuf error", __func__);
goto fail;
}
 
@@ -689,7 +687,6 @@ int
 mrt_dump_entry_v2(struct mrt *mrt, struct rib_entry *re, uint32_t snum)
 {
struct ibuf *hbuf = NULL, *nbuf = NULL, *apbuf = NULL, *pbuf;
-   struct bgpd_addr addr;
size_t   hlen, len;
uint16_t subtype, apsubtype, nump, apnump, afi;
uint8_t  safi;
@@ -725,9 +722,8 @@ mrt_dump_entry_v2(struct mrt *mrt, struc
break;
}
 
-   pt_getaddr(re->prefix, );
-   if (prefix_writebuf(pbuf, , re->prefix->prefixlen) == -1) {
-   log_warnx("%s: prefix_writebuf error", __func__);
+   if (pt_writebuf(pbuf, re->prefix) == -1) {
+   log_warnx("%s: pt_writebuf error", __func__);
goto fail;
}
 
Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.287
diff -u -p -r1.287 rde.h
--- rde.h   28 Mar 2023 13:30:31 -  1.287
+++ rde.h   28 Mar 2023 13:33:41 -
@@ -517,6 +517,8 @@ struct pt_entry *pt_add(struct bgpd_addr
 voidpt_remove(struct pt_entry *);
 struct pt_entry*pt_lookup(struct bgpd_addr *);
 int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
+int pt_write(u_char *, int, struct pt_entry *, int);
+int pt_writebuf(struct ibuf *, struct pt_entry *);
 
 static inline struct pt_entry *
 pt_ref(struct pt_entry *pt)
@@ -602,8 +604,6 @@ int  prefix_dump_subtree(struct rde_pee
uint8_t, unsigned int, void *,
void (*)(struct prefix *, void *),
void (*)(void *, uint8_t), int (*)(void *));
-int prefix_write(u_char *, int, struct bgpd_addr *, uint8_t, int);
-int prefix_writebuf(struct ibuf *, struct bgpd_addr *, uint8_t);
 struct prefix  *prefix_bypeer(struct rib_entry *, struct rde_peer *,
uint32_t);
 voidprefix_destroy(struct prefix *);
Index: rde_prefix.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_prefix.c,v
retrieving revision 1.44
diff -u -p -r1.44 rde_prefix.c
--- rde_prefix.c28 Mar 2023 13:30:31 -  1.44
+++ rde_prefix.c28 Mar 2023 13:33:41 -
@@ -371,3 +371,121 @@ pt_free(struct pt_entry *pte)
rdemem.pt_size[pte->aid] -= pt_sizes[pte->aid];
free(pte);
 }
+
+/* dump a prefix into specified buffer */
+int
+pt_write(u_char *buf, int len, struct pt_entry *pte, int withdraw)
+{
+   struct pt_entry_vpn4*pvpn4 = (struct pt_entry_vpn4 *)pte;
+   struct pt_entry_vpn6*pvpn6 = (struct pt_entry_vpn6 *)pte;
+   int  totlen, psize;
+   uint8_t  plen;
+
+   switch (pte->aid) {
+   case AID_INET:
+   case AID_INET6:
+   plen = pte->prefixlen;
+   totlen = PREFIX_SIZE(plen);
+
+   if (totlen > len)
+   return (-1);
+   *buf++ = plen;
+   memcpy(buf, pte->data, totlen - 1);
+   return (totlen);
+   case AID_VPN_IPv4:
+   plen =