Download raw body.
bgpd: make imsg_rde void and fail hard
On Wed, Feb 26, 2025 at 05:52:13PM +0100, Claudio Jeker wrote:
> 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.
ok tb
>
> --
> :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);
bgpd: make imsg_rde void and fail hard