From: Claudio Jeker Subject: Re: bgpd: convert control.c to new ibuf API To: Theo Buehler Cc: tech@openbsd.org Date: Wed, 10 Jan 2024 11:54:50 +0100 On Wed, Jan 10, 2024 at 11:06:32AM +0100, Theo Buehler wrote: > 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. Fixed the bad if () not sure why it was like this in the first place. I started to use unsigned long long in struct stats because of -portable. Stupid systems where %llu fails for uint64_t and requires a cast (or even worse PRIu64). > 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 */ > > > -- :wq Claudio