Re: New preview patch for ipfw to pfil_hooks conversion

Luigi Rizzo wrote:
> 
> On Mon, Jun 21, 2004 at 11:36:21PM +0200, Andre Oppermann wrote:
> > Here is the next preview patch for the ipfw to pfil_hooks conversion:
> >
> >   http://www.nrg4u.com/freebsd/ipfw-pfilhooks-and-more-20040621.diff
> >
> > This patch significantly cleans up ip_input.c and ip_output.c.
> >
> 
> assorted comments:
> - understood that this is WIP, but as someone else suggested it would
>   be a lot better to split the patch in logical chunks and apply them
>   one at a time. Especially because they seem to have different importance,
>   e.g. (as far as i can tell):

Sure thing that I will split it up.  It's just more convinient for me
to diff up one big patch at the moment.

>   + ipfw pfil stuff
>         i surely consider this extremely useful and welcome, but would
>         prefer to put the hooks in ip_fw2.c instead of using a
>         separate file -- both to keep ipfw stuff confined, and to
>         use stricter compiler checks (e.g. define stuff static as
>         much as possible, avoid exporting internal APIs, etc.)
>         The only motivation against it would be if we plan to
>         backport this stuff to 4.x where we still have the option
>         of using ipfw1.

I don't have any plans to backport it to 4.x.  I chose to work with an
external file at the moment for convinience.  This way I don't have to
scroll through thousands of lines each time.

>   + ip_reass replacement
> 
>   + ip options processing
>         on this particular one i am a bit unsure -- what is the point
>         for just moving stuff to a separate file instead of leaving
>         it where it is (ip_input/ip_output) so that many functions
>         that are only used there can be declared static as they are now ?
>         I'd rather just apply bugfixes.

There are no bugs except one static variable which is used for packets
in transit.  I'll convert that to an m_tag before it goes in.

Generally the ip_input.c and ip_output.c files have become very large
and moving the ip options related functions out to their own file is
makes it a lot more readable and sorted/ordered.  IP options are only
rarely used these days.  Most of the functions either take an mbuf or
a socket so there is not much to break.

> generally, i have become a big fan of very strict compiler checks -- lately
> they have saved me a huge amount of time in identifying dead code,
> inconsistent interfaces and other bugs, so when in doubt between two
> alternatives i tend to privilege the one that gives more chances to
> the compiler to check things.

Yea, on the hand we don't want to have everything in one file because
that would make the checks as useful as not doing it.  Normally in my
coding style I prefer the safe variants, like bzero'ing entire structs
instead of doing it piece-meal throughout the code that follows.  Because
one day someone will change the order of some code parts and *poof* things
break in subtile and hard to debug ways.

> >   o ipfw forward is not yet implemented again (comes next)
> >   o ipfw layer2 is not yet implemented again (comes next)
> 
> of course it is fundamental to preserve the entire existing
> functionality before the commit

Sure.  As I said, this is WIP.  And when I reach the point of current
functionality == new functionality then I'll go and commit it.

-- 
Andre