Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 30 Apr 2012 13:48:36 +0200
From:      Luigi Rizzo <rizzo@iet.unipi.it>
To:        "Alexander V. Chernikov" <melifaro@FreeBSD.org>, ae@freebsd.org
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
Message-ID:  <20120430114836.GA62284@onelab2.iet.unipi.it>
In-Reply-To: <201204301022.q3UAMNcq060049@svn.freebsd.org>
References:  <201204301022.q3UAMNcq060049@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <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 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 <netinet/if_ether.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>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120430114836.GA62284>