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++;
