Skip to contentSkip to conversation

RFD: <net/route.h> and struct route (trac #340)

Openvpn-devel In review Gert Doering 4 participants 14 messages Jan 13, 2014 – May 26, 2014

lore
Share & tools
lore
#0 lore raw

Hi,

there's an open bug in trac

https://community.openvpn.net/openvpn/ticket/340

that asks us to remove structures used in route.c that were copy-pasted from <net/route.h>, because OpenBSD will change some structure elements, leading to incompatibility with OpenVPN, and include <net/route.h> instead.

This makes sense, from a portability perspective, even if it needs careful testing across all supported platforms to see whether everything affected still works (this seems to primarily be the default gateway querying code, get-default_gateway()).

There is a catch to it: <net/route.h> defines a "struct route", while OpenVPN's route.h also has a "struct route", which are wildly different, so including <net/route.h> would require is to change all occurrences of "struct route" to "struct route_ipv4" or such. Which we can do, and I think it makes sense (we have "struct route_ipv6" for IPv6, so that would avoid the conflict while still aligning IPv4/IPv6 somewhat).

Now comes the catch: git master/2.4 only, or 2.3.3?

It's not exactly a bugfix, but is needed for portability, and since 2.3.x will be around a while, it would be good to have there.

Also, changing it in master and 2.3.3 would make Arne's and my life much easier in future should we have to fix a bug relating to (then) "struct openvpn_route", as we wouldn't be able to cherry-pick master->2.3 anymore otherwise...

So I'm tempted to go for "git master and 2.3.3", but want to put this up for discussion here.

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#1 lore raw

PATCH 1/2 Rename "struct route" to "struct route_ipv4"

To fix trac #340, we need to rename our "struct route" to avoid a collision with "struct route" from <net/route.h> on *BSD.

No functional changes.

---
 src/openvpn/route.c | 36 ++++++++++++++++++------------------
 src/openvpn/route.h | 10 +++++-----
 src/openvpn/tun.c   |  6 +++---
 3 files changed, 26 insertions(+), 26 deletions(-)
src/openvpn/route.c +18 -18
index f051dd3..d6344b9 100644
@@ -49,7 +49,7 @@
 #define METRIC_NOT_USED ((DWORD)-1)
 #endif
 
-static void delete_route (struct route *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es);
+static void delete_route (struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es);
 
 static void get_bypass_addresses (struct route_bypass *rb, const unsigned int flags);
 
@@ -150,7 +150,7 @@ struct route_list *
 new_route_list (const int max_routes, struct gc_arena *a)
 {
   struct route_list *ret;
-  ALLOC_VAR_ARRAY_CLEAR_GC (ret, struct route_list, struct route, max_routes, a);
+  ALLOC_VAR_ARRAY_CLEAR_GC (ret, struct route_list, struct route_ipv4, max_routes, a);
   ret->capacity = max_routes;
   return ret;
 }
@@ -165,7 +165,7 @@ new_route_ipv6_list (const int max_routes, struct gc_arena *a)
 }
 
 static const char *
