Download raw body.
bgpd: convert control.c to new ibuf API
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
bgpd: convert control.c to new ibuf API