On Thu, Oct 19, 2023 at 10:41:07AM +0200, Claudio Jeker wrote:
> More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
> the new functions.
>
> Two bits I'm unsure about:
> - I had to change some sizeof() to use native types (I especially dislike
> the sizeof(struct rtr_header).
Yes, that's not very nice.
> - ibuf_add_nXX() can fail if the value is too large. Which should be
> impossible but still maybe it would be better to check for errors.
While it should be impossible, the length calculations are non-trivial,
so it seems wiser to check.
It's a bit longer than what you have now, but maybe it's an option
to combine the length calculation with the errs += idiom.
len += sizeof(rs->version);
len += sizeof(type);
len += sizeof(session_id);
len += sizeof(len);
if ((buf = ibuf_open(len)) == NULL)
return NULL;
errs += ibuf_add_n8(buf, rs->version);
errs += ibuf_add_n8(buf, type);
errs += ibuf_add_n16(buf, session_id);
errs += ibuf_add_n32(buf, len);
if (errs) {
ibuf_free(ibuf);
return NULL;
}
I'm ok with the diff as it is and you can ponder how you want to shave
this particular Yak.
>
> --
> :wq Claudio
>
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 rtr_proto.c
> --- rtr_proto.c 16 Aug 2023 08:26:35 -0000 1.17
> +++ rtr_proto.c 19 Oct 2023 08:35:49 -0000
> @@ -233,24 +233,21 @@ rtr_newmsg(struct rtr_session *rs, enum
> uint16_t session_id)
> {
> struct ibuf *buf;
> - struct rtr_header rh;
>
> if (len > RTR_MAX_LEN) {
> errno = ERANGE;
> return NULL;
> }
> - len += sizeof(rh);
> + len += sizeof(struct rtr_header);
> if ((buf = ibuf_open(len)) == NULL)
> return NULL;
>
> - memset(&rh, 0, sizeof(rh));
> - rh.version = rs->version;
> - rh.type = type;
> - rh.session_id = htons(session_id);
> - rh.length = htonl(len);
> -
> /* cannot fail with fixed buffers */
> - ibuf_add(buf, &rh, sizeof(rh));
> + ibuf_add_n8(buf, rs->version);
> + ibuf_add_n8(buf, type);
> + ibuf_add_n16(buf, session_id);
> + ibuf_add_n32(buf, len);
> +
> return buf;
> }
>
> @@ -264,7 +261,6 @@ rtr_send_error(struct rtr_session *rs, e
> {
> struct ibuf *buf;
> size_t mlen = 0;
> - uint32_t hdrlen;
>
> rs->last_sent_error = err;
> if (msg) {
> @@ -273,7 +269,7 @@ rtr_send_error(struct rtr_session *rs, e
> } else
> memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
>
> - buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(hdrlen) + len + mlen,
> + buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
> err);
> if (buf == NULL) {
> log_warn("rtr %s: send error report", log_rtr(rs));
> @@ -281,11 +277,9 @@ rtr_send_error(struct rtr_session *rs, e
> }
>
> /* cannot fail with fixed buffers */
> - hdrlen = ntohl(len);
> - ibuf_add(buf, &hdrlen, sizeof(hdrlen));
> + ibuf_add_n32(buf, len);
> ibuf_add(buf, pdu, len);
> - hdrlen = ntohl(mlen);
> - ibuf_add(buf, &hdrlen, sizeof(hdrlen));
> + ibuf_add_n32(buf, mlen);
> ibuf_add(buf, msg, mlen);
> ibuf_close(&rs->w, buf);
>
> @@ -313,9 +307,8 @@ static void
> rtr_send_serial_query(struct rtr_session *rs)
> {
> struct ibuf *buf;
> - uint32_t s;
>
> - buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(s), rs->session_id);
> + buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
> if (buf == NULL) {
> log_warn("rtr %s: send serial query", log_rtr(rs));
> rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> @@ -323,8 +316,7 @@ rtr_send_serial_query(struct rtr_session
> }
>
> /* cannot fail with fixed buffers */
> - s = htonl(rs->serial);
> - ibuf_add(buf, &s, sizeof(s));
> + ibuf_add_n32(buf, rs->serial);
> ibuf_close(&rs->w, buf);
> }
>
>
