On Wed, Feb 26, 2025 at 05:44:44PM +0100, Theo Buehler wrote:
> On Wed, Feb 26, 2025 at 05:37:42PM +0100, Claudio Jeker wrote:
> > On Wed, Feb 26, 2025 at 05:30:38PM +0100, Theo Buehler wrote:
> > > On Wed, Feb 26, 2025 at 05:18:58PM +0100, Claudio Jeker wrote:
> > > > While working on all the other bits in the session engine I came to the
> > > > conclusion that if imsg_rde() fails the game is over. So just admit to
> > > > that and fatal in that case.
> > > > 
> > > > This simplifies all the callers since there is no need to check the return
> > > > value. This also allows some other functions to be come void.
> > > 
> > > ok
> > > 
> > > Should session_handle_{update,rrefresh}() also become void? Wouldn't
> > > it be more consistent if the return (0) was pushed to parse_update()
> > > and parse_rrefresh()?
> > 
> > Maybe we should make them all void including parse_update() and
> > parse_rrefresh(). The return value of parse_rrefresh is not checked and
> > the code around parse_update() is most probably wrong for the error case. 
> > In the end when a parse error happens we need to generate a notification
> > and properly throw an error in the FSM but there is no need to bubble up
> > the error.
> 
> Right, that makes sense.

That would look like this. 

-- 
:wq Claudio

Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.523 session.c
--- session.c	26 Feb 2025 16:39:18 -0000	1.523
+++ session.c	26 Feb 2025 16:50:36 -0000
@@ -1016,19 +1016,17 @@ get_alternate_addr(struct bgpd_addr *loc
 	freeifaddrs(ifap);
 }
 
-int
+void
 session_handle_update(struct peer *peer, struct ibuf *msg)
 {
 	/* pass the message verbatim to the rde. */
 	imsg_rde(IMSG_UPDATE, peer->conf.id, ibuf_data(msg), ibuf_size(msg));
-	return (0);
 }
 
-int
+void
 session_handle_rrefresh(struct peer *peer, struct route_refresh *rr)
 {
 	imsg_rde(IMSG_REFRESH, peer->conf.id, rr, sizeof(*rr));
-	return (0);
 }
 
 void
Index: session.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
diff -u -p -r1.190 session.h
--- session.h	26 Feb 2025 16:39:18 -0000	1.190
+++ session.h	26 Feb 2025 16:50:36 -0000
@@ -335,8 +335,8 @@ void		 get_alternate_addr(struct bgpd_ad
 struct peer	*getpeerbydesc(struct bgpd_config *, const char *);
 struct peer	*getpeerbyip(struct bgpd_config *, struct sockaddr *);
 struct peer	*getpeerbyid(struct bgpd_config *, uint32_t);
-int		 session_handle_update(struct peer *, struct ibuf *);
-int		 session_handle_rrefresh(struct peer *, struct route_refresh *);
+void		 session_handle_update(struct peer *, struct ibuf *);
+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,
Index: session_bgp.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v
diff -u -p -r1.2 session_bgp.c
--- session_bgp.c	26 Feb 2025 16:39:18 -0000	1.2
+++ session_bgp.c	26 Feb 2025 16:50:36 -0000
@@ -1079,13 +1079,13 @@ parse_open(struct peer *peer, struct ibu
 	return (0);
 }
 
-static int
+static void
 parse_update(struct peer *peer, struct ibuf *msg)
 {
-	return session_handle_update(peer, msg);
+	session_handle_update(peer, msg);
 }
 
-static int
+static void
 parse_rrefresh(struct peer *peer, struct ibuf *msg)
 {
 	struct route_refresh rr;
@@ -1114,7 +1114,7 @@ parse_rrefresh(struct peer *peer, struct
 				session_notification_data(peer, ERR_HEADER,
 				    ERR_HDR_LEN, &datalen, sizeof(datalen));
 				bgp_fsm(peer, EVNT_CON_FATAL, NULL);
-				return (-1);
+				return;
 			}
 			peer->stats.refresh_rcvd_req++;
 			break;
@@ -1129,7 +1129,7 @@ parse_rrefresh(struct peer *peer, struct
 				session_notification(peer, ERR_RREFRESH,
 				    ERR_RR_INV_LEN, msg);
 				bgp_fsm(peer, EVNT_CON_FATAL, NULL);
-				return (-1);
+				return;
 			}
 			if (subtype == ROUTE_REFRESH_BEGIN_RR)
 				peer->stats.refresh_rcvd_borr++;
@@ -1139,7 +1139,7 @@ parse_rrefresh(struct peer *peer, struct
 		default:
 			log_peer_warnx(&peer->conf, "peer sent bad refresh, "
 			    "bad subtype %d", subtype);
-			return (0);
+			return;
 		}
 	} else {
 		/* force subtype to default */
@@ -1151,18 +1151,18 @@ parse_rrefresh(struct peer *peer, struct
 	if (afi2aid(afi, safi, &aid) == -1) {
 		log_peer_warnx(&peer->conf, "peer sent bad refresh, "
 		    "invalid afi/safi pair");
-		return (0);
+		return;
 	}
 
 	if (!peer->capa.neg.refresh && !peer->capa.neg.enhanced_rr) {
 		log_peer_warnx(&peer->conf, "peer sent unexpected refresh");
-		return (0);
+		return;
 	}
 
 	rr.aid = aid;
 	rr.subtype = subtype;
 
-	return session_handle_rrefresh(peer, &rr);
+	session_handle_rrefresh(peer, &rr);
 }
 
 static void
@@ -1757,10 +1757,7 @@ bgp_fsm(struct peer *peer, enum session_
 			break;
 		case EVNT_RCVD_UPDATE:
 			start_timer_holdtime(peer);
-			if (parse_update(peer, msg))
-				change_state(peer, STATE_IDLE, event);
-			else
-				start_timer_holdtime(peer);
+			parse_update(peer, msg);
 			break;
 		case EVNT_RCVD_NOTIFICATION:
 			parse_notification(peer, msg);