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