On 29/10/15(Thu) 13:32, Mike Belopuhov wrote: > On Thu, Oct 29, 2015 at 11:58 +0100, Martin Pieuchot wrote: > > On 29/10/15(Thu) 02:49, Mike Belopuhov wrote: > > > On 28 October 2015 at 18:41, Alexandr Nedvedicky > > > <alexandr.nedvedi...@oracle.com> wrote: > > > > Hello Mike, > > > > > > > > just a quick question: > > > > > > > > are you going to commit your pfi_kif_find() et. al.? > > > > or more work is needed there? > > > > > > > > > > I need OKs > > > [...] > > > > pfsync_in_clr(caddr_t buf, int len, int count, int flags) > > > > { > > > > struct pfsync_clr *clr; > > > > - int i; > > > > - > > > > struct pf_state *st, *nexts; > > > > - struct pf_state_key *sk, *nextsk; > > > > - struct pf_state_item *si; > > > > + struct pfi_kif *kif = NULL; > > > > u_int32_t creatorid; > > > > + int i; > > > > > > > > for (i = 0; i < count; i++) { > > > > clr = (struct pfsync_clr *)buf + len * i; > > > > creatorid = clr->creatorid; > > > > + if (strlen(clr->ifname) && > > > > + (kif = pfi_kif_find(clr->ifname)) == NULL) > > > > + continue; > > > > -[...] > > > > + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = > > > > nexts) { > > > > + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st); > > > > + if (st->creatorid == creatorid && > > > > + ((kif && st->kif == kif) || !kif)) { > > > > I find this check confusing. > > > > Since you're continuing when "kif == NULL" can't you change this check > > into "st->kif == kif"? > > > > No. > > > Or is it possible for strlen(clr->ifname) to return 0 > > Yes. > > > in which case you > > might end up comparing a different ``kif''? > > > > To garbage even... > > ...and I need to move the "kif = NULL" after creatorid actually. > It's rather unlikely to get a message with several entries having > clr->ifname set, but the code must be correct nevertheless.
ok mpi@ > > diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c > index 7d633db..5296642 100644 > --- sys/net/if_pfsync.c > +++ sys/net/if_pfsync.c > @@ -752,46 +752,29 @@ done: > > int > pfsync_in_clr(caddr_t buf, int len, int count, int flags) > { > struct pfsync_clr *clr; > - int i; > - > struct pf_state *st, *nexts; > - struct pf_state_key *sk, *nextsk; > - struct pf_state_item *si; > + struct pfi_kif *kif; > u_int32_t creatorid; > + int i; > > for (i = 0; i < count; i++) { > clr = (struct pfsync_clr *)buf + len * i; > + kif = NULL; > creatorid = clr->creatorid; > + if (strlen(clr->ifname) && > + (kif = pfi_kif_find(clr->ifname)) == NULL) > + continue; > > - if (clr->ifname[0] == '\0') { > - for (st = RB_MIN(pf_state_tree_id, &tree_id); > - st; st = nexts) { > - nexts = RB_NEXT(pf_state_tree_id, &tree_id, st); > - if (st->creatorid == creatorid) { > - SET(st->state_flags, PFSTATE_NOSYNC); > - pf_unlink_state(st); > - } > - } > - } else { > - if (pfi_kif_get(clr->ifname) == NULL) > - continue; > - > - /* XXX correct? */ > - for (sk = RB_MIN(pf_state_tree, &pf_statetbl); > - sk; sk = nextsk) { > - nextsk = RB_NEXT(pf_state_tree, > - &pf_statetbl, sk); > - TAILQ_FOREACH(si, &sk->states, entry) { > - if (si->s->creatorid == creatorid) { > - SET(si->s->state_flags, > - PFSTATE_NOSYNC); > - pf_unlink_state(si->s); > - } > - } > + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts) { > + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st); > + if (st->creatorid == creatorid && > + ((kif && st->kif == kif) || !kif)) { > + SET(st->state_flags, PFSTATE_NOSYNC); > + pf_unlink_state(st); > } > } > } > > return (0); > diff --git sys/net/pf_if.c sys/net/pf_if.c > index caaf9f9..bf77184 100644 > --- sys/net/pf_if.c > +++ sys/net/pf_if.c > @@ -97,18 +97,25 @@ pfi_initialize(void) > if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL) > panic("pfi_kif_get for pfi_all failed"); > } > > struct pfi_kif * > -pfi_kif_get(const char *kif_name) > +pfi_kif_find(const char *kif_name) > { > - struct pfi_kif *kif; > struct pfi_kif_cmp s; > > bzero(&s, sizeof(s)); > strlcpy(s.pfik_name, kif_name, sizeof(s.pfik_name)); > - if ((kif = RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s)) != NULL) > + return (RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s)); > +} > + > +struct pfi_kif * > +pfi_kif_get(const char *kif_name) > +{ > + struct pfi_kif *kif; > + > + if ((kif = pfi_kif_find(kif_name))) > return (kif); > > /* create new one */ > if ((kif = malloc(sizeof(*kif), PFI_MTYPE, M_NOWAIT|M_ZERO)) == NULL) > return (NULL); > diff --git sys/net/pfvar.h sys/net/pfvar.h > index cdb2f7f..76a98a9 100644 > --- sys/net/pfvar.h > +++ sys/net/pfvar.h > @@ -1808,10 +1808,11 @@ int pfr_ina_define(struct pfr_table *, struct > pfr_addr *, int, int *, > int *, u_int32_t, int); > > extern struct pfi_kif *pfi_all; > > void pfi_initialize(void); > +struct pfi_kif *pfi_kif_find(const char *); > struct pfi_kif *pfi_kif_get(const char *); > void pfi_kif_ref(struct pfi_kif *, enum pfi_kif_refs); > void pfi_kif_unref(struct pfi_kif *, enum pfi_kif_refs); > int pfi_kif_match(struct pfi_kif *, struct pfi_kif *); > void pfi_attach_ifnet(struct ifnet *); >