Download raw body.
bgpd: convert control.c to new ibuf API
On Tue, Jan 09, 2024 at 04:56:56PM +0100, Claudio Jeker wrote:
> This diff converts the control.c code to use the new ibuf API.
> This uses ibuf_forward() to send messages from the control socket to the
> RDE or parent process.
Now is probably not a good time, but it would be nice to rename c->ibuf into
c->imsgbuf at some point. It's very confusing.
> The handling of the optional neighbor struct makes this code bit more complex.
Indeed. It looks good and seems to preserve existing behavior. Again
the result is generally more pleasant on the eyes.
I only have one extremely important nit inline. Also not terribly
important, but I dislike the (unsigned) long long vs uint64_t
inconsistency in various struct stats.
ok tb
> --
> :wq Claudio
>
> Index: control.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
> diff -u -p -r1.114 control.c
> --- control.c 7 Nov 2023 11:18:35 -0000 1.114
> +++ control.c 9 Jan 2024 15:51:58 -0000
> @@ -219,7 +219,7 @@ int
> control_close(struct ctl_conn *c)
> {
> if (c->terminate && c->ibuf.pid)
> - imsg_ctl_rde(IMSG_CTL_TERMINATE, 0, c->ibuf.pid, NULL, 0);
> + imsg_ctl_rde_msg(IMSG_CTL_TERMINATE, 0, c->ibuf.pid);
>
> msgbuf_clear(&c->ibuf.w);
> TAILQ_REMOVE(&ctl_conns, c, entry);
> @@ -234,12 +234,14 @@ int
> control_dispatch_msg(struct pollfd *pfd, struct peer_head *peers)
> {
> struct imsg imsg;
> + struct ctl_neighbor neighbor;
> + struct ctl_show_rib_request ribreq;
> struct ctl_conn *c;
> + struct peer *p;
> ssize_t n;
> + uint32_t type;
> + pid_t pid;
> int verbose, matched;
> - struct peer *p;
> - struct ctl_neighbor *neighbor;
> - struct ctl_show_rib_request *ribreq;
>
> if ((c = control_connbyfd(pfd->fd)) == NULL) {
> log_warn("control_dispatch_msg: fd %d: not found", pfd->fd);
> @@ -250,8 +252,7 @@ control_dispatch_msg(struct pollfd *pfd,
> if (msgbuf_write(&c->ibuf.w) <= 0 && errno != EAGAIN)
> return control_close(c);
> if (c->throttled && c->ibuf.w.queued < CTL_MSG_LOW_MARK) {
> - if (imsg_ctl_rde(IMSG_XON, 0, c->ibuf.pid, NULL, 0) !=
> - -1)
> + if (imsg_ctl_rde_msg(IMSG_XON, 0, c->ibuf.pid) != -1)
> c->throttled = 0;
> }
> }
> @@ -270,8 +271,10 @@ control_dispatch_msg(struct pollfd *pfd,
> if (n == 0)
> break;
>
> + type = imsg_get_type(&imsg);
> + pid = imsg_get_pid(&imsg);
> if (c->restricted) {
> - switch (imsg.hdr.type) {
> + switch (type) {
> case IMSG_CTL_SHOW_NEIGHBOR:
> case IMSG_CTL_SHOW_NEXTHOP:
> case IMSG_CTL_SHOW_INTERFACE:
> @@ -287,20 +290,25 @@ control_dispatch_msg(struct pollfd *pfd,
> break;
> default:
> /* clear imsg type to prevent processing */
> - imsg.hdr.type = IMSG_NONE;
> + type = IMSG_NONE;
> control_result(c, CTL_RES_DENIED);
> break;
> }
> }
>
> - switch (imsg.hdr.type) {
> + /*
> + * TODO: this is wrong and shoud work the other way around.
> + * The imsg.hdr.pid is from the remote end and should not
> + * be trusted.
> + */
> + c->ibuf.pid = pid;
> + switch (type) {
> case IMSG_NONE:
> /* message was filtered out, nothing to do */
> break;
> case IMSG_CTL_FIB_COUPLE:
> case IMSG_CTL_FIB_DECOUPLE:
> - imsg_ctl_parent(imsg.hdr.type, imsg.hdr.peerid,
> - 0, NULL, 0);
> + imsg_ctl_parent(&imsg);
> break;
> case IMSG_CTL_SHOW_TERSE:
> RB_FOREACH(p, peer_head, peers)
> @@ -309,23 +317,19 @@ control_dispatch_msg(struct pollfd *pfd,
> imsg_compose(&c->ibuf, IMSG_CTL_END, 0, 0, -1, NULL, 0);
> break;
> case IMSG_CTL_SHOW_NEIGHBOR:
> - c->ibuf.pid = imsg.hdr.pid;
> + if (imsg_get_data(&imsg, &neighbor,
> + sizeof(neighbor)) == -1)
> + memset(&neighbor, 0, sizeof(neighbor));
>
> - if (imsg.hdr.len == IMSG_HEADER_SIZE +
> - sizeof(struct ctl_neighbor)) {
> - neighbor = imsg.data;
> - } else {
> - neighbor = NULL;
> - }
> matched = 0;
> RB_FOREACH(p, peer_head, peers) {
> - if (!peer_matched(p, neighbor))
> + if (!peer_matched(p, &neighbor))
> continue;
>
> matched = 1;
> - if (!neighbor || !neighbor->show_timers) {
> - imsg_ctl_rde(imsg.hdr.type, p->conf.id,
> - imsg.hdr.pid, NULL, 0);
> + if (!neighbor.show_timers) {
> + imsg_ctl_rde_msg(type,
> + p->conf.id, pid);
> } else {
> u_int i;
> time_t d;
> @@ -348,9 +352,8 @@ control_dispatch_msg(struct pollfd *pfd,
> }
> if (!matched && RB_EMPTY(peers)) {
> control_result(c, CTL_RES_NOSUCHPEER);
> - } else if (!neighbor || !neighbor->show_timers) {
> - imsg_ctl_rde(IMSG_CTL_END, 0, imsg.hdr.pid,
> - NULL, 0);
> + } else if (!neighbor.show_timers) {
> + imsg_ctl_rde_msg(IMSG_CTL_END, 0, pid);
> } else {
> imsg_compose(&c->ibuf, IMSG_CTL_END, 0, 0, -1,
> NULL, 0);
> @@ -361,23 +364,21 @@ control_dispatch_msg(struct pollfd *pfd,
> case IMSG_CTL_NEIGHBOR_CLEAR:
> case IMSG_CTL_NEIGHBOR_RREFRESH:
> case IMSG_CTL_NEIGHBOR_DESTROY:
> - if (imsg.hdr.len != IMSG_HEADER_SIZE +
> - sizeof(struct ctl_neighbor)) {
> + if (imsg_get_data(&imsg, &neighbor,
> + sizeof(neighbor)) == -1) {
> log_warnx("got IMSG_CTL_NEIGHBOR_ with "
> "wrong length");
> break;
> }
>
> - neighbor = imsg.data;
> -
> matched = 0;
> RB_FOREACH(p, peer_head, peers) {
> - if (!peer_matched(p, neighbor))
> + if (!peer_matched(p, &neighbor))
> continue;
>
> matched = 1;
>
> - switch (imsg.hdr.type) {
> + switch (type) {
> case IMSG_CTL_NEIGHBOR_UP:
> bgp_fsm(p, EVNT_START);
> p->conf.down = 0;
> @@ -388,22 +389,20 @@ control_dispatch_msg(struct pollfd *pfd,
> control_result(c, CTL_RES_OK);
> break;
> case IMSG_CTL_NEIGHBOR_DOWN:
> - neighbor->reason[
> - sizeof(neighbor->reason) - 1] =
> - '\0';
> + neighbor.reason[
> + sizeof(neighbor.reason) - 1] = '\0';
> strlcpy(p->conf.reason,
> - neighbor->reason,
> + neighbor.reason,
> sizeof(p->conf.reason));
> p->conf.down = 1;
> session_stop(p, ERR_CEASE_ADMIN_DOWN);
> control_result(c, CTL_RES_OK);
> break;
> case IMSG_CTL_NEIGHBOR_CLEAR:
> - neighbor->reason[
> - sizeof(neighbor->reason) - 1] =
> - '\0';
> + neighbor.reason[
> + sizeof(neighbor.reason) - 1] = '\0';
> strlcpy(p->conf.reason,
> - neighbor->reason,
> + neighbor.reason,
> sizeof(p->conf.reason));
> p->IdleHoldTime =
> INTERVAL_IDLE_HOLD_INITIAL;
> @@ -455,51 +454,40 @@ control_dispatch_msg(struct pollfd *pfd,
> case IMSG_CTL_SHOW_INTERFACE:
> case IMSG_CTL_SHOW_FIB_TABLES:
> case IMSG_CTL_SHOW_RTR:
> - c->ibuf.pid = imsg.hdr.pid;
> - imsg_ctl_parent(imsg.hdr.type, 0, imsg.hdr.pid,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> + imsg_ctl_parent(&imsg);
> break;
> case IMSG_CTL_KROUTE:
> case IMSG_CTL_KROUTE_ADDR:
> case IMSG_CTL_SHOW_NEXTHOP:
> - c->ibuf.pid = imsg.hdr.pid;
> - imsg_ctl_parent(imsg.hdr.type, imsg.hdr.peerid,
> - imsg.hdr.pid, imsg.data, imsg.hdr.len -
> - IMSG_HEADER_SIZE);
> + imsg_ctl_parent(&imsg);
> break;
> case IMSG_CTL_SHOW_RIB:
> case IMSG_CTL_SHOW_RIB_PREFIX:
> - if (imsg.hdr.len != IMSG_HEADER_SIZE +
> - sizeof(struct ctl_show_rib_request)) {
> + if (imsg_get_data(&imsg, &ribreq, sizeof(ribreq)) ==
> + -1) {
> log_warnx("got IMSG_CTL_SHOW_RIB with "
> "wrong length");
> break;
> }
>
> - ribreq = imsg.data;
> - neighbor = &ribreq->neighbor;
> -
> /* check if at least one neighbor exists */
> RB_FOREACH(p, peer_head, peers)
> - if (peer_matched(p, neighbor))
> + if (peer_matched(p, &ribreq.neighbor))
> break;
> if (p == NULL && RB_EMPTY(peers)) {
> control_result(c, CTL_RES_NOSUCHPEER);
> break;
> }
>
> - if ((imsg.hdr.type == IMSG_CTL_SHOW_RIB_PREFIX)
> - && (ribreq->prefix.aid == AID_UNSPEC)) {
> + if ((type == IMSG_CTL_SHOW_RIB_PREFIX)
> + && (ribreq.prefix.aid == AID_UNSPEC)) {
Too many parentheses and the && seems better placed on the first line.
> /* malformed request, must specify af */
> control_result(c, CTL_RES_PARSE_ERROR);
> break;
> }
>
> - c->ibuf.pid = imsg.hdr.pid;
> c->terminate = 1;
> -
> - imsg_ctl_rde(imsg.hdr.type, 0, imsg.hdr.pid,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> + imsg_ctl_rde(&imsg);
> break;
> case IMSG_CTL_SHOW_NETWORK:
> case IMSG_CTL_SHOW_FLOWSPEC:
> @@ -507,9 +495,7 @@ control_dispatch_msg(struct pollfd *pfd,
> /* FALLTHROUGH */
> case IMSG_CTL_SHOW_RIB_MEM:
> case IMSG_CTL_SHOW_SET:
> - c->ibuf.pid = imsg.hdr.pid;
> - imsg_ctl_rde(imsg.hdr.type, 0, imsg.hdr.pid,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> + imsg_ctl_rde(&imsg);
> break;
> case IMSG_NETWORK_ADD:
> case IMSG_NETWORK_ASPATH:
> @@ -522,21 +508,16 @@ control_dispatch_msg(struct pollfd *pfd,
> case IMSG_FLOWSPEC_DONE:
> case IMSG_FLOWSPEC_FLUSH:
> case IMSG_FILTER_SET:
> - imsg_ctl_rde(imsg.hdr.type, 0, 0,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> + imsg_ctl_rde(&imsg);
> break;
> case IMSG_CTL_LOG_VERBOSE:
> - if (imsg.hdr.len != IMSG_HEADER_SIZE +
> - sizeof(verbose))
> + if (imsg_get_data(&imsg, &verbose, sizeof(verbose)) ==
> + -1)
> break;
>
> /* forward to other processes */
> - imsg_ctl_parent(imsg.hdr.type, 0, imsg.hdr.pid,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> - imsg_ctl_rde(imsg.hdr.type, 0, imsg.hdr.pid,
> - imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE);
> -
> - memcpy(&verbose, imsg.data, sizeof(verbose));
> + imsg_ctl_parent(&imsg);
> + imsg_ctl_rde(&imsg);
> log_setverbose(verbose);
> break;
> default:
> @@ -552,23 +533,28 @@ int
> control_imsg_relay(struct imsg *imsg, struct peer *p)
> {
> struct ctl_conn *c;
> + uint32_t type;
> + pid_t pid;
>
> - if ((c = control_connbypid(imsg->hdr.pid)) == NULL)
> + type = imsg_get_type(imsg);
> + pid = imsg_get_pid(imsg);
> +
> + if ((c = control_connbypid(pid)) == NULL)
> return (0);
>
> /* special handling for peers since only the stats are sent from RDE */
> - if (imsg->hdr.type == IMSG_CTL_SHOW_NEIGHBOR) {
> + if (type == IMSG_CTL_SHOW_NEIGHBOR) {
> struct rde_peer_stats stats;
>
> - if (imsg->hdr.len > IMSG_HEADER_SIZE + sizeof(stats)) {
> - log_warnx("wrong imsg len");
> + if (p == NULL) {
> + log_warnx("%s: no such peer: id=%u", __func__,
> + imsg_get_id(imsg));
> return (0);
> }
> - if (p == NULL) {
> - log_warnx("no such peer: id=%u", imsg->hdr.peerid);
> + if (imsg_get_data(imsg, &stats, sizeof(stats)) == -1) {
> + log_warnx("%s: imsg_get_data", __func__);
> return (0);
> }
> - memcpy(&stats, imsg->data, sizeof(stats));
> p->stats.prefix_cnt = stats.prefix_cnt;
> p->stats.prefix_out_cnt = stats.prefix_out_cnt;
> p->stats.prefix_rcvd_update = stats.prefix_rcvd_update;
> @@ -580,21 +566,19 @@ control_imsg_relay(struct imsg *imsg, st
> p->stats.pending_update = stats.pending_update;
> p->stats.pending_withdraw = stats.pending_withdraw;
>
> - return (imsg_compose(&c->ibuf, imsg->hdr.type, 0,
> - imsg->hdr.pid, -1, p, sizeof(*p)));
> + return imsg_compose(&c->ibuf, type, 0, pid, -1, p, sizeof(*p));
> }
>
> /* if command finished no need to send exit message */
> - if (imsg->hdr.type == IMSG_CTL_END || imsg->hdr.type == IMSG_CTL_RESULT)
> + if (type == IMSG_CTL_END || type == IMSG_CTL_RESULT)
> c->terminate = 0;
>
> if (!c->throttled && c->ibuf.w.queued > CTL_MSG_HIGH_MARK) {
> - if (imsg_ctl_rde(IMSG_XOFF, 0, imsg->hdr.pid, NULL, 0) != -1)
> + if (imsg_ctl_rde_msg(IMSG_XOFF, 0, pid) != -1)
> c->throttled = 1;
> }
>
> - return (imsg_compose(&c->ibuf, imsg->hdr.type, 0, imsg->hdr.pid, -1,
> - imsg->data, imsg->hdr.len - IMSG_HEADER_SIZE));
> + return (imsg_forward(&c->ibuf, imsg));
> }
>
> void
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.456 session.c
> --- session.c 14 Dec 2023 13:52:38 -0000 1.456
> +++ session.c 9 Jan 2024 15:45:00 -0000
> @@ -3576,14 +3576,25 @@ session_up(struct peer *p)
> }
>
> int
> -imsg_ctl_parent(int type, uint32_t peerid, pid_t pid, void *data,
> - uint16_t datalen)
> +imsg_ctl_parent(struct imsg *imsg)
> {
> - return imsg_compose(ibuf_main, type, peerid, pid, -1, data, datalen);
> + return imsg_forward(ibuf_main, imsg);
> }
>
> int
> -imsg_ctl_rde(int type, uint32_t peerid, pid_t pid, void *data, uint16_t datalen)
> +imsg_ctl_rde(struct imsg *imsg)
> +{
> + if (ibuf_rde_ctl == NULL)
> + return (0);
> + /*
> + * Use control socket to talk to RDE to bypass the queue of the
> + * regular imsg socket.
> + */
> + return imsg_forward(ibuf_rde_ctl, imsg);
> +}
> +
> +int
> +imsg_ctl_rde_msg(int type, uint32_t peerid, pid_t pid)
> {
> if (ibuf_rde_ctl == NULL)
> return (0);
> @@ -3592,7 +3603,7 @@ imsg_ctl_rde(int type, uint32_t peerid,
> * Use control socket to talk to RDE to bypass the queue of the
> * regular imsg socket.
> */
> - return imsg_compose(ibuf_rde_ctl, type, peerid, pid, -1, data, datalen);
> + return imsg_compose(ibuf_rde_ctl, type, peerid, pid, -1, NULL, 0);
> }
>
> int
> Index: session.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
> diff -u -p -r1.164 session.h
> --- session.h 19 Oct 2023 07:02:46 -0000 1.164
> +++ session.h 9 Jan 2024 15:44:04 -0000
> @@ -339,8 +339,9 @@ struct peer *getpeerbydesc(struct bgpd_c
> struct peer *getpeerbyip(struct bgpd_config *, struct sockaddr *);
> struct peer *getpeerbyid(struct bgpd_config *, uint32_t);
> int peer_matched(struct peer *, struct ctl_neighbor *);
> -int imsg_ctl_parent(int, uint32_t, pid_t, void *, uint16_t);
> -int imsg_ctl_rde(int, uint32_t, pid_t, void *, uint16_t);
> +int imsg_ctl_parent(struct imsg *);
> +int imsg_ctl_rde(struct imsg *);
> +int imsg_ctl_rde_msg(int, uint32_t, pid_t);
> void session_stop(struct peer *, uint8_t);
>
> /* timer.c */
>
bgpd: convert control.c to new ibuf API