Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-05-03 Thread Alexander V. Chernikov 

On 30.04.2012 17:39, Luigi Rizzo wrote:

On Mon, Apr 30, 2012 at 04:17:30PM +0400, Alexander V. Chernikov wrote:

On 30.04.2012 15:36, Gleb Smirnoff wrote:

On Mon, Apr 30, 2012 at 01:48:36PM +0200, Luigi Rizzo wrote:
L   On Mon, Apr 30, 2012 at 10:22:23AM +, Alexander V. Chernikov wrote:
L  Author: melifaro
L  Date: Mon Apr 30 10:22:23 2012
L  New Revision: 234834
L  URL: http://svn.freebsd.org/changeset/base/234834
L   
L  Log:
LMove several enums and structures required for L2 filtering from
ip_fw_private.h to ip_fw.h.
L
L   I would be really grateful if you could revert this back and discuss
L   what you wanted to achieve with this change other than saving one
L   entry in the list of includes.

Changing something inside ip_fw_private.h (for example, locking  change)
requires changes in several totally unrelated subsystems, which is
clearly bad.


There are certainly good reasons to split things even further
(as Gleb suggested) so if you want to follow that route
and have patches for review i will be glad to discuss that.

Yup. I'll make another patch


I am the first one to say that the name ip_fw_private.h is confusing,
but at the time i did not feel brave enough to move from a single
header (ip_fw.h) to the four (ip_fw.h, ip_fw_user.h, ip_fw_kernel.h,
ip_fw_kernel_private.h) that would be needed for proper confinement
of information.

This said, the change you have made introduce a worse form of header
pollution and this is why i am requesting a backout.


Reverted in r234946



L
L   As clearly mentioned in the commit logs
L
L   http://svnweb.freebsd.org/base?view=revisionrevision=200580

Maybe there are some other possibilities documenting preferred layout
other than commit log? Searching 2+yrs commit history is not the best
way of finding information.


sure, you could have asked people involved with ipfw development
to review this change.

Also don't expect a single policy in style(9) to hold for all
possible situations: the kernel is huge, it has parts that come
from multiple origins in time and space, and sometimes is shared
with other OSs as well. Refactoring (such as your changes) should
keep that in mind.

Let's not make a big deal of this thing: we all make mistakes or
have different opinions on how things should be done, and the only
way to avoid them is to discuss thing in advance, or be open to
resolve things when problems arise. No hard feelings.

Ok :)


cheers
luigi




--
WBR, Alexander
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-05-03 Thread Luigi Rizzo 
On Thu, May 03, 2012 at 01:00:16PM +0400, Alexander V. Chernikov wrote:
...
 Reverted in r234946

thank you very much for the quick resolution of the issue.
I really appreciate it.

cheers
luigi
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-04-30 Thread Alexander V. Chernikov 
Author: melifaro
Date: Mon Apr 30 10:22:23 2012
New Revision: 234834
URL: http://svn.freebsd.org/changeset/base/234834

Log:
  Move several enums and structures required for L2 filtering from 
ip_fw_private.h to ip_fw.h.
  Remove ipfw/ip_fw_private.h header from non-ipfw code.
  
  Approved by:ae(mentor)
  MFC after:  2 weeks

Modified:
  head/sys/contrib/pf/net/pf.c
  head/sys/net/if_bridge.c
  head/sys/net/if_ethersubr.c
  head/sys/netinet/ip_fw.h
  head/sys/netinet/ipfw/ip_fw_private.h
  head/sys/ofed/drivers/infiniband/ulp/ipoib/ipoib.h

Modified: head/sys/contrib/pf/net/pf.c
==
--- head/sys/contrib/pf/net/pf.cMon Apr 30 09:46:05 2012
(r234833)
+++ head/sys/contrib/pf/net/pf.cMon Apr 30 10:22:23 2012
(r234834)
@@ -122,7 +122,6 @@ __FBSDID($FreeBSD$);
 #include netinet/if_ether.h
 #ifdef __FreeBSD__
 #include netinet/ip_fw.h
-#include netinet/ipfw/ip_fw_private.h /* XXX: only for DIR_IN/DIR_OUT */
 #endif
 
 #ifndef __FreeBSD__

