On Thu, Oct 19, 2023 at 01:26:49PM +0200, Claudio Jeker wrote:
> On Thu, Oct 19, 2023 at 12:59:17PM +0200, Theo Buehler wrote:
> > 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.
>
> I like my yaks shaved like this...
I like this better than the errs dance, too.
ok tb
> The sizeof yak is still in queue... not sure about it.
It's not that horrible.
> --
> :wq Claudio
>
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 rtr_proto.c
> --- rtr_proto.c 19 Oct 2023 11:12:10 -0000 1.18
> +++ rtr_proto.c 19 Oct 2023 11:24:56 -0000
> @@ -233,6 +233,7 @@ rtr_newmsg(struct rtr_session *rs, enum
> uint16_t session_id)
> {
> struct ibuf *buf;
> + int saved_errno;
>
> if (len > RTR_MAX_LEN) {
> errno = ERANGE;
> @@ -240,15 +241,23 @@ rtr_newmsg(struct rtr_session *rs, enum
> }
> len += sizeof(struct rtr_header);
> if ((buf = ibuf_open(len)) == NULL)
> - return NULL;
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n8(buf, rs->version);
> - ibuf_add_n8(buf, type);
> - ibuf_add_n16(buf, session_id);
> - ibuf_add_n32(buf, len);
> + goto fail;
> + if (ibuf_add_n8(buf, rs->version) == -1)
> + goto fail;
> + if (ibuf_add_n8(buf, type) == -1)
> + goto fail;
> + if (ibuf_add_n16(buf, session_id) == -1)
> + goto fail;
> + if (ibuf_add_n32(buf, len) == -1)
> + goto fail;
>
> return buf;
> +
> + fail:
> + saved_errno = errno;
> + ibuf_free(buf);
> + errno = saved_errno;
> + return NULL;
> }
>
> /*
> @@ -271,22 +280,27 @@ rtr_send_error(struct rtr_session *rs, e
>
> 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));
> - return;
> - }
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n32(buf, len);
> - ibuf_add(buf, pdu, len);
> - ibuf_add_n32(buf, mlen);
> - ibuf_add(buf, msg, mlen);
> + if (buf == NULL)
> + goto fail;
> + if (ibuf_add_n32(buf, len) == -1)
> + goto fail;
> + if (ibuf_add(buf, pdu, len) == -1)
> + goto fail;
> + if (ibuf_add_n32(buf, mlen) == -1)
> + goto fail;
> + if (ibuf_add(buf, msg, mlen) == -1)
> + goto fail;
> ibuf_close(&rs->w, buf);
>
> log_warnx("rtr %s: sending error report[%u] %s", log_rtr(rs), err,
> msg ? msg : "");
>
> rtr_fsm(rs, RTR_EVNT_SEND_ERROR);
> + return;
> +
> + fail:
> + log_warn("rtr %s: send error report", log_rtr(rs));
> + ibuf_free(buf);
> }
>
> static void
> @@ -309,15 +323,17 @@ rtr_send_serial_query(struct rtr_session
> struct ibuf *buf;
>
> 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);
> - return;
> - }
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n32(buf, rs->serial);
> + if (buf == NULL)
> + goto fail;
> + if (ibuf_add_n32(buf, rs->serial) == -1)
> + goto fail;
> ibuf_close(&rs->w, buf);
> + return;
> +
> + fail:
> + log_warn("rtr %s: send serial query", log_rtr(rs));
> + ibuf_free(buf);
> + rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> }
>
> /*
