On 10/10/15(Sat) 20:02, Alexandr Nedvedicky wrote: > Hello, > > Patch fixes two small nits related to source node table in PF (a.k.a. > pf_src_tree_tracking). > > The first issue comes to `global` argument of pf_insert_src_node(). It is > always 0 everywhere in source code. The `global` is supposed to indicate > whether particular state is bound to global/main rule or to rule created on > behalf of admin. However in reality PF always uses 0 for `global` everywhere.
I'm ok with this with one nit below. > The second issue is related to pf_remove_src_node() function, which refuses > the remove source node from table (pf_src_tree_tracking) if node to be removed > is not bound to rule (sn->rule.ptr == NULL). Such node would hang in tree > forever. I think we never hit this problem, since source node is always > bound to rule. I don't know enough to say if it's correct or not, but I'd suggest sending another diff for that dealing with all the NULL checks :) What about pf_state_export() for example? > Index: pf.c > =================================================================== > RCS file: /cvs/src/sys/net/pf.c,v > retrieving revision 1.946 > diff -u -p -r1.946 pf.c > --- pf.c 8 Oct 2015 11:36:51 -0000 1.946 > +++ pf.c 10 Oct 2015 16:49:40 -0000 > @@ -531,10 +528,7 @@ pf_insert_src_node(struct pf_src_node ** > > (*sn)->type = type; > (*sn)->af = af; > - if (global) > - (*sn)->rule.ptr = NULL; > - else > - (*sn)->rule.ptr = rule; > + (*sn)->rule.ptr = rule; > PF_ACPY(&(*sn)->addr, src, af); > if (raddr) > PF_ACPY(&(*sn)->raddr, raddr, af); Here can't you also change: if ((*sn)->rule.ptr != NULL) (*sn)->rule.ptr->src_nodes++; into: (*sn)->rule.ptr->src_nodes++;