Modified: head/sys/net/if_bridge.c
==
--- head/sys/net/if_bridge.cMon Apr 30 09:46:05 2012(r234833)
+++ head/sys/net/if_bridge.cMon Apr 30 10:22:23 2012(r234834)
@@ -132,7 +132,6 @@ __FBSDID($FreeBSD$);
 
 #include net/route.h
 #include netinet/ip_fw.h
-#include netinet/ipfw/ip_fw_private.h
 
 /*
  * Size of the route hash table.  Must be a power of two.

Modified: head/sys/net/if_ethersubr.c
==
--- head/sys/net/if_ethersubr.c Mon Apr 30 09:46:05 2012(r234833)
+++ head/sys/net/if_ethersubr.c Mon Apr 30 10:22:23 2012(r234834)
@@ -72,7 +72,6 @@
 #include netinet/ip_carp.h
 #include netinet/ip_var.h
 #include netinet/ip_fw.h
-#include netinet/ipfw/ip_fw_private.h
 #endif
 #ifdef INET6
 #include netinet6/nd6.h

Modified: head/sys/netinet/ip_fw.h
==
--- head/sys/netinet/ip_fw.hMon Apr 30 09:46:05 2012(r234833)
+++ head/sys/netinet/ip_fw.hMon Apr 30 10:22:23 2012(r234834)
@@ -545,6 +545,88 @@ struct ipfw_flow_id {
 
 #define IS_IP6_FLOW_ID(id) ((id)-addr_type == 6)
 
+#ifdef _KERNEL
+/* Return values from ipfw_[ether_]chk() */
+enum {
+   IP_FW_PASS = 0,
+   IP_FW_DENY,
+   IP_FW_DIVERT,
+   IP_FW_TEE,
+   IP_FW_DUMMYNET,
+   IP_FW_NETGRAPH,
+   IP_FW_NGTEE,
+   IP_FW_NAT,
+   IP_FW_REASS,
+};
+
+/*
+ * Hooks sometime need to know the direction of the packet
+ * (divert, dummynet, netgraph, ...)
+ * We use a generic definition here, with bit0-1 indicating the
+ * direction, bit 2 indicating layer2 or 3, bit 3-4 indicating the
+ * specific protocol (if necessary)
+ */
+enum {
+   DIR_MASK =  0x3,
+   DIR_OUT =   0,
+   DIR_IN =1,
+   DIR_FWD =   2,
+   DIR_DROP =  3,
+   PROTO_LAYER2 =  0x4, /* set for layer 2 */
+   /* PROTO_DEFAULT = 0, */
+   PROTO_IPV4 =0x08,
+   PROTO_IPV6 =0x10,
+   PROTO_IFB = 0x0c, /* layer2 + ifbridge */
+   /*  PROTO_OLDBDG =  0x14, unused, old bridge */
+};
+
+/*
+ * Structure for collecting parameters to dummynet for ip6_output forwarding
+ */
+struct _ip6dn_args {
+   struct ip6_pktopts *opt_or;
+   struct route_in6 ro_or;
+   int flags_or;
+   struct ip6_moptions *im6o_or;
+   struct ifnet *origifp_or;
+   struct ifnet *ifp_or;
+   struct sockaddr_in6 dst_or;
+   u_long mtu_or;
+   struct route_in6 ro_pmtu_or;
+};
+
+/*
+ * Arguments for calling ipfw_chk() and dummynet_io(). We put them
+ * all into a structure because this way it is easier and more
+ * efficient to pass variables around and extend the interface.
+ */
+struct ip_fw_args {
+   struct mbuf *m; /* the mbuf chain   */
+   struct ifnet*oif;   /* output interface */
+   struct sockaddr_in *next_hop;   /* forward address  */
+   struct sockaddr_in6 *next_hop6; /* ipv6 forward address */
+
+   /*
+* On return, it points to the matching rule.
+* On entry, rule.slot  0 means the info is valid and
+* contains the starting rule for an ipfw search.
+* If chain_id == chain-id  slot 0 then jump to that slot.
+* Otherwise, we locate the first rule = rulenum:rule_id
+*/
+   struct ipfw_rule_ref rule;  /* match/restart info   */
+
+   struct ether_header *eh;/* for bridged packets  */
+
+   struct ipfw_flow_id f_id;   /* grabbed from IP header   */
+   //uint32_t  cookie; /* a cookie depending on rule action */
+   struct inpcb*inp;
+
+   struct _ip6dn_args  dummypar; /* dummynet-ip6_output */
+   struct sockaddr_in hopstore;

Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-04-30 Thread Luigi Rizzo 
On Mon, Apr 30, 2012 at 10:22:23AM +, Alexander V. Chernikov wrote:
 Author: melifaro
 Date: Mon Apr 30 10:22:23 2012
 New Revision: 234834
 URL: http://svn.freebsd.org/changeset/base/234834
 
 Log:
   Move several enums and structures required for L2 filtering from 
 ip_fw_private.h to ip_fw.h.

I would be really grateful if you could revert this back and discuss
what you wanted to achieve with this change other than saving one
entry in the list of includes.

As clearly mentioned in the commit logs

http://svnweb.freebsd.org/base?view=revisionrevision=200580

when i did the last revision of the ipfw+dummynet code i tried
to put a strong separation between what is visible in userland
(ip_fw.h and ip_dummynet.h) and kernel specific stuff.
This way changes in the kernel code do not need to affect userland,
modify installed headers and so on.

This is why kernel-specific definitions were put in private files.
We may discuss on the filename, ip_fw_kernel.h may be a better fit,
but merging back kernel and userland defs is a bad design decision.

20-30 years ago there were good reasons to use a single header
for all sorts of definitions: user-only, kernel-only, and kernel-userland API.
Machines were slow, disks were small, portability was not a big deal.

These days none of these conditions apply and keeping things
separate helps maintainance and avoid accidental pollution of
definitions and their misuse.

Besides, keep in mind that ipfw and dummynet are meant to work
on multiple platforms so this change is causing portability troubles.

cheers
luigi

   Approved by:ae(mentor)
   MFC after:  2 weeks
 
 Modified:
   head/sys/contrib/pf/net/pf.c
   head/sys/net/if_bridge.c
   head/sys/net/if_ethersubr.c
   head/sys/netinet/ip_fw.h
   head/sys/netinet/ipfw/ip_fw_private.h
   head/sys/ofed/drivers/infiniband/ulp/ipoib/ipoib.h
 
 Modified: head/sys/contrib/pf/net/pf.c
 ==
 --- head/sys/contrib/pf/net/pf.c  Mon Apr 30 09:46:05 2012
 (r234833)
 +++ head/sys/contrib/pf/net/pf.c  Mon Apr 30 10:22:23 2012
 (r234834)
 @@ -122,7 +122,6 @@ __FBSDID($FreeBSD$);
  #include netinet/if_ether.h
  #ifdef __FreeBSD__
  #include netinet/ip_fw.h
 -#include netinet/ipfw/ip_fw_private.h /* XXX: only for DIR_IN/DIR_OUT */
  #endif
  
  #ifndef __FreeBSD__
 
 Modified: head/sys/net/if_bridge.c
 ==
 --- head/sys/net/if_bridge.c  Mon Apr 30 09:46:05 2012(r234833)
 +++ head/sys/net/if_bridge.c  Mon Apr 30 10:22:23 2012(r234834)
 @@ -132,7 +132,6 @@ __FBSDID($FreeBSD$);
  
  #include net/route.h
  #include netinet/ip_fw.h
 -#include netinet/ipfw/ip_fw_private.h
  
  /*
   * Size of the route hash table.  Must be a power of two.
 
 Modified: head/sys/net/if_ethersubr.c
 ==
 --- head/sys/net/if_ethersubr.c   Mon Apr 30 09:46:05 2012
 (r234833)
 +++ head/sys/net/if_ethersubr.c   Mon Apr 30 10:22:23 2012
 (r234834)
 @@ -72,7 +72,6 @@
  #include netinet/ip_carp.h
  #include netinet/ip_var.h
  #include netinet/ip_fw.h
 -#include netinet/ipfw/ip_fw_private.h
  #endif
  #ifdef INET6
  #include netinet6/nd6.h
 
 Modified: head/sys/netinet/ip_fw.h
 ==
 --- head/sys/netinet/ip_fw.h  Mon Apr 30 09:46:05 2012(r234833)
 +++ head/sys/netinet/ip_fw.h  Mon Apr 30 10:22:23 2012(r234834)
 @@ -545,6 +545,88 @@ struct ipfw_flow_id {
  
  #define IS_IP6_FLOW_ID(id)   ((id)-addr_type == 6)
  
 +#ifdef _KERNEL
 +/* Return values from ipfw_[ether_]chk() */
 +enum {
 + IP_FW_PASS = 0,
 + IP_FW_DENY,
 + IP_FW_DIVERT,
 + IP_FW_TEE,
 + IP_FW_DUMMYNET,
 + IP_FW_NETGRAPH,
 + IP_FW_NGTEE,
 + IP_FW_NAT,
 + IP_FW_REASS,
 +};
 +
 +/*
 + * Hooks sometime need to know the direction of the packet
 + * (divert, dummynet, netgraph, ...)
 + * We use a generic definition here, with bit0-1 indicating the
 + * direction, bit 2 indicating layer2 or 3, bit 3-4 indicating the
 + * specific protocol (if necessary)
 + */
 +enum {
 + DIR_MASK =  0x3,
 + DIR_OUT =   0,
 + DIR_IN =1,
 + DIR_FWD =   2,
 + DIR_DROP =  3,
 + PROTO_LAYER2 =  0x4, /* set for layer 2 */
 + /* PROTO_DEFAULT = 0, */
 + PROTO_IPV4 =0x08,
 + PROTO_IPV6 =0x10,
 + PROTO_IFB = 0x0c, /* layer2 + ifbridge */
 +   /*PROTO_OLDBDG =  0x14, unused, old bridge */
 +};
 +
 +/*
 + * Structure for collecting parameters to dummynet for ip6_output forwarding
 + */
 +struct _ip6dn_args {
 +   struct ip6_pktopts *opt_or;
 +   struct route_in6 ro_or;
 +   int flags_or;
 +   struct ip6_moptions *im6o_or;
 +   struct ifnet *origifp_or;
 +   struct ifnet *ifp_or;
 +   struct sockaddr_in6

Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-04-30 Thread Gleb Smirnoff 
On Mon, Apr 30, 2012 at 01:48:36PM +0200, Luigi Rizzo wrote:
L On Mon, Apr 30, 2012 at 10:22:23AM +, Alexander V. Chernikov wrote:
L  Author: melifaro
L  Date: Mon Apr 30 10:22:23 2012
L  New Revision: 234834
L  URL: http://svn.freebsd.org/changeset/base/234834
L  
L  Log:
LMove several enums and structures required for L2 filtering from 
ip_fw_private.h to ip_fw.h.
L 
L I would be really grateful if you could revert this back and discuss
L what you wanted to achieve with this change other than saving one
L entry in the list of includes.
L 
L As clearly mentioned in the commit logs
L 
L http://svnweb.freebsd.org/base?view=revisionrevision=200580
L 
L when i did the last revision of the ipfw+dummynet code i tried
L to put a strong separation between what is visible in userland
L (ip_fw.h and ip_dummynet.h) and kernel specific stuff.
L This way changes in the kernel code do not need to affect userland,
L modify installed headers and so on.
L 
L This is why kernel-specific definitions were put in private files.
L We may discuss on the filename, ip_fw_kernel.h may be a better fit,
L but merging back kernel and userland defs is a bad design decision.
L 
L 20-30 years ago there were good reasons to use a single header
L for all sorts of definitions: user-only, kernel-only, and kernel-userland 
API.
L Machines were slow, disks were small, portability was not a big deal.
L 
L These days none of these conditions apply and keeping things
L separate helps maintainance and avoid accidental pollution of
L definitions and their misuse.
L 
L Besides, keep in mind that ipfw and dummynet are meant to work
L on multiple platforms so this change is causing portability troubles.

Can we split ip_fw_private.h to ip_fw_private.h, and ip_fw_var.h? The
former is really private, and the latter is for other kernel modules.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-04-30 Thread Alexander V. Chernikov 

On 30.04.2012 15:36, Gleb Smirnoff wrote:

On Mon, Apr 30, 2012 at 01:48:36PM +0200, Luigi Rizzo wrote:
L  On Mon, Apr 30, 2012 at 10:22:23AM +, Alexander V. Chernikov wrote:
LAuthor: melifaro
LDate: Mon Apr 30 10:22:23 2012
LNew Revision: 234834
LURL: http://svn.freebsd.org/changeset/base/234834
L  
LLog:
L  Move several enums and structures required for L2 filtering from 
ip_fw_private.h to ip_fw.h.
L
L  I would be really grateful if you could revert this back and discuss
L  what you wanted to achieve with this change other than saving one
L  entry in the list of includes.
Changing something inside ip_fw_private.h (for example, locking  change) 
requires changes in several totally unrelated subsystems, which is 
clearly bad.

L
L  As clearly mentioned in the commit logs
L
L  http://svnweb.freebsd.org/base?view=revisionrevision=200580
Maybe there are some other possibilities documenting preferred layout 
other than commit log? Searching 2+yrs commit history is not the best 
way of finding information.



L
L  when i did the last revision of the ipfw+dummynet code i tried
L  to put a strong separation between what is visible in userland
L  (ip_fw.h and ip_dummynet.h) and kernel specific stuff.
L  This way changes in the kernel code do not need to affect userland,
L  modify installed headers and so on.
L
L  This is why kernel-specific definitions were put in private files.
Unfortunately, it is not so obvious (at least for me ip_fw_ 
_private__.h looks much like ipfw private headers, used by ipfw 
subsystem only, not kernel ipfw specific stuff).

L  We may discuss on the filename, ip_fw_kernel.h may be a better fit,
Personally I prefer glebius@ suggestion with ip_fw_var.h for such common 
headers.

L  but merging back kernel and userland defs is a bad design decision.
So should style(9) be updated with _KERNEL define is bad line to make 
newcomers aware of this easy way ? :)

L
L  20-30 years ago there were good reasons to use a single header
L  for all sorts of definitions: user-only, kernel-only, and kernel-userland 
API.
L  Machines were slow, disks were small, portability was not a big deal.
L
L  These days none of these conditions apply and keeping things
L  separate helps maintainance and avoid accidental pollution of
L  definitions and their misuse.
L
L  Besides, keep in mind that ipfw and dummynet are meant to work
L  on multiple platforms so this change is causing portability troubles.

Can we split ip_fw_private.h to ip_fw_private.h, and ip_fw_var.h? The
former is really private, and the latter is for other kernel modules.




___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib

2012-04-30 Thread Luigi Rizzo 
On Mon, Apr 30, 2012 at 04:17:30PM +0400, Alexander V. Chernikov wrote:
 On 30.04.2012 15:36, Gleb Smirnoff wrote:
 On Mon, Apr 30, 2012 at 01:48:36PM +0200, Luigi Rizzo wrote:
 L  On Mon, Apr 30, 2012 at 10:22:23AM +, Alexander V. Chernikov wrote:
 LAuthor: melifaro
 LDate: Mon Apr 30 10:22:23 2012
 LNew Revision: 234834
 LURL: http://svn.freebsd.org/changeset/base/234834
 L  
 LLog:
 L  Move several enums and structures required for L2 filtering from 
 ip_fw_private.h to ip_fw.h.
 L
 L  I would be really grateful if you could revert this back and discuss
 L  what you wanted to achieve with this change other than saving one
 L  entry in the list of includes.
 Changing something inside ip_fw_private.h (for example, locking  change) 
 requires changes in several totally unrelated subsystems, which is 
 clearly bad.

There are certainly good reasons to split things even further
(as Gleb suggested) so if you want to follow that route
and have patches for review i will be glad to discuss that.

I am the first one to say that the name ip_fw_private.h is confusing,
but at the time i did not feel brave enough to move from a single
header (ip_fw.h) to the four (ip_fw.h, ip_fw_user.h, ip_fw_kernel.h,
ip_fw_kernel_private.h) that would be needed for proper confinement
of information.

This said, the change you have made introduce a worse form of header
pollution and this is why i am requesting a backout.

 L
 L  As clearly mentioned in the commit logs
 L
 L  http://svnweb.freebsd.org/base?view=revisionrevision=200580
 Maybe there are some other possibilities documenting preferred layout 
 other than commit log? Searching 2+yrs commit history is not the best 
 way of finding information.

sure, you could have asked people involved with ipfw development
to review this change.

Also don't expect a single policy in style(9) to hold for all
possible situations: the kernel is huge, it has parts that come
from multiple origins in time and space, and sometimes is shared
with other OSs as well. Refactoring (such as your changes) should
keep that in mind.

Let's not make a big deal of this thing: we all make mistakes or
have different opinions on how things should be done, and the only
way to avoid them is to discuss thing in advance, or be open to
resolve things when problems arise. No hard feelings.

cheers
luigi
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org