svn commit: r186187 - head/sys/net

Max Laier max at
Tue Dec 16 18:48:56 UTC 2008

On Tuesday 16 December 2008 19:20:40 Robert Watson wrote:
> On Tue, 16 Dec 2008, Max Laier wrote:
> >> Log:
> >>   A few locking fixes and cleanups to pfil hook registration,
> >>   unregistration, and execution:
> >>
> >>   - Add some brackets for clarity and trim a bit of vertical whitespace.
> >>   - Remove comments that may not contribute to clarity, such as "Lock"
> >>     before acquiring a lock and "Get memory" before allocating memory.
> >
> > These were meant as sectioning comments as pfil_add_hook grew larger and
> > larger in order to allow WAITOK allocations.  Should probably have been
> > "/* 1. get memory */" etc.
> I used to comment a bit more gratuitously along those lines, but bde seems
> to have (at least partially) broken me of the habit.  I felt sure there was
> a note in style(9) on not commenting on obvious things, but the closest I
> found was this:
>               exit(EX_OK);    /*
>                                * Avoid obvious comments such as
>                                * "Exit 0 on success."
>                                */
> I think, generally, that I agree with the observation that code comments
> should contribute to greater understanding of the code by summarizing
> complex code in a simple way, or lending insight to non-obvious
> implications of code, and that simply mirroring of code behavior at a low
> level in comments doesn't contribute a great deal.

I don't disagree - I was just offering the rational behind these comments.

> Speaking of PFIL_WAITOK, I actually had a pfil_flags-related question for
> you: I notice that pfil on NetBSD saves and propagates pfil_flags from
> handler to the packet_filter_hook, but on FreeBSD, the flags field is
> unused.  Is that intentional?  If so, I was thinking of putting a /*
> Unused. */ comment in the definition of packet_filter_hook.

I think this must have been lost along the way - unintentionally.  Since we 
get by fine without it, calling it unused seems like the right move.  It was 
obsoleted by having a separate list for each direction.

> Also, I notice PFIL_TYPE_IFNET is defined but not implemented -- did you
> plan to implement it at some point?

I didn't import pfil - it fell into my lap back in the day.  I don't have 
plans for PFIL_TYPE_IFNET, but I actively decided not to remove the leftovers 
in case somebody else would.

I do have some plans (way down on my priority list) to extend pfil(9) to 
support named hooks and hook reordering via sysctl.  This would allow people 
to use dummynet and pf at the same time.  There is some code over at pfsense, 
but it needs a bit of cleaning up - I'm afraid.

I'll leave the rest for the locking pros.

/"\  Best regards,                      | mlaier at
\ /  Max Laier                          | ICQ #67774661
 X  | mlaier at EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News

More information about the svn-src-all mailing list