On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>
> > On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> > <[email protected]> wrote:
> >
> > Hello,
> >
> > patch below introduces struct refcnt to pfi_kif structure. Patch also
> > changes
> > pfi_kif_get() function to always return a reference to pfi_kif instance.
> >
> > Furthermore existing functions pfi_kif_ref()/pfi_kif_unref() are thrown away
> > in favor of pfi_kif_take()/pfi_kif_rele(), which follow naming convention
> > set by refcnt_init(9). Patch also removes kif reference types (enum
> > pfi_kif_refs).
> >
> > Patch also updates if_pfsync.c file. I'm bit puzzled with test as follows
> > in pfsync_in_clr():
> >
> > 770 for (i = 0; i < count; i++) {
> > 771 clr = (struct pfsync_clr *)buf + len * i;
> > 772 creatorid = clr->creatorid;
> > 773
> > 774 if (clr->ifname[0] == '\0') {
> > ...
> > 783 } else {
> > 784 if (pfi_kif_get(clr->ifname) == NULL)
> > 789 continue;
> > 790
> >
> > I could not make any sense of line 784. Are we just making sure particular
> > kif
> > object exists for clr->ifname? Note pfi_kif_get() creates kif for us if it
> > does not exist. If we want to query for kif existence only patch offers
> > pfi_kif_check() function. I need some advice here if we should keep, what's
> > currently in patch or switch to pfi_kif_check().
>
> my name is probably against that code.
>
> i didn't realise that pfi_kif_get creates a kif on demand. however,
> it also uses malloc(, , M_NOWAIT) to try and allocate it, which can
> fail and cause pfi_kif_get to return NULL.
>
Revision 1.102 used to get the kif and check the state against it:
if (si->s->creatorid == creatorid &&
si->s->kif == kif) {
Then in the rev 1.103 you have removed the check. The question is
why did you do that? (-:
