Hello,
I don't object. diff improces things. Just few bike shedding nits
further below.
On Mon, Dec 19, 2022 at 04:48:57PM +1000, David Gwynne wrote:
</snip>
>
> i hope i didn't mess up the (sk_)states list traversals.
could not spot anything wrong there.
</snip>
>
> ok?
>
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1157
> diff -u -p -r1.1157 pf.c
> --- pf.c 16 Dec 2022 02:05:44 -0000 1.1157
> +++ pf.c 19 Dec 2022 06:39:45 -0000
> @@ -315,7 +315,7 @@ struct pf_state_tree_id tree_id;
> struct pf_state_list pf_state_list =
> PF_STATE_LIST_INITIALIZER(pf_state_list);
>
> RB_GENERATE(pf_src_tree, pf_src_node, entry, pf_src_compare);
> -RB_GENERATE(pf_state_tree, pf_state_key, entry, pf_state_compare_key);
> +RB_GENERATE(pf_state_tree, pf_state_key, sk_entry, pf_state_compare_key);
> RB_GENERATE(pf_state_tree_id, pf_state,
> entry_id, pf_state_compare_id);
I think pf_state_tree prototype should be moved to net/pfvar_priv.h perhaps
others should be moved as well, because those trees are private to pf(4).
can be done in follow up commit.
>
> @@ -736,24 +736,25 @@ pf_state_key_attach(struct pf_state_key
> PF_ASSERT_LOCKED();
>
> KASSERT(s->key[idx] == NULL);
> - sk->removed = 0;
> + sk->sk_removed = 0;
> cur = RB_INSERT(pf_state_tree, &pf_statetbl, sk);
> if (cur != NULL) {
> - sk->removed = 1;
> + sk->sk_removed = 1;
> /* key exists. check for same kif, if none, add to key */
> - TAILQ_FOREACH(si, &cur->states, entry) {
> - if (si->s->kif == s->kif &&
> - ((si->s->key[PF_SK_WIRE]->af == sk->af &&
> - si->s->direction == s->direction) ||
> - (si->s->key[PF_SK_WIRE]->af !=
> - si->s->key[PF_SK_STACK]->af &&
> - sk->af == si->s->key[PF_SK_STACK]->af &&
> - si->s->direction != s->direction))) {
> + TAILQ_FOREACH(si, &cur->sk_states, si_entry) {
> + struct pf_state *tst = si->si_st;
appreciate consistency in your diff. it uses 'tst = si->si_st;'
however going for 'sis' instead of 'tst' would remind us data here
come from state item. This is just a nit. Don't feel strongly
about it.
diff reads OK to me.
thanks and
regards
sashan
