Hello,
> > > 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).
> > >
> [snip]
> > > @@ -810,3 +762,28 @@ pfi_unmask(void *addr)
> > > return (b);
> > > }
> > >
> > > +struct pfi_kif *
> > > +pfi_kif_take(struct pfi_kif *kif)
> > > +{
> > > + if (kif != pfi_all) {
> > > + PF_REF_TAKE(kif->pfik_refcnt);
> > > + }
>
> No need for curly braces here --^
>
yes, that's true, will fix that...
>
> Even the typedef? (-:
>
sorry I forgot about OpenBSD hates typedefs ;-)
I'll keep in mind to kill all typdefs in my patches next time.
> but I'd prefer the former, e.g. _k instead of _kif_. Double
O.K. I'll stick to _k style.
>
> PFI_KIF_TAKE and PFI_KIF_RELE macros add another level of
> complexity for almost zero gain. PFI_KIF_TAKE is not even
> used. I'd say scrap them.
it's true in case of pfi_kif objects we can scrap them.
on the other hand the macros illustrate the way I'd like to reference counting
for other objects. I'll stick to the pfi_kif just as an illustration.
if I use lowercase function such as:
r->kif = pfi_kif_take(kif);
then I'm sure the kif instance exists, if it does not exist the thing
will blow up with NULL pointer dereference and I know I'm wrong. On the
other hand if I do:
r->kif = PFI_KIF_TAKE(kif);
then it means kif is optional here, but if it exists, then I must have
a reference for it here.
The way we have it in PF on Solaris these days looks as follows:
_take(x)
{
if (x != NULL)
PF_INC_REF(x->refcnt);
return (x);
}
This way we assume 'x' is always optional. And this is sometimes quite
wrong, as we might set mine, we trip over later.
The uppercase macros allows us to easily annotate cases, where the
object is optional and vice versa: the lower case functions annotate
cases, where we assume the object must exist. This could save us few
days
of debugging.
So I'd like to make sure if you hate that approach in general or in the
scope of this particular patch. I admit the pfi_kif is not the right
guy for the uppercase version as the pfi_kif instance always must exist.
I'll remove those macros from proposed patch, but I'd like to know
how do you feel about them for other objects such as tables, anchors,
states, ....
> I would also say that the PF_REF_* should stay under _KERNEL
> as they simply cannot be used in the userland and moved some-
> where before "extern struct pf_status pf_status" (after the
> prototypes chunk) or after the "extern struct pf_pool_limit"
> right before the "#endif /* _KERNEL */".
>
yes that's very good point. (How I could miss it?)
thanks and
regards
sasha
