On Tue, Apr 18, 2023 at 12:52:00PM +0200, Theo Buehler wrote:
> On Tue, Apr 18, 2023 at 11:29:26AM +0200, Claudio Jeker wrote:
> > This diff adds the parse.y and config.c bits for flowspec.
> > I tried to make flowspec rules as similar to pf rules (even though
> > flowspec is more flexible).
> >
> > Now this diff does nothing in itself but is already large enough to not
> > add more to it. In parse.y the individual flowspec components are built up
> > in a flowspec context and then at the end converted into a struct flowspec
> > object. I wrapped that into a struct flowspec_config so that all the
> > parent config bits can be kept together. (For struct network this is
> > currently the other way around but I plan to change this at a later
> > point).
>
> ok tb
>
> Couple of nits and comments inline. Apart from two copy-paste errors
> (s/4/6) nothing important
>
> > +flowspec_alloc(uint8_t aid, int len)
> > +{
> > + struct flowspec_config *conf;
> > + struct flowspec *flow;
> > +
> > + flow = malloc(FLOWSPEC_SIZE + len);
> > + if (flow == NULL)
> > + return NULL;
> > + memset(flow, 0, FLOWSPEC_SIZE);
>
> I assume not zeroing len bytes is worth this extra dance as opposed
> to using calloc().
I went back and for between the two versions. I want to use zero the start
of struct flowspec because of the pad but it feels wasteful to zero
everything and then set all but 1 byte.
> > + /*
> > + * merge the flowspec statements, but first mark the old ones
> > + * for deletion. Which happens when the flowspec is sent to the RDE.
> > + */
>
> This comment is a bit awkward. Maybe split the sentence differently like this:
>
> /*
> * Merge the flowspec statements. Mark the old ones for deletion
> * which happens when the flowspec is sent to the RDE.
> */
Thanks, I used your version.
> > +parse_flags(char *s)
> > +{
> > + const char *flags = FLOWSPEC_TCP_FLAG_STRING;
> > + char *p, *q;
> > + uint8_t f = 0;
> > +
> > + if (curflow->type == FLOWSPEC_TYPE_FRAG) {
> > + if (curflow->aid == AID_INET)
> > + flags = FLOWSPEC_FRAG_STRING4;
> > + else
> > + flags = FLOWSPEC_FRAG_STRING4;
>
> s/4/6
>
> flags = FLOWSPEC_FRAG_STRING6;
Thanks for finding this and the other 6 vs 4 copy paste errors
> > +static int
> > +geticmptypebyname(char *w, uint8_t aid)
> > +{
> > + unsigned int i;
> > +
> > + switch (aid) {
> > + case AID_INET:
> > + for (i=0; i < (sizeof (icmp_type) / sizeof(icmp_type[0]));
>
> No space after sizeof, I'd drop the extra parens. Might also be worth it
> to add a local version of nitems() to this file.
This and the others are IIRC streight from pfctl. So if someone wants a
free commit :)
--
:wq Claudio
