Download raw body.
bgpd: Adjust BGP FSM handling in change_state
change_state() switches the peer FSM form one state to another.
One thing I dislike is that it does a lot of work before changing the
state and so it is possible to shoot yourself with e.g. session_up() that
is called with a peer->state that is not ESTABLISHED.
I prefer to first update the peer state (and prev_state) and then do all
the fumbling using peer->state and peer->prev_state. This makes the code
less of a mind bender.
As a side-effect the mrt_state_dump call becomes a lot simpler and
log_statechange() would also but there is this 'don't clutter the logs'
magic that needs 3 states. But even that is less horrible like this.
--
:wq Claudio
Index: logmsg.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/logmsg.c,v
diff -u -p -r1.14 logmsg.c
--- logmsg.c 20 May 2024 10:00:00 -0000 1.14
+++ logmsg.c 14 Apr 2025 14:53:43 -0000
@@ -111,24 +111,21 @@ log_peer_warnx(const struct peer_config
}
void
-log_statechange(struct peer *peer, enum session_state nstate,
+log_statechange(struct peer *peer, enum session_state ostate,
enum session_events event)
{
- char *p;
-
/* don't clutter the logs with constant Connect -> Active -> Connect */
- if (nstate == STATE_CONNECT && peer->state == STATE_ACTIVE &&
- peer->prev_state == STATE_CONNECT)
+ if (peer->state == STATE_CONNECT && peer->prev_state == STATE_ACTIVE &&
+ ostate == STATE_CONNECT)
return;
- if (nstate == STATE_ACTIVE && peer->state == STATE_CONNECT &&
- peer->prev_state == STATE_ACTIVE)
+ if (peer->state == STATE_ACTIVE && peer->prev_state == STATE_CONNECT &&
+ ostate == STATE_ACTIVE)
return;
peer->lasterr = 0;
- p = log_fmt_peer(&peer->conf);
- logit(LOG_INFO, "%s: state change %s -> %s, reason: %s",
- p, statenames[peer->state], statenames[nstate], eventnames[event]);
- free(p);
+ log_peer_info(&peer->conf, "state change %s -> %s, reason: %s",
+ statenames[peer->prev_state], statenames[peer->state],
+ eventnames[event]);
}
void
Index: mrt.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
diff -u -p -r1.126 mrt.c
--- mrt.c 20 Feb 2025 19:47:31 -0000 1.126
+++ mrt.c 14 Apr 2025 14:53:43 -0000
@@ -190,8 +190,7 @@ fail:
}
void
-mrt_dump_state(struct mrt *mrt, uint16_t old_state, uint16_t new_state,
- struct peer *peer)
+mrt_dump_state(struct mrt *mrt, struct peer *peer)
{
struct ibuf *buf;
uint16_t subtype = BGP4MP_STATE_CHANGE;
@@ -203,9 +202,9 @@ mrt_dump_state(struct mrt *mrt, uint16_t
2 * sizeof(short), 0) == -1)
goto fail;
- if (ibuf_add_n16(buf, old_state) == -1)
+ if (ibuf_add_n16(buf, peer->prev_state) == -1)
goto fail;
- if (ibuf_add_n16(buf, new_state) == -1)
+ if (ibuf_add_n16(buf, peer->state) == -1)
goto fail;
ibuf_close(mrt->wbuf, buf);
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.525 session.c
--- session.c 14 Apr 2025 14:52:34 -0000 1.525
+++ session.c 14 Apr 2025 14:53:44 -0000
@@ -1089,8 +1089,7 @@ session_graceful_flush(struct peer *p, u
}
void
-session_mrt_dump_state(struct peer *p, enum session_state oldstate,
- enum session_state newstate)
+session_mrt_dump_state(struct peer *p)
{
struct mrt *mrt;
@@ -1100,7 +1099,7 @@ session_mrt_dump_state(struct peer *p, e
if ((mrt->peer_id == 0 && mrt->group_id == 0) ||
mrt->peer_id == p->conf.id || (mrt->group_id != 0 &&
mrt->group_id == p->conf.groupid))
- mrt_dump_state(mrt, oldstate, newstate, p);
+ mrt_dump_state(mrt, p);
}
}
Index: session.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
diff -u -p -r1.191 session.h
--- session.h 26 Feb 2025 19:31:31 -0000 1.191
+++ session.h 14 Apr 2025 14:53:44 -0000
@@ -263,7 +263,7 @@ unsigned int control_accept(int, int);
/* log.c */
char *log_fmt_peer(const struct peer_config *);
-void log_statechange(struct peer *, enum session_state,
+void log_statechange(struct peer *, enum session_state,
enum session_events);
void log_notification(const struct peer *, uint8_t, uint8_t,
const struct ibuf *, const char *);
@@ -273,8 +273,7 @@ void log_conn_attempt(const struct peer
/* mrt.c */
void mrt_dump_bgp_msg(struct mrt *, struct ibuf *, struct peer *,
enum msg_type);
-void mrt_dump_state(struct mrt *, uint16_t, uint16_t,
- struct peer *);
+void mrt_dump_state(struct mrt *, struct peer *);
void mrt_done(struct mrt *);
/* pfkey.c */
@@ -339,8 +338,7 @@ void session_handle_update(struct peer
void session_handle_rrefresh(struct peer *, struct route_refresh *);
void session_graceful_restart(struct peer *);
void session_graceful_flush(struct peer *, uint8_t, const char *);
-void session_mrt_dump_state(struct peer *, enum session_state,
- enum session_state);
+void session_mrt_dump_state(struct peer *);
void session_mrt_dump_bgp_msg(struct peer *, struct ibuf *,
enum msg_type, enum directions);
int peer_matched(struct peer *, struct ctl_neighbor *);
Index: session_bgp.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v
diff -u -p -r1.4 session_bgp.c
--- session_bgp.c 27 Feb 2025 13:35:00 -0000 1.4
+++ session_bgp.c 14 Apr 2025 14:53:44 -0000
@@ -1799,10 +1799,18 @@ void
change_state(struct peer *peer, enum session_state state,
enum session_events event)
{
- switch (state) {
+ enum session_state ostate;
+
+ /* first apply new state */
+ ostate = peer->prev_state;
+ peer->prev_state = peer->state;
+ peer->state = state;
+
+ /* then act on it */
+ switch (peer->state) {
case STATE_IDLE:
/* carp demotion first. new peers handled in init_peer */
- if (peer->state == STATE_ESTABLISHED &&
+ if (peer->prev_state == STATE_ESTABLISHED &&
peer->conf.demote_group[0] && !peer->demoted)
session_demote(peer, +1);
@@ -1810,7 +1818,7 @@ change_state(struct peer *peer, enum ses
* try to write out what's buffered (maybe a notification),
* don't bother if it fails
*/
- if (peer->state >= STATE_OPENSENT &&
+ if (peer->prev_state >= STATE_OPENSENT &&
msgbuf_queuelen(peer->wbuf) > 0)
ibuf_write(peer->fd, peer->wbuf);
@@ -1835,7 +1843,7 @@ change_state(struct peer *peer, enum ses
memset(&peer->capa.peer, 0, sizeof(peer->capa.peer));
session_md5_reload(peer);
- if (peer->state == STATE_ESTABLISHED) {
+ if (peer->prev_state == STATE_ESTABLISHED) {
if (peer->capa.neg.grestart.restart == 2 &&
(event == EVNT_CON_CLOSED ||
event == EVNT_CON_FATAL ||
@@ -1864,15 +1872,15 @@ change_state(struct peer *peer, enum ses
peer->IdleHoldTime *= 2;
}
- if (peer->state == STATE_NONE ||
- peer->state == STATE_ESTABLISHED) {
+ if (peer->prev_state == STATE_NONE ||
+ peer->prev_state == STATE_ESTABLISHED) {
/* initialize capability negotiation structures */
memcpy(&peer->capa.ann, &peer->conf.capabilities,
sizeof(peer->capa.ann));
}
break;
case STATE_CONNECT:
- if (peer->state == STATE_ESTABLISHED &&
+ if (peer->prev_state == STATE_ESTABLISHED &&
peer->capa.neg.grestart.restart == 2) {
/* do the graceful restart dance */
session_graceful_restart(peer);
@@ -1907,10 +1915,6 @@ change_state(struct peer *peer, enum ses
break;
}
- log_statechange(peer, state, event);
-
- session_mrt_dump_state(peer, peer->state, state);
-
- peer->prev_state = peer->state;
- peer->state = state;
+ log_statechange(peer, ostate, event);
+ session_mrt_dump_state(peer);
}
bgpd: Adjust BGP FSM handling in change_state