On Mon, Oct 12, 2015 at 11:13 +0200, Martin Pieuchot wrote: > 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? >
I think you might be confusing state rule pointers and source node rule pointers. I think Sasha has got all of the latter ones (albeit with your correction), but on the other hand I would love to know how st->rule.ptr can be NULL in the pf_state_export as pf_rm_rule is not supposed to remove a rule with active states. > > 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++; > I agree.
