Re: [PATCH 4/9] ethernet: Correctly match bonding ethernet devices with bonding connection

[Dan Williams <dcbw redhat com>]

On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:
> Based on in-kernel device name
> 
> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  src/nm-device-ethernet.c   |   19 ++++++++++++++++---
>  src/nm-device.c            |   16 ++++++++++++++++
>  src/nm-device.h            |    2 ++
>  src/settings/nm-settings.c |   13 +++++++++++--
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/src/nm-device-ethernet.c b/src/nm-device-ethernet.c
> index 8556c5b..4f3b89c 100644
> --- a/src/nm-device-ethernet.c
> +++ b/src/nm-device-ethernet.c
> @@ -55,6 +55,7 @@
>  #include "nm-setting-wired.h"
>  #include "nm-setting-8021x.h"
>  #include "nm-setting-pppoe.h"
> +#include "nm-setting-bond.h"
>  #include "ppp-manager/nm-ppp-manager.h"
>  #include "nm-logging.h"
>  #include "nm-properties-changed-signal.h"
> @@ -890,6 +891,14 @@ real_get_best_auto_connection (NMDevice *dev,
>  		g_assert (s_con);
>  
>  		connection_type = nm_setting_connection_get_connection_type (s_con);
> +
> +		if (!strcmp (connection_type, NM_SETTING_BOND_SETTING_NAME)) {
> +			if (nm_device_bond_connection_matches (dev, connection))
> +				return connection;
> +
> +			continue;
> +		}
> +	
>  		if (!strcmp (connection_type, NM_SETTING_PPPOE_SETTING_NAME))
>  			is_pppoe = TRUE;
>  
> @@ -1625,7 +1634,7 @@ real_check_connection_compatible (NMDevice *device,
>  	NMSettingConnection *s_con;
>  	NMSettingWired *s_wired;
>  	const char *connection_type;
> -	gboolean is_pppoe = FALSE;
> +	gboolean is_pppoe = FALSE, is_bond = FALSE;
>  	const GByteArray *mac;
>  	gboolean try_mac = TRUE;
>  	const GSList *mac_blacklist, *mac_blacklist_iter;
> @@ -1635,19 +1644,23 @@ real_check_connection_compatible (NMDevice *device,
>  
>  	connection_type = nm_setting_connection_get_connection_type (s_con);
>  	if (   strcmp (connection_type, NM_SETTING_WIRED_SETTING_NAME)
> +	    && strcmp (connection_type, NM_SETTING_BOND_SETTING_NAME)
>  	    && strcmp (connection_type, NM_SETTING_PPPOE_SETTING_NAME)) {
>  		g_set_error (error,
>  		             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_NOT_WIRED,
> -		             "The connection was not a wired or PPPoE connection.");
> +		             "The connection was not a wired, bond, or PPPoE connection.");
>  		return FALSE;
>  	}
>  
>  	if (!strcmp (connection_type, NM_SETTING_PPPOE_SETTING_NAME))
>  		is_pppoe = TRUE;
>  
> +	if (!strcmp (connection_type, NM_SETTING_BOND_SETTING_NAME))
> +		is_bond = TRUE;
> +
>  	s_wired = (NMSettingWired *) nm_connection_get_setting (connection, NM_TYPE_SETTING_WIRED);
>  	/* Wired setting is optional for PPPoE */
> -	if (!is_pppoe && !s_wired) {
> +	if (!is_pppoe && !s_wired && !is_bond) {
>  		g_set_error (error,
>  		             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INVALID,
>  		             "The connection was not a valid wired connection.");
> diff --git a/src/nm-device.c b/src/nm-device.c
> index 559606c..6142243 100644
> --- a/src/nm-device.c
> +++ b/src/nm-device.c
> @@ -4061,3 +4061,19 @@ nm_device_clear_autoconnect_inhibit (NMDevice *device)
>  	priv->autoconnect_inhibit = FALSE;
>  }
>  
> +gboolean
> +nm_device_bond_connection_matches(NMDevice *device, NMConnection *connection)
> +{
> +	NMSettingBond *s_bond;
> +	const char *devname;
> +
> +	devname = nm_device_get_iface (device);
> +	g_assert(devname);
> +
> +	s_bond = NM_SETTING_BOND (nm_connection_get_setting (connection,
> +				  NM_TYPE_SETTING_BOND));
> +	if (s_bond && !strcmp (devname, nm_setting_bond_get_device_name (s_bond)))
> +		return TRUE;
> +
> +	return FALSE;
> +}

I'd actually rather stuff this into nm-setting.c instead of having it
generic in nm-device.c, and call it something like
bond_connection_matches() or something.  At least until we have another
user.

Dan

> diff --git a/src/nm-device.h b/src/nm-device.h
> index 1d080ed..62c8fc0 100644
> --- a/src/nm-device.h
> +++ b/src/nm-device.h
> @@ -192,6 +192,8 @@ void nm_device_clear_autoconnect_inhibit (NMDevice *device);
>  
>  gboolean nm_device_dhcp4_renew (NMDevice *device, gboolean release);
>  
> +gboolean nm_device_bond_connection_matches(NMDevice *device, NMConnection *connection);
> +
>  G_END_DECLS
>  
>  #endif	/* NM_DEVICE_H */
> diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
> index cb094e9..6822722 100644
> --- a/src/settings/nm-settings.c
> +++ b/src/settings/nm-settings.c
> @@ -51,6 +51,7 @@
>  #include <nm-setting-wired.h>
>  #include <nm-setting-wireless.h>
>  #include <nm-setting-wireless-security.h>
> +#include <nm-setting-bond.h>
>  
>  #include "../nm-device-ethernet.h"
>  #include "nm-dbus-glib-types.h"
> @@ -1174,7 +1175,7 @@ impl_settings_save_hostname (NMSettings *self,
>  }
>  
>  static gboolean
> -have_connection_for_device (NMSettings *self, GByteArray *mac)
> +have_connection_for_device (NMSettings *self, GByteArray *mac, NMDevice *device)
>  {
>  	NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self);
>  	GHashTableIter iter;
> @@ -1196,6 +1197,14 @@ have_connection_for_device (NMSettings *self, GByteArray *mac)
>  		s_con = nm_connection_get_setting_connection (connection);
>  		ctype = nm_setting_connection_get_connection_type (s_con);
>  
> +		if (!strcmp (ctype, NM_SETTING_BOND_SETTING_NAME)) {
> +			if (nm_device_bond_connection_matches (device, connection)) {
> +				ret = TRUE;
> +				break;
> +			} else
> +				continue;
> +		}
> +
>  		if (   strcmp (ctype, NM_SETTING_WIRED_SETTING_NAME)
>  		    && strcmp (ctype, NM_SETTING_PPPOE_SETTING_NAME))
>  			continue;
> @@ -1447,7 +1456,7 @@ nm_settings_device_added (NMSettings *self, NMDevice *device)
>  	mac = g_byte_array_sized_new (ETH_ALEN);
>  	g_byte_array_append (mac, tmp.ether_addr_octet, ETH_ALEN);
>  
> -	if (   have_connection_for_device (self, mac)
> +	if (   have_connection_for_device (self, mac, device)
>  	    || is_mac_auto_wired_blacklisted (self, mac))
>  		goto ignore;
>