Date: Sun, 1 Jul 2012 21:09:21 +0200 From: Luigi Rizzo <rizzo@iet.unipi.it> To: melifaro@freebsd.org, bug-followup@freebsd.org, alter@alter.org.ua Cc: freebsd-ipfw@freebsd.org Subject: Re: kern/156770: [ipfw] [dummynet] [patch]: performance improvement and several extensions Message-ID: <20120701190921.GA63663@onelab2.iet.unipi.it> In-Reply-To: <201207011554.q61FsZ6A039188@freefall.freebsd.org> References: <201207011554.q61FsZ6A039188@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jul 01, 2012 at 03:54:35PM +0000, melifaro@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120701190921.GA63663>