-route_string (const struct route *r, struct gc_arena *gc)
+route_string (const struct route_ipv4 *r, struct gc_arena *gc)
 {
   struct buffer out = alloc_buf_gc (256, gc);
   buf_printf (&out, "ROUTE network %s netmask %s gateway %s",
@@ -267,7 +267,7 @@ is_special_addr (const char *addr_str)
 }
 
 static bool
-init_route (struct route *r,
+init_route (struct route_ipv4 *r,
 	    struct addrinfo **network_list,
 	    const struct route_option *ro,
 	    const struct route_list *rl)
@@ -484,7 +484,7 @@ void
 clear_route_list (struct route_list *rl)
 {
   const int capacity = rl->capacity;
-  const size_t rl_size = array_mult_safe (sizeof(struct route), capacity, sizeof(struct route_list));
+  const size_t rl_size = array_mult_safe (sizeof(struct route_ipv4), capacity, sizeof(struct route_list));
   memset(rl, 0, rl_size);
   rl->capacity = capacity;
 }
@@ -519,7 +519,7 @@ add_block_local_item (struct route_list *rl,
       && rl->rgi.gateway.netmask < 0xFFFFFFFF
       && (rl->n)+2 <= rl->capacity)
     {
-      struct route r;
+      struct route_ipv4 r;
       unsigned int l2;
 
       /* split a route into two smaller blocking routes, and direct them to target */
@@ -649,7 +649,7 @@ init_route_list (struct route_list *rl,
     for (i = 0; i < opt->n; ++i)
       {
         struct addrinfo* netlist;
-	struct route r;
+	struct route_ipv4 r;
 
 	if (!init_route (&r,
 			 &netlist,
@@ -760,7 +760,7 @@ add_route3 (in_addr_t network,
 	    const struct route_gateway_info *rgi,
 	    const struct env_set *es)
 {
-  struct route r;
+  struct route_ipv4 r;
   CLEAR (r);
   r.flags = RT_DEFINED;
   r.network = network;
@@ -778,7 +778,7 @@ del_route3 (in_addr_t network,
 	    const struct route_gateway_info *rgi,
 	    const struct env_set *es)
 {
-  struct route r;
+  struct route_ipv4 r;
   CLEAR (r);
   r.flags = RT_DEFINED|RT_ADDED;
   r.network = network;
@@ -1028,7 +1028,7 @@ add_routes (struct route_list *rl, struct route_ipv6_list *rl6, const struct tun
       
       for (i = 0; i < rl->n; ++i)
 	{
-	  struct route *r = &rl->routes[i];
+	  struct route_ipv4 *r = &rl->routes[i];
 	  check_subnet_conflict (r->network, r->netmask, "route");
 	  if (flags & ROUTE_DELETE_FIRST)
 	    delete_route (r, tt, flags, &rl->rgi, es);
@@ -1060,7 +1060,7 @@ delete_routes (struct route_list *rl, struct route_ipv6_list *rl6,
       int i;
       for (i = rl->n - 1; i >= 0; --i)
 	{
-	  struct route * r = &rl->routes[i];
+	  struct route_ipv4 * r = &rl->routes[i];
 	  delete_route (r, tt, flags, &rl->rgi, es);
 	}
       rl->iflags &= ~RL_ROUTES_ADDED;
@@ -1154,7 +1154,7 @@ print_default_gateway(const int msglevel, const struct route_gateway_info *rgi)
 #endif
 
 static void
-print_route (const struct route *r, int level)
+print_route (const struct route_ipv4 *r, int level)
 {
   struct gc_arena gc = gc_new ();
   if (r->flags & RT_DEFINED)
@@ -1171,7 +1171,7 @@ print_routes (const struct route_list *rl, int level)
 }
 
 static void
-setenv_route (struct env_set *es, const struct route *r, int i)
+setenv_route (struct env_set *es, const struct route_ipv4 *r, int i)
 {
   struct gc_arena gc = gc_new ();
   if (r->flags & RT_DEFINED)
@@ -1288,7 +1288,7 @@ is_on_link (const int is_local_route, const unsigned int flags, const struct rou
 }
 
 void
-add_route (struct route *r,
+add_route (struct route_ipv4 *r,
 	   const struct tuntap *tt,
 	   unsigned int flags,
 	   const struct route_gateway_info *rgi, /* may be NULL */
@@ -1741,7 +1741,7 @@ add_route_ipv6 (struct route_ipv6 *r6, const struct tuntap *tt, unsigned int fla
 }
 
 static void
-delete_route (struct route *r,
+delete_route (struct route_ipv4 *r,
 	      const struct tuntap *tt,
 	      unsigned int flags,
 	      const struct route_gateway_info *rgi,
@@ -2246,7 +2246,7 @@ get_default_gateway (struct route_gateway_info *rgi)
 }
 
 static DWORD
-windows_route_find_if_index (const struct route *r, const struct tuntap *tt)
+windows_route_find_if_index (const struct route_ipv4 *r, const struct tuntap *tt)
 {
   struct gc_arena gc = gc_new ();
   DWORD ret = TUN_ADAPTER_INDEX_INVALID;
@@ -2291,7 +2291,7 @@ windows_route_find_if_index (const struct route *r, const struct tuntap *tt)
 }
 
 bool
-add_route_ipapi (const struct route *r, const struct tuntap *tt, DWORD adapter_index)
+add_route_ipapi (const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapter_index)
 {
   struct gc_arena gc = gc_new ();
   bool ret = false;
@@ -2365,7 +2365,7 @@ add_route_ipapi (const struct route *r, const struct tuntap *tt, DWORD adapter_i
 }
 
 bool
-del_route_ipapi (const struct route *r, const struct tuntap *tt)
+del_route_ipapi (const struct route_ipv4 *r, const struct tuntap *tt)
 {
   struct gc_arena gc = gc_new ();
   bool ret = false;
src/openvpn/route.h +5 -5
index a40de32..fe9b461 100644
@@ -110,7 +110,7 @@ struct route_ipv6_option_list {
   struct route_ipv6_option routes_ipv6[EMPTY_ARRAY_SIZE];
 };
 
-struct route {
+struct route_ipv4 {
 # define RT_DEFINED        (1<<0)
 # define RT_ADDED          (1<<1)
 # define RT_METRIC_DEFINED (1<<2)
@@ -190,7 +190,7 @@ struct route_list {
   unsigned int flags;     /* RG_x flags */
   int capacity;
   int n;
-  struct route routes[EMPTY_ARRAY_SIZE];
+  struct route_ipv4 routes[EMPTY_ARRAY_SIZE];
 };
 
 #if P2MP
@@ -223,7 +223,7 @@ struct route_ipv6_list *new_route_ipv6_list (const int max_routes, struct gc_are
 void add_route_ipv6 (struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es);
 void delete_route_ipv6 (const struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es);
 
-void add_route (struct route *r,
+void add_route (struct route_ipv4 *r,
 		const struct tuntap *tt,
 		unsigned int flags,
 		const struct route_gateway_info *rgi,
@@ -301,8 +301,8 @@ void print_routes (const struct route_list *rl, int level);
 
 void show_routes (int msglev);
 bool test_routes (const struct route_list *rl, const struct tuntap *tt);
-bool add_route_ipapi (const struct route *r, const struct tuntap *tt, DWORD adapter_index);
-bool del_route_ipapi (const struct route *r, const struct tuntap *tt);
+bool add_route_ipapi (const struct route_ipv4 *r, const struct tuntap *tt, DWORD adapter_index);
+bool del_route_ipapi (const struct route_ipv4 *r, const struct tuntap *tt);
 
 #else
 static inline bool test_routes (const struct route_list *rl, const struct tuntap *tt) { return true; }
src/openvpn/tun.c +3 -3
index a300275..672486e 100644
@@ -909,7 +909,7 @@ do_ifconfig (struct tuntap *tt,
       if (!tun && tt->topology == TOP_SUBNET)
 	{
 	  /* Add a network route for the local tun interface */
-	  struct route r;
+	  struct route_ipv4 r;
 	  CLEAR (r);      
 	  r.flags = RT_DEFINED | RT_METRIC_DEFINED;
 	  r.network = tt->local & tt->remote_netmask;
@@ -1106,7 +1106,7 @@ do_ifconfig (struct tuntap *tt,
       /* Add a network route for the local tun interface */
       if (!tun && tt->topology == TOP_SUBNET)
 	{
-	  struct route r;
+	  struct route_ipv4 r;
 	  CLEAR (r);
 	  r.flags = RT_DEFINED;
 	  r.network = tt->local & tt->remote_netmask;
@@ -1172,7 +1172,7 @@ do_ifconfig (struct tuntap *tt,
 	/* Add a network route for the local tun interface */
       if (!tun && tt->topology == TOP_SUBNET)
         {
-          struct route r;
+          struct route_ipv4 r;
           CLEAR (r);
           r.flags = RT_DEFINED;
           r.network = tt->local & tt->remote_netmask;
Signature · 1 line
1.8.3.2
#2 lore raw

PATCH 2/2 Replace copied structure elements with including <net/route.h>

The code for FreeBSD, Dragonfly, OpenBSD and NetBSD contained copies of structures from <net/route.h> (struct rt_msghdr in particular).

OpenBSD changed some structure elements, making OpenVPN incompatible, depending on the specific OpenBSD version. Clean up: remove copied definitions, replace by including <net/route.h> directly - this could not be done originally due to a conflict with "struct route" in OpenVPN and <net/route.h>, cleaned up by the previous commit.

Tested on FreeBSD 9.1-RELEASE, NetBSD 5.1, OpenBSD 4.9 (route.c compiles with no warnings, and "openvpn --show-gateway" works, which is the only part of the code that uses the structures in question).

Fix trac #340

---
 src/openvpn/route.c | 95 ++---------------------------------------------------
 1 file changed, 2 insertions(+), 93 deletions(-)
src/openvpn/route.c +2 -93
index d6344b9..d397f11 100644
@@ -2611,53 +2611,7 @@ get_default_gateway (struct route_gateway_info *rgi)
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
-
-/* all of this is taken from <net/route.h> in FreeBSD */
-#define RTA_DST     0x1
-#define RTA_GATEWAY 0x2
-#define RTA_NETMASK 0x4
-
-#define RTM_GET     0x4
-#define RTM_VERSION 5
-
-#define RTF_UP      0x1
-#define RTF_GATEWAY 0x2
-
-/*
- * These numbers are used by reliable protocols for determining
- * retransmission behavior and are included in the routing structure.
- */
-struct rt_metrics {
-        u_long  rmx_locks;      /* Kernel must leave these values alone */
-        u_long  rmx_mtu;        /* MTU for this path */
-        u_long  rmx_hopcount;   /* max hops expected */
-        u_long  rmx_expire;     /* lifetime for route, e.g. redirect */
-        u_long  rmx_recvpipe;   /* inbound delay-bandwidth product */
-        u_long  rmx_sendpipe;   /* outbound delay-bandwidth product */
-        u_long  rmx_ssthresh;   /* outbound gateway buffer limit */
-        u_long  rmx_rtt;        /* estimated round trip time */
-        u_long  rmx_rttvar;     /* estimated rtt variance */
-        u_long  rmx_pksent;     /* packets sent using this route */
-        u_long  rmx_filler[4];  /* will be used for T/TCP later */
-};
-
-/*
- * Structures for routing messages.
- */
-struct rt_msghdr {
-        u_short rtm_msglen;     /* to skip over non-understood messages */
-        u_char  rtm_version;    /* future binary compatibility */
-        u_char  rtm_type;       /* message type */
-        u_short rtm_index;      /* index for associated ifp */
-        int     rtm_flags;      /* flags, incl. kern & message, e.g. DONE */
-        int     rtm_addrs;      /* bitmask identifying sockaddrs in msg */
-        pid_t   rtm_pid;        /* identify sender */
-        int     rtm_seq;        /* for sender to identify action */
-        int     rtm_errno;      /* why failed */
-        int     rtm_use;        /* from rtentry */
-        u_long  rtm_inits;      /* which metrics we are initializing */
-        struct  rt_metrics rtm_rmx; /* metrics themselves */
-};
+#include <net/route.h>
 
 struct {
   struct rt_msghdr m_rtm;
@@ -2976,52 +2930,7 @@ get_default_gateway (struct route_gateway_info *rgi)
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
-
-/* all of this is taken from <net/route.h> in OpenBSD 3.6 */
-#define RTA_DST		0x1	/* destination sockaddr present */
-#define RTA_GATEWAY	0x2	/* gateway sockaddr present */
-#define RTA_NETMASK	0x4	/* netmask sockaddr present */
-
-#define RTM_GET		0x4	/* Report Metrics */
-
-#define RTM_VERSION	3	/* Up the ante and ignore older versions */
-
-#define	RTF_UP		0x1		/* route usable */
-#define	RTF_GATEWAY	0x2		/* destination is a gateway */
-
-/*
- * Huge version for userland compatibility.
- */
-struct rt_metrics {
-	u_long	rmx_locks;	/* Kernel must leave these values alone */
-	u_long	rmx_mtu;	/* MTU for this path */
-	u_long	rmx_hopcount;	/* max hops expected */
-	u_long	rmx_expire;	/* lifetime for route, e.g. redirect */
-	u_long	rmx_recvpipe;	/* inbound delay-bandwidth product */
-	u_long	rmx_sendpipe;	/* outbound delay-bandwidth product */
-	u_long	rmx_ssthresh;	/* outbound gateway buffer limit */
-	u_long	rmx_rtt;	/* estimated round trip time */
-	u_long	rmx_rttvar;	/* estimated rtt variance */
-	u_long	rmx_pksent;	/* packets sent using this route */
-};
-
-/*
- * Structures for routing messages.
- */
-struct rt_msghdr {
-	u_short	rtm_msglen;	/* to skip over non-understood messages */
-	u_char	rtm_version;	/* future binary compatibility */
-	u_char	rtm_type;	/* message type */
-	u_short	rtm_index;	/* index for associated ifp */
-	int	rtm_flags;	/* flags, incl. kern & message, e.g. DONE */
-	int	rtm_addrs;	/* bitmask identifying sockaddrs in msg */
-	pid_t	rtm_pid;	/* identify sender */
-	int	rtm_seq;	/* for sender to identify action */
-	int	rtm_errno;	/* why failed */
-	int	rtm_use;	/* from rtentry */
-	u_long	rtm_inits;	/* which metrics we are initializing */
-	struct	rt_metrics rtm_rmx; /* metrics themselves */
-};
+#include <net/route.h>
 
 struct {
   struct rt_msghdr m_rtm;
Signature · 1 line
1.8.3.2
#3 lore raw

Re: [Openvpn-devel] [PATCH 2/2] Replace copied structure elements with including <net/route.h>

Am 13.01.14 22:54, schrieb Gert Doering:

Quoted 9 lines

The code for FreeBSD, Dragonfly, OpenBSD and NetBSD contained copies of structures from <net/route.h> (struct rt_msghdr in particular).

OpenBSD changed some structure elements, making OpenVPN incompatible, depending on the specific OpenBSD version. Clean up: remove copied definitions, replace by including <net/route.h> directly - this could not be done originally due to a conflict with "struct route" in OpenVPN and <net/route.h>, cleaned up by the previous commit.

Quoted 6 lines

Tested on FreeBSD 9.1-RELEASE, NetBSD 5.1, OpenBSD 4.9 (route.c compiles with no warnings, and "openvpn --show-gateway" works, which is the only part of the code that uses the structures in question).

Fix trac #340

ACK.

Arne

#4 lore raw

[Openvpn-devel] [PATCH applied] Re: Replace copied structure elements with including <net/route.h>

In reply to Gert Doering

Patch has been applied to the master and release/2.3 branches.

commit 615fb9ef36310f85fd6171301128a12740444455 (master) commit 15e0d0f5674bfe9a2de17095c6273b15b0eeec22 (release/2.3)

Author: Gert Doering
Date:   Mon Jan 13 22:54:34 2014 +0100
     Replace copied structure elements with including <net/route.h>

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/8230

--
kind regards,

Gert Doering

#5 lore raw

Re: [Openvpn-devel] [PATCH 1/2] Rename "struct route" to "struct route_ipv4"

In reply to Gert Doering

Am 13.01.14 22:54, schrieb Gert Doering:

Quoted 5 lines

To fix trac #340, we need to rename our "struct route" to avoid a collision with "struct route" from <net/route.h> on *BSD.

ACK.

Arne

#6 lore raw

[Openvpn-devel] [PATCH applied] Re: Rename 'struct route' to 'struct route_ipv4'

In reply to Gert Doering

Patch has been applied to the master and release/2.3 branches.

commit b57e005b8f232760081875937a53e8e7d235faa6 (master) commit 07b5c1d02579b5e897a0e5e0c80f0ba0098220d7 (release/2.3)

Author: Gert Doering
Date:   Mon Jan 13 22:54:33 2014 +0100
     Rename 'struct route' to 'struct route_ipv4'

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/8231

--
kind regards,

Gert Doering

#7 lore raw
In reply to Gert Doering
Quoted 28 lines · Gert Doering

Hi,

there's an open bug in trac

https://community.openvpn.net/openvpn/ticket/340

that asks us to remove structures used in route.c that were copy-pasted from <net/route.h>, because OpenBSD will change some structure elements, leading to incompatibility with OpenVPN, and include <net/route.h> instead.

This makes sense, from a portability perspective, even if it needs careful testing across all supported platforms to see whether everything affected still works (this seems to primarily be the default gateway querying code, get-default_gateway()).

There is a catch to it: <net/route.h> defines a "struct route", while OpenVPN's route.h also has a "struct route", which are wildly different, so including <net/route.h> would require is to change all occurrences of "struct route" to "struct route_ipv4" or such. Which we can do, and I think it makes sense (we have "struct route_ipv6" for IPv6, so that would avoid the conflict while still aligning IPv4/IPv6 somewhat).

Now comes the catch: git master/2.4 only, or 2.3.3?

It's not exactly a bugfix, but is needed for portability, and since 2.3.x will be around a while, it would be good to have there.

Quoted 8 lines

Also, changing it in master and 2.3.3 would make Arne's and my life much easier in future should we have to fix a bug relating to (then) "struct openvpn_route", as we wouldn't be able to cherry-pick master->2.3 anymore otherwise...

So I'm tempted to go for "git master and 2.3.3", but want to put this up for discussion here.

Generally speaking, I'd prefer as few changes to the 2.3 branch as possible. Try to let it be bugfixes mostly. This is also to encourage users to move over to 2.4 ASAP when that's ready, with more new features. It also helps stability and maintainability within the OpenVPN project.

Arne's OpenVPN for Android seems to always have been more experimental and bleeding edge than OpenVPN Connect. So unless Arne screams back at me telling me I'm wrong, I believe Arne will be one of the first ones to move over to the 2.4/master codebase, thus not making things that difficult for him. But Arne is the one who can fully answer this. If he says it will be more complicated, then let it go into 2.3 too.

Having all that said. Looking at your patches, they seem clean enough and I'm not terrified by it. I'd expect compile errors from our buildslaves if this breaks something.

So I'd vote for master instantly. And then 2.3 if really needed, after it has been tested in master for a little while.

Signature · 3 lines
kind regards,

David Sommerseth
#8 lore raw
Quoted 6 lines

Generally speaking, I'd prefer as few changes to the 2.3 branch as possible. Try to let it be bugfixes mostly. This is also to encourage users to move over to 2.4 ASAP when that's ready, with more new features. It also helps stability and maintainability within the OpenVPN project.

Quoted 6 lines

Arne's OpenVPN for Android seems to always have been more experimental and bleeding edge than OpenVPN Connect. So unless Arne screams back at me telling me I'm wrong, I believe Arne will be one of the first ones to move over to the 2.4/master codebase, thus not making things that difficult for him. But Arne is the one who can fully answer this. If he says it will be more complicated, then let it go into 2.3 too.

I am already on 2.4/master codebase. I might consider tracking release/2.4 in the future but definitively not release/2.3. (With all the bugfixes/patches I need/want on Android it would be almost -master anyway).

I have not really an opinoion about 2.3 and what should go in there. At the moment it feels like release/2.3 is a "baby" master since almost all changes are cherry picked into it.

Arne

#9 lore raw

Hi,

Quoted 3 lines · Arne Schwabe

I have not really an opinoion about 2.3 and what should go in there. At the moment it feels like release/2.3 is a "baby" master since almost all changes are cherry picked into it.

My current "rules" for application to release/2.3 are:

- obvious bugfixes
- changes important for long-term usability of 2.3
(I think the main examples are TLS version negotiation & PolarSSL 1.2)

what does not get into there is

- fundamental rewrites (dual-stack patches, preresolve patch)
- large new functionality (the android patchset, the windows service pipe, ...)

so there *is* a difference :-) - but as long as we're mostly committing bugfixes, it indeed looks as if everything goes into both branches.

These days, there's "lots of bugfixes", while I'm working my way through trac...

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#10 lore raw

Hi,

Quoted 3 lines · Gert Doering

My current "rules" for application to release/2.3 are:
- changes important for long-term usability of 2.3
(I think the main examples are TLS version negotiation & PolarSSL 1.2)

I'd say this patch qualifies for release/2.3 according to this rule (and I agree with the rule), so I vote for applying this to release/2.3 too.

-Steffan

#11 lore raw

[Openvpn-devel] [PATCH 3/2] Merge get_default_gateway() implementation for all 4+1 BSD variants.

In reply to Gert Doering

This was 3 times mostly the same code, with the 4 traditional BSDs only differing in a single line, while MacOS X had more refined code to do not only gateway determination, but also netmask, interface name and MAC address.

The MacOS X code works perfectly well on the other BSDs *if* one macro is #ifdef'ed to use "(uint32_t)" on MacOS X and "(long)" on all other BSDs, 32 and 64 bit variants. API change by OSX when going to 64bit.

Tested on FreeBSD 8.3/i386, FreeBSD 9.1/amd64, NetbSD 5.1/amd64, OpenBSD 4.9/i386, OpenBSD 5.4/amd64 (= all affected platforms except DragonFly BSD).

See also trac#42 and trac#340.

---
 src/openvpn/route.c | 246 +++++-----------------------------------------------
 1 file changed, 21 insertions(+), 225 deletions(-)
src/openvpn/route.c +21 -225
index d397f11..25c81b9 100644
@@ -2606,118 +2606,9 @@ get_default_gateway (struct route_gateway_info *rgi)
   gc_free (&gc);
 }
 
-#elif defined(TARGET_FREEBSD)||defined(TARGET_DRAGONFLY)
-
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <net/route.h>
-
-struct {
-  struct rt_msghdr m_rtm;
-  char       m_space[512];
-} m_rtmsg;
-
-#define ROUNDUP(a) \
-        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
-
-/*
- * FIXME -- add support for netmask, hwaddr, and iface
- */
-void
-get_default_gateway (struct route_gateway_info *rgi)
-{
-  struct gc_arena gc = gc_new ();
-  int s, seq, l, pid, rtm_addrs, i;
-  struct sockaddr so_dst, so_mask;
-  char *cp = m_rtmsg.m_space; 
-  struct sockaddr *gate = NULL, *sa;
-  struct  rt_msghdr *rtm_aux;
-
-#define NEXTADDR(w, u) \
-        if (rtm_addrs & (w)) {\
-            l = ROUNDUP(u.sa_len); memmove(cp, &(u), l); cp += l;\
-        }
-
-#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
-
-#define rtm m_rtmsg.m_rtm
-
-  CLEAR(*rgi);
-
-  pid = getpid();
-  seq = 0;
-  rtm_addrs = RTA_DST | RTA_NETMASK;
-
-  bzero(&so_dst, sizeof(so_dst));
-  bzero(&so_mask, sizeof(so_mask));
-  bzero(&rtm, sizeof(struct rt_msghdr));
-
-  rtm.rtm_type = RTM_GET;
-  rtm.rtm_flags = RTF_UP | RTF_GATEWAY;
-  rtm.rtm_version = RTM_VERSION;
-  rtm.rtm_seq = ++seq;
-  rtm.rtm_addrs = rtm_addrs; 
-
-  so_dst.sa_family = AF_INET;
-  so_dst.sa_len = sizeof(struct sockaddr_in);
-  so_mask.sa_family = AF_INET;
-  so_mask.sa_len = sizeof(struct sockaddr_in);
-
-  NEXTADDR(RTA_DST, so_dst);
-  NEXTADDR(RTA_NETMASK, so_mask);
-
-  rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;
-
-  s = socket(PF_ROUTE, SOCK_RAW, 0);
-
-  if (write(s, (char *)&m_rtmsg, l) < 0)
-    {
-      msg(M_WARN|M_ERRNO, "Could not retrieve default gateway from route socket:");
-      gc_free (&gc);
-      close(s);
-      return;
-    }
-
-  do {
-    l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-  } while (l > 0 && (rtm.rtm_seq != seq || rtm.rtm_pid != pid));
-                        
-  close(s);
-
-  rtm_aux = &rtm;
-
-  cp = ((char *)(rtm_aux + 1));
-  if (rtm_aux->rtm_addrs) {
-    for (i = 1; i; i <<= 1)
-      if (i & rtm_aux->rtm_addrs) {
-	sa = (struct sockaddr *)cp;
-	if (i == RTA_GATEWAY )
-	  gate = sa;
-	ADVANCE(cp, sa);
-      }
-  }
-  else
-    {
-      gc_free (&gc);
-      return;
-    }
-
-
-  if (gate != NULL )
-    {
-      rgi->gateway.addr = ntohl(((struct sockaddr_in *)gate)->sin_addr.s_addr);
-      rgi->flags |= RGI_ADDR_DEFINED;
-
-      gc_free (&gc);
-    }
-  else
-    {
-      gc_free (&gc);
-    }
-}
-
-#elif defined(TARGET_DARWIN)
+#elif defined(TARGET_DARWIN) || \
+	defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) || \
+	defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -2730,8 +2621,25 @@ struct rtmsg {
   char       m_space[512];
 };
 
-#define ROUNDUP(a) \
+/* the route socket code is identical for all 4 supported BSDs and for
+ * MacOS X (Darwin), with one crucial difference: when going from
+ * 32 bit to 64 bit, the BSDs increased the structure size but kept
+ * source code compatibility by keeping the use of "long", while
+ * MacOS X decided to keep binary compatibility by *changing* the API
+ * to use "uint32_t", thus 32 bit on all OS X variants
+ *
+ * We used to have a large amount of duplicate code here which really
+ * differed only in this (long) vs. (uint32_t) - IMHO, worse than
+ * having a combined block for all BSDs with this single #ifdef inside
+ */
+
+#if defined(TARGET_DARWIN)
+# define ROUNDUP(a) \
         ((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t))
+#else
+# define ROUNDUP(a) \
+        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
+#endif
 
 #define NEXTADDR(w, u) \
         if (rtm_addrs & (w)) {\
@@ -2925,118 +2833,6 @@ get_default_gateway (struct route_gateway_info *rgi)
 
 #undef max
 
-#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
-
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <net/route.h>
-
-struct {
-  struct rt_msghdr m_rtm;
-  char       m_space[512];
-} m_rtmsg;
-
-#define ROUNDUP(a) \
-        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
-
-/*
- * FIXME -- add support for netmask, hwaddr, and iface
- */
-void
-get_default_gateway (struct route_gateway_info *rgi)
-{
-  struct gc_arena gc = gc_new ();
-  int s, seq, l, rtm_addrs, i;
-  pid_t pid;
-  struct sockaddr so_dst, so_mask;
-  char *cp = m_rtmsg.m_space; 
-  struct sockaddr *gate = NULL, *sa;
-  struct  rt_msghdr *rtm_aux;
-
-#define NEXTADDR(w, u) \
-        if (rtm_addrs & (w)) {\
-            l = ROUNDUP(u.sa_len); memmove(cp, &(u), l); cp += l;\
-        }
-
-#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
-
-#define rtm m_rtmsg.m_rtm
-
-  CLEAR(*rgi);
-
-  pid = getpid();
-  seq = 0;
-  rtm_addrs = RTA_DST | RTA_NETMASK;
-
-  bzero(&so_dst, sizeof(so_dst));
-  bzero(&so_mask, sizeof(so_mask));
-  bzero(&rtm, sizeof(struct rt_msghdr));
-
-  rtm.rtm_type = RTM_GET;
-  rtm.rtm_flags = RTF_UP | RTF_GATEWAY;
-  rtm.rtm_version = RTM_VERSION;
-  rtm.rtm_seq = ++seq;
-  rtm.rtm_addrs = rtm_addrs; 
-
-  so_dst.sa_family = AF_INET;
-  so_dst.sa_len = sizeof(struct sockaddr_in);
-  so_mask.sa_family = AF_INET;
-  so_mask.sa_len = sizeof(struct sockaddr_in);
-
-  NEXTADDR(RTA_DST, so_dst);
-  NEXTADDR(RTA_NETMASK, so_mask);
-
-  rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;
-
-  s = socket(PF_ROUTE, SOCK_RAW, 0);
-
-  if (write(s, (char *)&m_rtmsg, l) < 0)
-    {
-      msg(M_WARN|M_ERRNO, "Could not retrieve default gateway from route socket:");
-      gc_free (&gc);
-      close(s);
-      return;
-    }
-
-  do {
-    l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-  } while (l > 0 && (rtm.rtm_seq != seq || rtm.rtm_pid != pid));
-                        
-  close(s);
-
-  rtm_aux = &rtm;
-
-  cp = ((char *)(rtm_aux + 1));
-  if (rtm_aux->rtm_addrs) {
-    for (i = 1; i; i <<= 1)
-      if (i & rtm_aux->rtm_addrs) {
-	sa = (struct sockaddr *)cp;
-	if (i == RTA_GATEWAY )
-	  gate = sa;
-	ADVANCE(cp, sa);
-      }
-  }
-  else
-    {
-      gc_free (&gc);
-      return;
-    }
-
-
-  if (gate != NULL )
-    {
-      rgi->gateway.addr = ntohl(((struct sockaddr_in *)gate)->sin_addr.s_addr);
-      rgi->flags |= RGI_ADDR_DEFINED;
-
-      gc_free (&gc);
-    }
-  else
-    {
-      gc_free (&gc);
-    }
-}
-
 #else
 
 /*
Signature · 1 line
1.8.3.2
#12 lore raw

Re: [Openvpn-devel] [PATCH 3/2] Merge get_default_gateway() implementation for all 4+1 BSD variants.

Hi,

bringing this one back into people's memory. I just rediscovered this today while going through my own list of not-yet-merged patches - and since this is

- less code
- less duplicate code
- more functionality (due to "duplicated code was missing more recent bits")
- tested on all the affected platforms

I'm going to apply lazy-ack rules "soon". Unless someone else finds time to review and ACK :-)

gert

Quoted 295 lines · Gert Doering

This was 3 times mostly the same code, with the 4 traditional BSDs only differing in a single line, while MacOS X had more refined code to do not only gateway determination, but also netmask, interface name and MAC address.

The MacOS X code works perfectly well on the other BSDs *if* one macro is #ifdef'ed to use "(uint32_t)" on MacOS X and "(long)" on all other BSDs, 32 and 64 bit variants. API change by OSX when going to 64bit.

Tested on FreeBSD 8.3/i386, FreeBSD 9.1/amd64, NetbSD 5.1/amd64, OpenBSD 4.9/i386, OpenBSD 5.4/amd64 (= all affected platforms except DragonFly BSD).

See also trac#42 and trac#340.

---
 src/openvpn/route.c | 246 +++++-----------------------------------------------
 1 file changed, 21 insertions(+), 225 deletions(-)
src/openvpn/route.c +21 -225
index d397f11..25c81b9 100644
@@ -2606,118 +2606,9 @@ get_default_gateway (struct route_gateway_info *rgi)
   gc_free (&gc);
 }
 
-#elif defined(TARGET_FREEBSD)||defined(TARGET_DRAGONFLY)
-
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <net/route.h>
-
-struct {
-  struct rt_msghdr m_rtm;
-  char       m_space[512];
-} m_rtmsg;
-
-#define ROUNDUP(a) \
-        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
-
-/*
- * FIXME -- add support for netmask, hwaddr, and iface
- */
-void
-get_default_gateway (struct route_gateway_info *rgi)
-{
-  struct gc_arena gc = gc_new ();
-  int s, seq, l, pid, rtm_addrs, i;
-  struct sockaddr so_dst, so_mask;
-  char *cp = m_rtmsg.m_space; 
-  struct sockaddr *gate = NULL, *sa;
-  struct  rt_msghdr *rtm_aux;
-
-#define NEXTADDR(w, u) \
-        if (rtm_addrs & (w)) {\
-            l = ROUNDUP(u.sa_len); memmove(cp, &(u), l); cp += l;\
-        }
-
-#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
-
-#define rtm m_rtmsg.m_rtm
-
-  CLEAR(*rgi);
-
-  pid = getpid();
-  seq = 0;
-  rtm_addrs = RTA_DST | RTA_NETMASK;
-
-  bzero(&so_dst, sizeof(so_dst));
-  bzero(&so_mask, sizeof(so_mask));
-  bzero(&rtm, sizeof(struct rt_msghdr));
-
-  rtm.rtm_type = RTM_GET;
-  rtm.rtm_flags = RTF_UP | RTF_GATEWAY;
-  rtm.rtm_version = RTM_VERSION;
-  rtm.rtm_seq = ++seq;
-  rtm.rtm_addrs = rtm_addrs; 
-
-  so_dst.sa_family = AF_INET;
-  so_dst.sa_len = sizeof(struct sockaddr_in);
-  so_mask.sa_family = AF_INET;
-  so_mask.sa_len = sizeof(struct sockaddr_in);
-
-  NEXTADDR(RTA_DST, so_dst);
-  NEXTADDR(RTA_NETMASK, so_mask);
-
-  rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;
-
-  s = socket(PF_ROUTE, SOCK_RAW, 0);
-
-  if (write(s, (char *)&m_rtmsg, l) < 0)
-    {
-      msg(M_WARN|M_ERRNO, "Could not retrieve default gateway from route socket:");
-      gc_free (&gc);
-      close(s);
-      return;
-    }
-
-  do {
-    l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-  } while (l > 0 && (rtm.rtm_seq != seq || rtm.rtm_pid != pid));
-                        
-  close(s);
-
-  rtm_aux = &rtm;
-
-  cp = ((char *)(rtm_aux + 1));
-  if (rtm_aux->rtm_addrs) {
-    for (i = 1; i; i <<= 1)
-      if (i & rtm_aux->rtm_addrs) {
-	sa = (struct sockaddr *)cp;
-	if (i == RTA_GATEWAY )
-	  gate = sa;
-	ADVANCE(cp, sa);
-      }
-  }
-  else
-    {
-      gc_free (&gc);
-      return;
-    }
-
-
-  if (gate != NULL )
-    {
-      rgi->gateway.addr = ntohl(((struct sockaddr_in *)gate)->sin_addr.s_addr);
-      rgi->flags |= RGI_ADDR_DEFINED;
-
-      gc_free (&gc);
-    }
-  else
-    {
-      gc_free (&gc);
-    }
-}
-
-#elif defined(TARGET_DARWIN)
+#elif defined(TARGET_DARWIN) || \
+	defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) || \
+	defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -2730,8 +2621,25 @@ struct rtmsg {
   char       m_space[512];
 };
 
-#define ROUNDUP(a) \
+/* the route socket code is identical for all 4 supported BSDs and for
+ * MacOS X (Darwin), with one crucial difference: when going from
+ * 32 bit to 64 bit, the BSDs increased the structure size but kept
+ * source code compatibility by keeping the use of "long", while
+ * MacOS X decided to keep binary compatibility by *changing* the API
+ * to use "uint32_t", thus 32 bit on all OS X variants
+ *
+ * We used to have a large amount of duplicate code here which really
+ * differed only in this (long) vs. (uint32_t) - IMHO, worse than
+ * having a combined block for all BSDs with this single #ifdef inside
+ */
+
+#if defined(TARGET_DARWIN)
+# define ROUNDUP(a) \
         ((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t))
+#else
+# define ROUNDUP(a) \
+        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
+#endif
 
 #define NEXTADDR(w, u) \
         if (rtm_addrs & (w)) {\
@@ -2925,118 +2833,6 @@ get_default_gateway (struct route_gateway_info *rgi)
 
 #undef max
 
-#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
-
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <net/route.h>
-
-struct {
-  struct rt_msghdr m_rtm;
-  char       m_space[512];
-} m_rtmsg;
-
-#define ROUNDUP(a) \
-        ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
-
-/*
- * FIXME -- add support for netmask, hwaddr, and iface
- */
-void
-get_default_gateway (struct route_gateway_info *rgi)
-{
-  struct gc_arena gc = gc_new ();
-  int s, seq, l, rtm_addrs, i;
-  pid_t pid;
-  struct sockaddr so_dst, so_mask;
-  char *cp = m_rtmsg.m_space; 
-  struct sockaddr *gate = NULL, *sa;
-  struct  rt_msghdr *rtm_aux;
-
-#define NEXTADDR(w, u) \
-        if (rtm_addrs & (w)) {\
-            l = ROUNDUP(u.sa_len); memmove(cp, &(u), l); cp += l;\
-        }
-
-#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
-
-#define rtm m_rtmsg.m_rtm
-
-  CLEAR(*rgi);
-
-  pid = getpid();
-  seq = 0;
-  rtm_addrs = RTA_DST | RTA_NETMASK;
-
-  bzero(&so_dst, sizeof(so_dst));
-  bzero(&so_mask, sizeof(so_mask));
-  bzero(&rtm, sizeof(struct rt_msghdr));
-
-  rtm.rtm_type = RTM_GET;
-  rtm.rtm_flags = RTF_UP | RTF_GATEWAY;
-  rtm.rtm_version = RTM_VERSION;
-  rtm.rtm_seq = ++seq;
-  rtm.rtm_addrs = rtm_addrs; 
-
-  so_dst.sa_family = AF_INET;
-  so_dst.sa_len = sizeof(struct sockaddr_in);
-  so_mask.sa_family = AF_INET;
-  so_mask.sa_len = sizeof(struct sockaddr_in);
-
-  NEXTADDR(RTA_DST, so_dst);
-  NEXTADDR(RTA_NETMASK, so_mask);
-
-  rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;
-
-  s = socket(PF_ROUTE, SOCK_RAW, 0);
-
-  if (write(s, (char *)&m_rtmsg, l) < 0)
-    {
-      msg(M_WARN|M_ERRNO, "Could not retrieve default gateway from route socket:");
-      gc_free (&gc);
-      close(s);
-      return;
-    }
-
-  do {
-    l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-  } while (l > 0 && (rtm.rtm_seq != seq || rtm.rtm_pid != pid));
-                        
-  close(s);
-
-  rtm_aux = &rtm;
-
-  cp = ((char *)(rtm_aux + 1));
-  if (rtm_aux->rtm_addrs) {
-    for (i = 1; i; i <<= 1)
-      if (i & rtm_aux->rtm_addrs) {
-	sa = (struct sockaddr *)cp;
-	if (i == RTA_GATEWAY )
-	  gate = sa;
-	ADVANCE(cp, sa);
-      }
-  }
-  else
-    {
-      gc_free (&gc);
-      return;
-    }
-
-
-  if (gate != NULL )
-    {
-      rgi->gateway.addr = ntohl(((struct sockaddr_in *)gate)->sin_addr.s_addr);
-      rgi->flags |= RGI_ADDR_DEFINED;
-
-      gc_free (&gc);
-    }
-  else
-    {
-      gc_free (&gc);
-    }
-}
-
 #else
 
 /*
Signature · 1 line
1.8.3.2
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#13 lore raw

[Openvpn-devel] [PATCH applied] Re: Merge get_default_gateway() implementation for all 4+1 BSD variants.

In reply to Gert Doering

Lazy-ACK rules applied. Patch has been applied to the master branch.

commit cab6305be749930eae9c4a1348fb90c4c515d8d7

Author: Gert Doering
Date:   Tue Jan 14 14:33:18 2014 +0100
     Merge get_default_gateway() implementation for all 4+1 BSD variants.

     Signed-off-by: Gert Doering <[email protected]...>
     Lazy-Ack-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/8233

--
kind regards,

Gert Doering