From owner-svn-src-head@FreeBSD.ORG Mon Apr 30 11:28:55 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 72EFA106566C; Mon, 30 Apr 2012 11:28:55 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id C1C0C8FC16; Mon, 30 Apr 2012 11:28:54 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 405727300A; Mon, 30 Apr 2012 13:48:36 +0200 (CEST) Date: Mon, 30 Apr 2012 13:48:36 +0200 From: Luigi Rizzo To: "Alexander V. Chernikov" , ae@freebsd.org Message-ID: <20120430114836.GA62284@onelab2.iet.unipi.it> References: <201204301022.q3UAMNcq060049@svn.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201204301022.q3UAMNcq060049@svn.freebsd.org> User-Agent: Mutt/1.4.2.3i Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r234834 - in head/sys: contrib/pf/net net netinet netinet/ipfw ofed/drivers/infiniband/ulp/ipoib X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Apr 2012 11:28:55 -0000 On Mon, Apr 30, 2012 at 10:22:23AM +0000, 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=revision&revision=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 > #ifdef __FreeBSD__ > #include > -#include /* 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 > #include > -#include > > /* > * 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 > #include > #include > -#include > #endif > #ifdef INET6 > #include > > 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 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; /* store here if cannot use a pointer */ > +}; > + > +#endif /* _KERNEL */ > + > /* > * Dynamic ipfw rule. > */ > > Modified: head/sys/netinet/ipfw/ip_fw_private.h > ============================================================================== > --- head/sys/netinet/ipfw/ip_fw_private.h Mon Apr 30 09:46:05 2012 (r234833) > +++ head/sys/netinet/ipfw/ip_fw_private.h Mon Apr 30 10:22:23 2012 (r234834) > @@ -48,89 +48,8 @@ > #define SYSEND > #endif > > -/* Return values from ipfw_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, > -}; > - > -/* > - * 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; /* store here if cannot use a pointer */ > -}; > - > MALLOC_DECLARE(M_IPFW); > > -/* > - * 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 > - * indicating the 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 */ > -}; > - > /* wrapper for freeing a packet, in case we need to do more work */ > #ifndef FREE_PKT > #if defined(__linux__) || defined(_WIN32) > > Modified: head/sys/ofed/drivers/infiniband/ulp/ipoib/ipoib.h > ============================================================================== > --- head/sys/ofed/drivers/infiniband/ulp/ipoib/ipoib.h Mon Apr 30 09:46:05 2012 (r234833) > +++ head/sys/ofed/drivers/infiniband/ulp/ipoib/ipoib.h Mon Apr 30 10:22:23 2012 (r234834) > @@ -68,7 +68,6 @@ > #include > #include > #include > -#include > #endif > #ifdef INET6 > #include