kern/156770: [ipfw] [dummynet] [patch]: performance improvement and several extensions

[Alexander V. Chernikov]

On 01.07.2012 23:09, Luigi Rizzo wrote:
> On Sun, Jul 01, 2012 at 03:54:35PM +0000, melifaro at freebsd.org wrote:
>> Synopsis: [ipfw] [dummynet] [patch]: performance improvement and several extensions
>>
>> Responsible-Changed-From-To: freebsd-ipfw->melifaro
>> Responsible-Changed-By: melifaro
>> Responsible-Changed-When: Sun Jul 1 15:54:17 UTC 2012
>> Responsible-Changed-Why:
>> Take
>>
>> http://www.freebsd.org/cgi/query-pr.cgi?pr=156770
>
> Alex,
Not sure if you're speaking to me, since both submitter and I are 
Alexanders :) However I'll try to answer some of the questions.
> please any ipfw-related patch through me before committing.
>
> On this specific PR i have some comments and several concerns.
>
> First, as mentioned in the thread, some specific features (e.g. ftags)
> might be of interest, but the fact that this is a single monolitic patch
> make it hard to apply and review. Especially, at least judging from the
> description, i believe some of the changes replicate features that
> were already inserted around 2009 and later (in then-head).
We already got private discussion resulting in preparation of some most 
interesting (at least to me) parts of code to be split into different 
patches and remade to work on -current.

Particularly I'm interested in rule indexes mostly.

>
> On the negative side:
> - documentation on new features is completely absent. Just a brief mention
>    in the manpage of ftag/funtag, a short comment in a C source code.
>
> - the way some features are implemented is through adding new IOCTLs,
>    which is the wrong way of doing things. In the 2009 rewrite (ipfw3)
>    i tried to use a single ioctl which carries tagged messages
>    for the various requests (similar to the microinstructions which make
>    up a rule) so the code is easier to extend without breaking ABIs.
>    Please follow the new style if you need to add commands.
IP_FW3 is already used in ipv6 tables code, so there are some ipfw(8) 
and kernel code to reuse.
>
> - can you please split the patch in individual components, and
>    make sure that they not replicate functions already existent
>    (or if they do, are they an improvement) ? I am especially
>    referring to indexed skipto
>
> - a large number of changes to the userspace code replaces errx()
>    with return my_err(...) . I might agree on the principle, but
>    I'd like to see a few notes on why this change is required,
>    and whether it can be applied independently of the others.
>
> cheers
> luigi
>