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