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

[Luigi Rizzo]

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,
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).

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.

- 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