Re: [PATCH] [1/3] Use libnl instead of iproute/AF_PACKET (nm_system_device_add_route_via_device_with_iface)

[Dan Williams <dcbw redhat com>]

On Fri, 2008-04-18 at 20:10 -0400, Benoit Boissinot wrote:
> (depends on the cleanup of the frugalware backend)
> 
> Remove nm_system_device_add_ip4_route_via_device_with_iface from all
> backends, implement it with libnl.
> Removes validate_ip4_route since we don't call system anymore

Committed, thanks!

Dan

> diff -r 630520f22ebb src/NetworkManagerSystem.c
> --- a/src/NetworkManagerSystem.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/NetworkManagerSystem.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -52,7 +52,11 @@
>  #include "nm-utils.h"
>  #include "nm-netlink.h"
>  
> +/* Because of a bug in libnl, rtnl.h should be included before route.h */
> +#include <netlink/route/rtnl.h>
> +
>  #include <netlink/route/addr.h>
> +#include <netlink/route/route.h>
>  #include <netlink/netlink.h>
>  #include <netlink/utils.h>
>  #include <netlink/route/link.h>
> @@ -284,83 +288,6 @@
>  	return success;
>  }
>  
> -
> -/*
> - * validate_ip4_route
> - *
> - * Ensure that IP4 routes are in the correct format
> - *
> - */
> -static char *validate_ip4_route (const char *route)
> -{
> -	char *		ret = NULL;
> -	char *		temp = NULL;
> -	int			slash_pos = -1;
> -	char *		p = NULL;
> -	int			len, i;
> -	int			dot_count = 0;
> -	gboolean		have_slash = FALSE;
> -	struct in_addr	addr;
> -
> -	g_return_val_if_fail (route != NULL, NULL);
> -
> -	len = strlen (route);
> -	/* Minimum length, ie 1.1.1.1/8 */
> -	if (len < 9)
> -		return NULL;
> -
> -	for (i = 0; i < len; i++)
> -	{
> -		/* Ensure there is only one slash */
> -		if (route[i] == '/')
> -		{
> -			if (have_slash)
> -				goto out;
> -
> -			have_slash = TRUE;
> -			slash_pos = i;
> -			continue;
> -		}
> -
> -		if (route[i] == '.')
> -		{
> -			if (dot_count >= 4)
> -				goto out;
> -
> -			dot_count++;
> -			continue;
> -		}
> -
> -		if (!isdigit (route[i]))
> -			goto out;
> -	}
> -
> -	/* Make sure there is at least one slash and 3 dots */
> -	if (!have_slash || !slash_pos || (dot_count != 3))
> -		goto out;
> -
> -	/* Valid IP address part */
> -	temp = g_strdup (route);
> -	temp[slash_pos] = '\0';
> -	memset (&addr, 0, sizeof (struct in_addr));
> -	if (inet_aton (temp, &addr) == 0)
> -		goto out;
> -
> -	/* Ensure the network # is valid */
> -	p = temp + slash_pos + 1;
> -	i = (int) strtol (p, NULL, 10);
> -	if ((i < 0) || (i > 32))
> -		goto out;
> -
> -	/* Success! */
> -	ret = g_strdup (route);
> -
> -out:
> -	g_free (temp);
> -	return ret;
> -}
> -
> -
>  /*
>   * nm_system_vpn_device_set_from_ip4_config
>   *
> @@ -424,22 +351,8 @@
>  	} else {
>  		GSList *iter;
>  
> -		for (iter = routes; iter; iter = iter->next) {
> -			char *valid_ip4_route;
> -
> -			/* Make sure the route is valid, otherwise it's a security risk as the route
> -			 * text is simply taken from the user, and passed directly to system().  If
> -			 * we did not check the route, think of:
> -			 *
> -			 *     system("/sbin/ip route add `rm -rf /` dev eth0")
> -			 *
> -			 * where `rm -rf /` was the route text.  As UID 0 (root), we have to be careful.
> -			 */
> -			if ((valid_ip4_route = validate_ip4_route ((char *) iter->data))) {
> -				nm_system_device_add_route_via_device_with_iface (iface, valid_ip4_route);
> -				g_free (valid_ip4_route);
> -			}
> -		}
> +		for (iter = routes; iter; iter = iter->next)
> +			nm_system_device_add_ip4_route_via_device_with_iface (iface, (char *) iter->data);
>  	}
>  
>  out:
> @@ -553,5 +466,37 @@
>  	return success;
>  }
>  
> +/*
> + * nm_system_device_add_ip4_route_via_device_with_iface
> + *
> + * Add route to the given device
> + *
> + */
> +void nm_system_device_add_ip4_route_via_device_with_iface (const char *iface, const char *addr)
> +{
> +	struct rtnl_route * route;
> +	struct nl_handle  * nlh;
> +	struct nl_addr    * dst;
> +	int iface_idx;
>  
> +	nlh = nm_netlink_get_default_handle ();
> +	g_return_if_fail (nlh != NULL);
>  
> +	route = rtnl_route_alloc ();
> +	g_return_if_fail (route != NULL);
> +
> +	iface_idx = nm_netlink_iface_to_index (iface);
> +	if (iface_idx < 0)
> +		goto out;
> +	rtnl_route_set_oif (route, iface_idx);
> +
> +	if (!(dst = nl_addr_parse (addr, AF_INET)))
> +		goto out;
> +	rtnl_route_set_dst (route, dst);
> +	nl_addr_put (dst);
> +
> +	rtnl_route_add (nlh, route, 0);
> +out:
> +	rtnl_route_put (route);
> +}
> +
> diff -r 630520f22ebb src/NetworkManagerSystem.h
> --- a/src/NetworkManagerSystem.h	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/NetworkManagerSystem.h	Wed Apr 16 00:35:47 2008 -0400
> @@ -42,7 +42,7 @@
>                                                               guint32 gw,
>                                                               guint32 mss);
>  
> -void			nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route);
> +void			nm_system_device_add_ip4_route_via_device_with_iface (const char *iface, const char *route);
>  
>  void			nm_system_device_flush_ip4_addresses			(NMDevice *dev);
>  void			nm_system_device_flush_ip4_addresses_with_iface	(const char *iface);
> diff -r 630520f22ebb src/backends/NetworkManagerArch.c
> --- a/src/backends/NetworkManagerArch.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerArch.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -80,18 +80,6 @@
>  {
>  	nm_generic_device_replace_default_route (iface, gw, mss);
>  }
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_device_flush_ip4_addresses
> diff -r 630520f22ebb src/backends/NetworkManagerDebian.c
> --- a/src/backends/NetworkManagerDebian.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerDebian.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -71,18 +71,6 @@
>  }
>  
>  /*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
> -
> -/*
>   * nm_system_device_flush_ip4_addresses
>   *
>   * Flush all network addresses associated with a network device
> diff -r 630520f22ebb src/backends/NetworkManagerFrugalware.c
> --- a/src/backends/NetworkManagerFrugalware.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerFrugalware.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -160,18 +160,6 @@
>  		nm_spawn_process ("/etc/rc.d/rc.avahi-daemon restart");
>  	}
>  }
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_device_replace_default_route
> diff -r 630520f22ebb src/backends/NetworkManagerGeneric.c
> --- a/src/backends/NetworkManagerGeneric.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerGeneric.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -85,25 +85,6 @@
>  }
>  
>  /*
> - * nm_generic_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_generic_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	char	*buf;
> -
> -	g_return_if_fail (iface != NULL);
> -
> -	/* Add default gateway */
> -	buf = g_strdup_printf (IP_BINARY_PATH" route add %s dev %s", route, iface);
> -	nm_spawn_process (buf);
> -	g_free (buf);
> -}
> -
> -
> -/*
>   * nm_generic_device_flush_ip4_addresses
>   *
>   * Flush all network addresses associated with a network device
> diff -r 630520f22ebb src/backends/NetworkManagerGeneric.h
> --- a/src/backends/NetworkManagerGeneric.h	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerGeneric.h	Wed Apr 16 00:35:47 2008 -0400
> @@ -41,8 +41,6 @@
>  
>  void            nm_generic_device_replace_default_route (const char *iface, guint32 gw, guint32 mss);
>  
> -void			nm_generic_device_add_route_via_device_with_iface (const char *iface, const char *route);
> -
>  void			nm_generic_device_flush_ip4_addresses			(NMDevice *dev);
>  void			nm_generic_device_flush_ip4_addresses_with_iface	(const char *iface);
>  
> diff -r 630520f22ebb src/backends/NetworkManagerGentoo.c
> --- a/src/backends/NetworkManagerGentoo.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerGentoo.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -123,18 +123,6 @@
>  	nm_spawn_process (buf);
>  	g_free (buf);
>  #endif
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_enable_loopback
> diff -r 630520f22ebb src/backends/NetworkManagerMandriva.c
> --- a/src/backends/NetworkManagerMandriva.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerMandriva.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -90,19 +90,6 @@
>  {
>  	nm_generic_device_replace_default_route (iface, gw, mss);
>  }
> -
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_device_has_active_routes
> diff -r 630520f22ebb src/backends/NetworkManagerPaldo.c
> --- a/src/backends/NetworkManagerPaldo.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerPaldo.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -92,19 +92,6 @@
>  	nm_generic_device_replace_default_route (iface, gw, mss);
>  }
>  
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
> -
>  /*
>   * nm_system_device_has_active_routes
>   *
> diff -r 630520f22ebb src/backends/NetworkManagerRedHat.c
> --- a/src/backends/NetworkManagerRedHat.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerRedHat.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -88,19 +88,6 @@
>  {
>  	nm_generic_device_replace_default_route (iface, gw, mss);
>  }
> -
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_device_has_active_routes
> diff -r 630520f22ebb src/backends/NetworkManagerSlackware.c
> --- a/src/backends/NetworkManagerSlackware.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerSlackware.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -158,16 +158,6 @@
>  {
>  }
>  
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
>  
>  /*
>   * nm_system_device_replace_default_route
> diff -r 630520f22ebb src/backends/NetworkManagerSuSE.c
> --- a/src/backends/NetworkManagerSuSE.c	Tue Apr 15 20:11:30 2008 -0400
> +++ b/src/backends/NetworkManagerSuSE.c	Wed Apr 16 00:35:47 2008 -0400
> @@ -97,19 +97,6 @@
>  {
>  	nm_generic_device_replace_default_route (iface, gw, mss);
>  }
> -
> -
> -/*
> - * nm_system_device_add_route_via_device_with_iface
> - *
> - * Add route to the given device
> - *
> - */
> -void nm_system_device_add_route_via_device_with_iface (const char *iface, const char *route)
> -{
> -	nm_generic_device_add_route_via_device_with_iface (iface, route);
> -}
> -
>  
>  /*
>   * nm_system_device_has_active_routes