> On 13 Oct 2015, at 21:12, Mike Belopuhov <m...@belopuhov.com> wrote:
>
> On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>>
>>> On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
>>> <alexandr.nedvedi...@oracle.com> 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? (-:
oh. i have no idea. it was a long time ago :(
putting it back makes sense to me, but id like the opinions of people who are
more aware of pf internals to agree with me too.
dlg
