Re: bgpd, don't allocate initial aspath for update parsing
On Mon, Jul 09, 2018 at 05:47:11PM +0200, Claudio Jeker wrote:
> Similar to the rde_filter code there is no need to path_get() the aspath
> used in rde_update_dispatch(). Also makes the code a bit easier since the
> cleanup can be done all the time.
>
looks good, compiles fine, runs well, OK denis@
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.386
> diff -u -p -r1.386 rde.c
> --- rde.c 9 Jul 2018 14:44:02 - 1.386
> +++ rde.c 9 Jul 2018 14:50:21 -
> @@ -938,10 +938,10 @@ rde_dispatch_imsg_parent(struct imsgbuf
> int
> rde_update_dispatch(struct imsg *imsg)
> {
> + struct rde_aspathasp;
> struct bgpd_addr prefix;
> struct mpattrmpa;
> struct rde_peer *peer;
> - struct rde_aspath *asp = NULL;
> u_char *p, *mpp = NULL;
> int error = -1, pos = 0;
> u_int16_tafi, len, mplen;
> @@ -986,11 +986,11 @@ rde_update_dispatch(struct imsg *imsg)
> imsg->hdr.len - IMSG_HEADER_SIZE - 4 - withdrawn_len - attrpath_len;
> bzero(&mpa, sizeof(mpa));
>
> + path_prep(&asp);
> if (attrpath_len != 0) { /* 0 = no NLRI information in this message */
> /* parse path attributes */
> - asp = path_get();
> while (len > 0) {
> - if ((pos = rde_attr_parse(p, len, peer, asp,
> + if ((pos = rde_attr_parse(p, len, peer, &asp,
> &mpa)) < 0)
> goto done;
> p += pos;
> @@ -998,19 +998,19 @@ rde_update_dispatch(struct imsg *imsg)
> }
>
> /* check for missing but necessary attributes */
> - if ((subtype = rde_attr_missing(asp, peer->conf.ebgp,
> + if ((subtype = rde_attr_missing(&asp, peer->conf.ebgp,
> nlri_len))) {
> rde_update_err(peer, ERR_UPDATE, ERR_UPD_MISSNG_WK_ATTR,
> &subtype, sizeof(u_int8_t));
> goto done;
> }
>
> - rde_as4byte_fixup(peer, asp);
> + rde_as4byte_fixup(peer, &asp);
>
> /* enforce remote AS if requested */
> - if (asp->flags & F_ATTR_ASPATH &&
> + if (asp.flags & F_ATTR_ASPATH &&
> peer->conf.enforce_as == ENFORCE_AS_ON) {
> - fas = aspath_neighbor(asp->aspath);
> + fas = aspath_neighbor(asp.aspath);
> if (peer->conf.remote_as != fas) {
> log_peer_warnx(&peer->conf, "bad path, "
> "starting with %s, "
> @@ -1021,7 +1021,7 @@ rde_update_dispatch(struct imsg *imsg)
> }
> }
>
> - rde_reflector(peer, asp);
> + rde_reflector(peer, &asp);
> }
>
> p = imsg->data;
> @@ -1103,7 +1103,7 @@ rde_update_dispatch(struct imsg *imsg)
> goto done;
> }
>
> - if ((asp->flags & ~F_ATTR_MP_UNREACH) == 0 && mplen == 0) {
> + if ((asp.flags & ~F_ATTR_MP_UNREACH) == 0 && mplen == 0) {
> /* EoR marker */
> peer_recv_eor(peer, aid);
> }
> @@ -1166,7 +1166,7 @@ rde_update_dispatch(struct imsg *imsg)
> break;
> }
>
> - if ((asp->flags & ~F_ATTR_MP_UNREACH) == 0) {
> + if ((asp.flags & ~F_ATTR_MP_UNREACH) == 0) {
> error = 0;
> goto done;
> }
> @@ -1178,8 +1178,8 @@ rde_update_dispatch(struct imsg *imsg)
> /* aspath needs to be loop free nota bene this is not a hard error */
> if (peer->conf.ebgp &&
> peer->conf.enforce_local_as == ENFORCE_AS_ON &&
> - !aspath_loopfree(asp->aspath, peer->conf.local_as))
> - asp->flags |= F_ATTR_LOOP;
> + !aspath_loopfree(asp.aspath, peer->conf.local_as))
> + asp.flags |= F_ATTR_LOOP;
>
> /* parse nlri prefix */
> while (nlri_len > 0) {
> @@ -1208,7 +1208,7 @@ rde_update_dispatch(struct imsg *imsg)
> goto done;
> }
>
> - if (rde_update_update(peer, asp, &prefix, prefixlen) == -1)
> + if (rde_update_update(peer, &asp, &prefix, prefixlen) == -1)
> goto done;
>
> }
> @@ -1244,11 +1244,11 @@ rde_update_dispatch(struct imsg *imsg)
>* this works because asp is not linked.
>* But first unlock the previously locked nexthop.
>*/
> - if (asp->nexthop) {
> - (void)nexthop_put(asp->nexthop);
> - asp->nexthop = NULL;
> + if (asp.nexthop) {
> +
bgpd, don't allocate initial aspath for update parsing
Similar to the rde_filter code there is no need to path_get() the aspath
used in rde_update_dispatch(). Also makes the code a bit easier since the
cleanup can be done all the time.
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.386
diff -u -p -r1.386 rde.c
--- rde.c 9 Jul 2018 14:44:02 - 1.386
+++ rde.c 9 Jul 2018 14:50:21 -
@@ -938,10 +938,10 @@ rde_dispatch_imsg_parent(struct imsgbuf
int
rde_update_dispatch(struct imsg *imsg)
{
+ struct rde_aspathasp;
struct bgpd_addr prefix;
struct mpattrmpa;
struct rde_peer *peer;
- struct rde_aspath *asp = NULL;
u_char *p, *mpp = NULL;
int error = -1, pos = 0;
u_int16_tafi, len, mplen;
@@ -986,11 +986,11 @@ rde_update_dispatch(struct imsg *imsg)
imsg->hdr.len - IMSG_HEADER_SIZE - 4 - withdrawn_len - attrpath_len;
bzero(&mpa, sizeof(mpa));
+ path_prep(&asp);
if (attrpath_len != 0) { /* 0 = no NLRI information in this message */
/* parse path attributes */
- asp = path_get();
while (len > 0) {
- if ((pos = rde_attr_parse(p, len, peer, asp,
+ if ((pos = rde_attr_parse(p, len, peer, &asp,
&mpa)) < 0)
goto done;
p += pos;
@@ -998,19 +998,19 @@ rde_update_dispatch(struct imsg *imsg)
}
/* check for missing but necessary attributes */
- if ((subtype = rde_attr_missing(asp, peer->conf.ebgp,
+ if ((subtype = rde_attr_missing(&asp, peer->conf.ebgp,
nlri_len))) {
rde_update_err(peer, ERR_UPDATE, ERR_UPD_MISSNG_WK_ATTR,
&subtype, sizeof(u_int8_t));
goto done;
}
- rde_as4byte_fixup(peer, asp);
+ rde_as4byte_fixup(peer, &asp);
/* enforce remote AS if requested */
- if (asp->flags & F_ATTR_ASPATH &&
+ if (asp.flags & F_ATTR_ASPATH &&
peer->conf.enforce_as == ENFORCE_AS_ON) {
- fas = aspath_neighbor(asp->aspath);
+ fas = aspath_neighbor(asp.aspath);
if (peer->conf.remote_as != fas) {
log_peer_warnx(&peer->conf, "bad path, "
"starting with %s, "
@@ -1021,7 +1021,7 @@ rde_update_dispatch(struct imsg *imsg)
}
}
- rde_reflector(peer, asp);
+ rde_reflector(peer, &asp);
}
p = imsg->data;
@@ -1103,7 +1103,7 @@ rde_update_dispatch(struct imsg *imsg)
goto done;
}
- if ((asp->flags & ~F_ATTR_MP_UNREACH) == 0 && mplen == 0) {
+ if ((asp.flags & ~F_ATTR_MP_UNREACH) == 0 && mplen == 0) {
/* EoR marker */
peer_recv_eor(peer, aid);
}
@@ -1166,7 +1166,7 @@ rde_update_dispatch(struct imsg *imsg)
break;
}
- if ((asp->flags & ~F_ATTR_MP_UNREACH) == 0) {
+ if ((asp.flags & ~F_ATTR_MP_UNREACH) == 0) {
error = 0;
goto done;
}
@@ -1178,8 +1178,8 @@ rde_update_dispatch(struct imsg *imsg)
/* aspath needs to be loop free nota bene this is not a hard error */
if (peer->conf.ebgp &&
peer->conf.enforce_local_as == ENFORCE_AS_ON &&
- !aspath_loopfree(asp->aspath, peer->conf.local_as))
- asp->flags |= F_ATTR_LOOP;
+ !aspath_loopfree(asp.aspath, peer->conf.local_as))
+ asp.flags |= F_ATTR_LOOP;
/* parse nlri prefix */
while (nlri_len > 0) {
@@ -1208,7 +1208,7 @@ rde_update_dispatch(struct imsg *imsg)
goto done;
}
- if (rde_update_update(peer, asp, &prefix, prefixlen) == -1)
+ if (rde_update_update(peer, &asp, &prefix, prefixlen) == -1)
goto done;
}
@@ -1244,11 +1244,11 @@ rde_update_dispatch(struct imsg *imsg)
* this works because asp is not linked.
* But first unlock the previously locked nexthop.
*/
- if (asp->nexthop) {
- (void)nexthop_put(asp->nexthop);
- asp->nexthop = NULL;
+ if (asp.nexthop) {
+ (void)nexthop_put(asp.nexthop);
+ asp.nexthop = NULL;
}
- if ((pos = rde_get_mp_nextho
