From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: factor filter_set imsg handling into own functions
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 3 Dec 2025 11:14:41 +0100

On Wed, Dec 03, 2025 at 11:10:32AM +0100, Theo Buehler wrote:
> On Wed, Dec 03, 2025 at 10:48:14AM +0100, Claudio Jeker wrote:
> > The handling of filter_set in the RDE is not optimal. It is time to
> > optimise rde_apply_set() and this is the first step.
> > 
> > Having filterset_send and recv will allow to use a different struct inside
> > the RDE and drop a lot of fat from that struct.
> > This is the first trivial step.
> 
> ok, some grumbling below.
> 
> > Index: rde_filter.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
> > diff -u -p -r1.136 rde_filter.c
> > --- rde_filter.c	9 May 2023 13:11:19 -0000	1.136
> > +++ rde_filter.c	3 Dec 2025 09:35:20 -0000
> > @@ -743,6 +743,37 @@ filterset_name(enum action_types type)
> >  	fatalx("filterset_name: got lost");
> >  }
> >  
> > +int
> > +filterset_send(struct imsgbuf *i, struct filter_set_head *set)
> 
> I understand that this just moves code around, but i really isn't my
> preferred name for a struct imsgbuf. Not sure I like the ibuf* in the
> callers better, though :)

Yes, i is not great. Will switch to imsgbuf as name.
 
> > +{
> > +	struct filter_set	*s;
> > +
> > +	TAILQ_FOREACH(s, set, entry)
> > +		if (imsg_compose(i, IMSG_FILTER_SET, 0, 0, -1, s,
> > +		    sizeof(struct filter_set)) == -1)
> 
> can this use sizeof(*s) like in filterset_recv?

Sure, I missed that one.
 
> > +			return (-1);
> > +	return (0);
> > +}
> > +
> > +void
> > +filterset_recv(struct imsg *imsg, struct filter_set_head *set)
> > +{
> > +	struct filter_set	*s;
> > +
> > +	if ((s = malloc(sizeof(*s))) == NULL)
> > +		fatal(NULL);
> > +	if (imsg_get_data(imsg, s, sizeof(*s)) == -1) {
> > +		log_warnx("rde_dispatch: wrong imsg len");
> > +		free(s);
> > +		return;
> > +	}
> > +	if (s->type == ACTION_SET_NEXTHOP) {
> > +		s->action.nh_ref = nexthop_get(&s->action.nexthop);
> > +		s->type = ACTION_SET_NEXTHOP_REF;
> > +	}
> > +	TAILQ_INSERT_TAIL(set, s, entry);
> > +}
> > +
> >  /*
> >   * Copyright (c) 2001 Daniel Hartmeier
> >   * All rights reserved.
> > 
> 

-- 
:wq Claudio