Date: Fri, 14 Sep 2012 12:17:39 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Luigi Rizzo <rizzo@iet.unipi.it> Cc: luigi@FreeBSD.org, "Bjoern A. Zeeb" <bz@FreeBSD.org>, net@FreeBSD.org Subject: Re: moving pfil consumers to sys/netpfil Message-ID: <20120914081739.GV85604@glebius.int.ru> In-Reply-To: <20120914083340.GA31420@onelab2.iet.unipi.it> References: <20120912123457.GC85604@glebius.int.ru> <20120912211726.GB10974@onelab2.iet.unipi.it> <alpine.BSF.2.00.1209131623350.13080@ai.fobar.qr> <20120913174105.GA22571@onelab2.iet.unipi.it> <20120913193125.GO85604@glebius.int.ru> <20120913205721.GA24570@onelab2.iet.unipi.it> <20120914072350.GQ85604@glebius.int.ru> <20120914083340.GA31420@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Sep 14, 2012 at 10:33:40AM +0200, Luigi Rizzo wrote:
L> On Fri, Sep 14, 2012 at 11:23:50AM +0400, Gleb Smirnoff wrote:
L> > On Thu, Sep 13, 2012 at 10:57:21PM +0200, Luigi Rizzo wrote:
L> > L> On Thu, Sep 13, 2012 at 11:31:25PM +0400, Gleb Smirnoff wrote:
L> > L> > On Thu, Sep 13, 2012 at 07:41:05PM +0200, Luigi Rizzo wrote:
L> > L> > L> Second:
L> > L> > L> What i contest is the fact that you classify ipfw as a "pfil client",
L> > L> > L> when pfil is just a tiny adaptation layer to access ipfw.
L> > L> > L> I mentioned the three alternative APIs (netmap, netfilter, ndis)
L> > L> > L> to witness the fact that pfil is irrelevant to ipfw's operation.
L> > L> >
L> > L> > In FreeBSD ipfw is a pfil client. Dot.
L> > L> >
L> > L> > In Linux it is netfilter client, and if they want to they may or may not
L> > L> > put it under netfilter/ipfw. We don't care. Same for Windows and ndis.
L> > L>
L> > L> This is the second time a "we don't care" statement appears in
L> > L> this discussion and I find them extremely non constructive.
L> >
L> > Don't take "we don't care" out of context! We don't care where ipfw
L> > resides in Linux or Windows. I don't see why we should to. We do care
L> > where pfil consumers reside in FreeBSD.
L>
L> i am not taking it out of context, i am saying that deliberately
L> ignoring that this (or other) code is used elsewhere is not a sensible
L> thing to do -- it makes maintainance harder, reduces the chance of
L> getting bugfixes from the outside, etc. Not long ago there was a
L> thread on using wrappers to access ifnet fields so that third
L> parties have an easier time in using our code, and this is the
L> approach that we should follow.
L>
L> So rather than "we don't care" i'd like to read "let's see if we can
L> find a solution that satisfies all". I take your offer to move the
L> code also in stable/9 as a step in this direction.
L>
L> So, to somehow close the issue (as it seems a made decision)
L>
L> > L> I do not know who you consider as "we", but as one who maintains
L> > L> the code under multiple platforms i do have an interest in avoiding
L> > L> gratuitous changes that introduce differences between the various
L> > L> versions.
L> >
L> > The movement is mergeable since userland doesn't depend on it. If you'd
L> > like I could merge movement to stable/9, so you won't have any differencies
L> > between maintained versions.
L>
L> deal.
L>
L> > L> please remain in context.
L> > L> We were talking about the files in netinet/ipfw, which have
L> > L> #include <netinet/ipfw/something>
L> > L> in them, so when moved to the new location these 35 lines
L> > L> need to be changed to
L> > L> #include <netpfil/ipfw/something>
L> > L> Which is completely trivial, but introduces a difference between
L> > L> the files in HEAD and those in stable/9 (and in the other versions
L> > L> "we don't care" about) .
L> >
L> > Does ipfw resides in netinet/ipfw on other platforms: Linux, Windows?
L>
L> right now yes (it's an external module, so the only real concern is
L> the hardwired netinet/ in the #includes).
L> I dont mind relocating it there too, as long as it is done everywhere.
L>
L> > L> Now if your plan involves removing the netinet/ prefix and adding
L> > L> ... compile-with "${NORMAL_C} -I$S/netpfil"
L> > L> to the lines in HEAD:sys/conf/files, and
L> > L> ... compile-with "${NORMAL_C} -I$S/netinet"
L> > L> for those in stable/9, i can lift this objection.
L> >
L> > Don't see reason for -I. It compiles okay w/o it. See diff attached.
L>
L> It compiles because you have all these tiny changes:
L>
L> Index: sys/netpfil/ipfw/dn_heap.c
L> ===================================================================
L> --- sys/netpfil/ipfw/dn_heap.c (working copy)
L> +++ sys/netpfil/ipfw/dn_heap.c (working copy)
L> @@ -37,7 +37,7 @@
L> #include <sys/systm.h>
L> #include <sys/malloc.h>
L> #include <sys/kernel.h>
L> -#include <netinet/ipfw/dn_heap.h>
L> +#include <netpfil/ipfw/dn_heap.h>
L> #ifndef log
L> #define log(x, arg...)
L> #endif
L>
L> and not just a plain 'svn mv'.
L>
L> > L> I still think that a subsystem should not be classified by looking
L> > L> at one of the APIs it uses, but rather based on overall functionality.
L> > L> Hence in my opinion ipfw does not belong under netpfil/ more than
L> > L> it belongs under netinet/ , and lacking a better place i consider
L> > L> the relocation a gratuitous one.
L> >
L> > Ipfw overall functionality is packet filtering. Since packet filtering
L> > can happen at different layers and protocols, none of the following
L> > directories suit perfectly: net, netinet, netinet6. Thus, we (me + Bjoern)
L> > decided to make a separate directory for packet filters. Since all
L> > packet filters attach to kernel via pfil(9) we decided to name
L> > the directory netpfil.
L>
L> Up until now you were only talking about "pfil consumers",
L> and this is what i was disagreeing about.
L>
L> Now you say netpfil/ is the home for "packet filters", which is a
L> different story, and one that makes sense to me (but it also means
L> that it can be used as the home for other packet filters even if
L> they do not use the pfil api).
L>
L> So, it seems we have reached a compromise:
L>
L> netpfil is the home for "packet filters", and ipfw goes there also
L> in stable/9 (or, if it is simpler, we remove the netinet/ prefix
L> from the #include and compile with -I ...).
L>
L> ok ?
Thanks!
I'm fine with this. Then I just need ack from Bjoern that the move is
mergeable. IMO, it is mergeable, but I may miss something.
--
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120914081739.GV85604>