On Thu, Apr 20, 2023 at 06:23:45PM +0200, Claudio Jeker wrote:
> This currently only supports prefixes and numeric options.
> It does not handle TCP and fragment flags right now.
> Appart from that lists of options work.
ok, some small suggestions inline. Do it the way you like it better.
>
> --
> :wq Claudio
>
> Index: bgpctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.291
> diff -u -p -r1.291 bgpctl.c
> --- bgpctl.c 20 Apr 2023 14:01:50 -0000 1.291
> +++ bgpctl.c 20 Apr 2023 14:09:27 -0000
> @@ -57,6 +57,7 @@ void show_mrt_msg(struct mrt_bgp_msg *
> const char *msg_type(uint8_t);
> void network_bulk(struct parse_result *);
> int match_aspath(void *, uint16_t, struct filter_as *);
> +struct flowspec *res_to_flowspec(struct parse_result *);
>
> struct imsgbuf *ibuf;
> struct mrt_parser show_mrt = { show_mrt_dump, show_mrt_state, show_mrt_msg };
> @@ -85,6 +86,7 @@ main(int argc, char *argv[])
> struct parse_result *res;
> struct ctl_neighbor neighbor;
> struct ctl_show_rib_request ribreq;
> + struct flowspec *f;
> char *sockname;
> enum imsg_type type;
>
> @@ -363,6 +365,18 @@ main(int argc, char *argv[])
> break;
> case FLOWSPEC_ADD:
> case FLOWSPEC_REMOVE:
> + f = res_to_flowspec(res);
> + /* attribute sets are not supported */
> + if (res->action == FLOWSPEC_ADD) {
> + imsg_compose(ibuf, IMSG_FLOWSPEC_ADD, 0, 0, -1,
> + f, FLOWSPEC_SIZE + f->len);
> + send_filterset(ibuf, &res->set);
> + imsg_compose(ibuf, IMSG_FLOWSPEC_DONE, 0, 0, -1,
> + NULL, 0);
> + } else
> + imsg_compose(ibuf, IMSG_FLOWSPEC_REMOVE, 0, 0, -1,
> + f, FLOWSPEC_SIZE + f->len);
> + printf("request sent.\n");
> done = 1;
> break;
> case FLOWSPEC_FLUSH:
> @@ -1953,4 +1967,112 @@ match_aspath(void *data, uint16_t len, s
> }
> }
> return (0);
> +}
> +
> +static void
> +component_finish(int type, uint8_t *data, int len)
> +{
> + uint8_t *last;
> + int i = 0;
> +
> + switch (type) {
> + case FLOWSPEC_TYPE_DEST:
> + case FLOWSPEC_TYPE_SOURCE:
> + /* nothing todo */
> + return;
> + default:
> + break;
> + }
> +
I'd prefer initializing i = 0 here.
> + do {
> + last = data + i;
> + i += FLOWSPEC_OP_LEN(*last) + 1;
> + } while (i < len);
> + *last |= FLOWSPEC_OP_EOL;
> +}
> +
> +static void
> +push_prefix(struct parse_result *r, int type, struct bgpd_addr *addr,
> + uint8_t len)
> +{
> + void *data;
> + uint8_t *comp;
> + int complen, l = 0;
> +
> + switch (addr->aid) {
> + case AID_UNSPEC:
> + return;
> + case AID_INET:
> + complen = PREFIX_SIZE(len);
> + data = &addr->v4;
> + break;
> + case AID_INET6:
> + /* IPv6 includes an offset byte */
> + complen = PREFIX_SIZE(len) + 1;
> + data = &addr->v6;
> + break;
> + }
> + comp = malloc(complen);
> + if (comp == NULL)
> + err(1, NULL);
> +
Same here: I'd probably initialize l = 0 here
> + comp[l++] = len;
> + if (addr->aid == AID_INET6)
> + comp[l++] = 0;
> + memcpy(comp + l, data, PREFIX_SIZE(len) - 1);
and then do
> + memcpy(comp + l, data, complen - l);
> +
> + r->flow.complen[type] = complen;
> + r->flow.components[type] = comp;
> +}
