On Fri, Oct 27, 2023 at 01:06:31PM +0200, Claudio Jeker wrote:
> In the big ibuf API refactor I also broke the optparamlen handling
> by using one variable for two things.
>
> All the size handling in session_open() can be simplified since
> ibuf_size() is cheap to call.
>
> I think the result is cleaner than the code before. It is still somewhat
> funky because there are a fair amount of conditions to cover now.
There's no way to avoid catching some funk if the spec has a groovy
legacy heritage...
Diff makes sense and matches my understanding of that RFC's hack.
ok
>
> --
> :wq Claudio
>
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.452
> diff -u -p -r1.452 session.c
> --- session.c 27 Oct 2023 09:40:27 -0000 1.452
> +++ session.c 27 Oct 2023 11:00:13 -0000
> @@ -1471,8 +1471,9 @@ session_open(struct peer *p)
> {
> struct bgp_msg *buf;
> struct ibuf *opb;
> - uint16_t len, optparamlen = 0, holdtime;
> - uint8_t i, op_type;
> + size_t optparamlen;
> + uint16_t holdtime;
> + uint8_t i;
> int errs = 0, extlen = 0;
> int mpcapa = 0;
>
> @@ -1556,16 +1557,16 @@ session_open(struct peer *p)
> if (optparamlen == 0) {
> /* nothing */
> } else if (optparamlen + 2 >= 255) {
> - /* RFC9072: 2 byte length instead of 1 + 3 byte extra header */
> - optparamlen += sizeof(op_type) + 2 + 3;
> + /* RFC9072: use 255 as magic size and request extra header */
> optparamlen = 255;
> extlen = 1;
> } else {
> - optparamlen += sizeof(op_type) + 1;
> + /* regular capabilities header */
> + optparamlen += 2;
> }
>
> - len = MSGSIZE_OPEN_MIN + optparamlen;
> - if (errs || (buf = session_newmsg(OPEN, len)) == NULL) {
> + if (errs || (buf = session_newmsg(OPEN,
> + MSGSIZE_OPEN_MIN + optparamlen)) == NULL) {
> ibuf_free(opb);
> bgp_fsm(p, EVNT_CON_FATAL);
> return;
> @@ -1584,20 +1585,19 @@ session_open(struct peer *p)
> errs += ibuf_add_n8(buf->buf, optparamlen);
>
> if (extlen) {
> - /* write RFC9072 extra header */
> + /* RFC9072 extra header which spans over the capabilities hdr */
> errs += ibuf_add_n8(buf->buf, OPT_PARAM_EXT_LEN);
> - errs += ibuf_add_n16(buf->buf, optparamlen - 3);
> + errs += ibuf_add_n16(buf->buf, ibuf_size(opb) + 1 + 2);
> }
>
> if (optparamlen) {
> errs += ibuf_add_n8(buf->buf, OPT_PARAM_CAPABILITIES);
>
> - optparamlen = ibuf_size(opb);
> if (extlen) {
> /* RFC9072: 2-byte extended length */
> - errs += ibuf_add_n16(buf->buf, optparamlen);
> + errs += ibuf_add_n16(buf->buf, ibuf_size(opb));
> } else {
> - errs += ibuf_add_n8(buf->buf, optparamlen);
> + errs += ibuf_add_n8(buf->buf, ibuf_size(opb));
> }
> errs += ibuf_add_buf(buf->buf, opb);
> }
>
