Re: [PATCH 4/8] ethernet: Move all connection <-> device matching into a single function

[Dan Williams <dcbw redhat com>]

On Mon, 2011-11-21 at 17:50 +0100, Thomas Graf wrote:
> Moves all code to match an ethernet connection into a single function
> match_ethernet_connection() and use it from everywhere within
> NMDeviceEthernet.

Looks good pending the outcome of the connection/interface patch.  Note
that danw's IPoIB patches will conflict with this, do you want to wait
for that to merge (I can do that ASAP) or do you want me to rebase his
stuff on top of this one?  (he splits up NMDeviceEthernet into an
abstract NMDeviceWired and a concrete NMDeviceEthernet so that
NMDeviceInfiniband can be a subclass of NMDeviceWired).

Dan

> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  src/nm-device-ethernet.c |  267 +++++++++++++++++-----------------------------
>  1 files changed, 97 insertions(+), 170 deletions(-)
> 
> diff --git a/src/nm-device-ethernet.c b/src/nm-device-ethernet.c
> index d398bf0..ac28654 100644
> --- a/src/nm-device-ethernet.c
> +++ b/src/nm-device-ethernet.c
> @@ -870,79 +870,116 @@ match_subchans (NMDeviceEthernet *self, NMSettingWired *s_wired, gboolean *try_m
>  	return TRUE;
>  }
>  
> +static gboolean
> +match_ethernet_connection (NMDevice *device, NMConnection *connection,
> +                           gboolean check_blacklist, GError **error)
> +{
> +	const char *type, *iface;
> +	NMSettingConnection *s_con;
> +	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device);
> +	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
> +	NMSettingWired *s_wired;
> +
> +	s_wired = nm_connection_get_setting_wired (connection);
> +	s_con = nm_connection_get_setting_connection (connection);
> +	g_assert (s_con);
> +
> +	iface = nm_connection_get_iface_name (connection);
> +	if (iface && strcmp (nm_device_get_iface (device), iface))
> +		return FALSE;
> +
> +	type = nm_setting_connection_get_connection_type (s_con);
> +	g_assert (type);
> +
> +	if (!strcmp (type, NM_SETTING_PPPOE_SETTING_NAME)) {
> +		/* NOP */
> +	} else if (!strcmp (type, NM_SETTING_BOND_SETTING_NAME)) {
> +		/* NOP */
> +	} else if (!strcmp (type, NM_SETTING_WIRED_SETTING_NAME)) {
> +		if (!s_wired) {
> +			if (error)
> +				g_set_error (error,
> +				             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INVALID,
> +				             "The connection was not a valid wired connection.");
> +			return FALSE;
> +		}
> +	} else {
> +		if (error)
> +				g_set_error (error,
> +							 NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_NOT_WIRED,
> +							 "The connection was not a wired, bond, or PPPoE connection.");
> +
> +		return FALSE;
> +	}
> +
> +	if (s_wired) {
> +		const GByteArray *mac;
> +		gboolean try_mac = TRUE;
> +		const GSList *mac_blacklist, *mac_blacklist_iter;
> +
> +		if (!match_subchans (self, s_wired, &try_mac)) {
> +			if (error)
> +				g_set_error (error,
> +							 NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> +							 "The connection's s390 subchannels did not match this device.");
> +			return FALSE;
> +		}
> +
> +		mac = nm_setting_wired_get_mac_address (s_wired);
> +		if (try_mac && mac && memcmp (mac->data, &priv->perm_hw_addr, ETH_ALEN)) {
> +			if (error)
> +				g_set_error (error,
> +							 NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> +							 "The connection's MAC address did not match this device.");
> +			return FALSE;
> +		}
> +
> +		if (!check_blacklist)
> +			return TRUE;
> +
> +		/* Check for MAC address blacklist */
> +		mac_blacklist = nm_setting_wired_get_mac_address_blacklist (s_wired);
> +		for (mac_blacklist_iter = mac_blacklist; mac_blacklist_iter;
> +			 mac_blacklist_iter = g_slist_next (mac_blacklist_iter)) {
> +			struct ether_addr addr;
> +
> +			if (!ether_aton_r (mac_blacklist_iter->data, &addr)) {
> +				g_warn_if_reached ();
> +				return FALSE;
> +			}
> +			if (memcmp (&addr, &priv->perm_hw_addr, ETH_ALEN) == 0) {
> +				if (error)
> +					g_set_error (error,
> +								 NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> +								 "The connection's MAC address (%s) is blacklisted in %s.",
> +								 (char *) mac_blacklist_iter->data, NM_SETTING_WIRED_MAC_ADDRESS_BLACKLIST);
> +				return FALSE;
> +			}
> +		}
> +	}
> +
> +	return TRUE;
> +}
> +
>  static NMConnection *
>  real_get_best_auto_connection (NMDevice *dev,
>                                 GSList *connections,
>                                 char **specific_object)
>  {
> -	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (dev);
> -	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
>  	GSList *iter;
>  
>  	for (iter = connections; iter; iter = g_slist_next (iter)) {
>  		NMConnection *connection = NM_CONNECTION (iter->data);
>  		NMSettingConnection *s_con;
> -		NMSettingWired *s_wired;
> -		const char *connection_type, *iface;
> -		gboolean is_pppoe = FALSE;
> -		const GSList *mac_blacklist, *mac_blacklist_iter;
> -		gboolean mac_blacklist_found = FALSE;
>  
>  		s_con = nm_connection_get_setting_connection (connection);
>  		g_assert (s_con);
>  
> -		connection_type = nm_setting_connection_get_connection_type (s_con);
> -
> -		iface = nm_connection_get_iface_name (candidate);
> -		if (iface && strcmp (nm_device_get_iface (dev), iface))
> -			continue;
> -	
> -		if (!strcmp (connection_type, NM_SETTING_PPPOE_SETTING_NAME))
> -			is_pppoe = TRUE;
> -
> -		if (!is_pppoe && strcmp (connection_type, NM_SETTING_WIRED_SETTING_NAME))
> -			continue;
> -		if (!nm_setting_connection_get_autoconnect (s_con))
> -			continue;
> -
> -		s_wired = nm_connection_get_setting_wired (connection);
> -		/* Wired setting optional for PPPoE */
> -		if (!is_pppoe && !s_wired)
> -			continue;
> -
> -		if (s_wired) {
> -			const GByteArray *mac;
> -			gboolean try_mac = TRUE;
> -
> -			if (!match_subchans (self, s_wired, &try_mac))
> -				continue;
> -
> -			mac = nm_setting_wired_get_mac_address (s_wired);
> -			if (try_mac && mac && memcmp (mac->data, &priv->perm_hw_addr, ETH_ALEN))
> -				continue;
> -
> -			/* Check for MAC address blacklist */
> -			mac_blacklist = nm_setting_wired_get_mac_address_blacklist (s_wired);
> -			for (mac_blacklist_iter = mac_blacklist; mac_blacklist_iter;
> -			     mac_blacklist_iter = g_slist_next (mac_blacklist_iter)) {
> -				struct ether_addr addr;
> -
> -				if (!ether_aton_r (mac_blacklist_iter->data, &addr)) {
> -					g_warn_if_reached ();
> -					continue;
> -				}
> -				if (memcmp (&addr, &priv->perm_hw_addr, ETH_ALEN) == 0) {
> -					mac_blacklist_found = TRUE;
> -					break;
> -				}
> -			}
> -			/* Found device MAC address in the blacklist - do not use this connection */
> -			if (mac_blacklist_found)
> -				continue;
> -		}
> -
> -		return connection;
> +		if (   nm_setting_connection_get_autoconnect (s_con)
> +		    && match_ethernet_connection (dev, connection, TRUE, NULL))
> +			return connection;
>  	}
> +
>  	return NULL;
>  }
>  
> @@ -1595,83 +1632,7 @@ real_check_connection_compatible (NMDevice *device,
>                                    NMConnection *connection,
>                                    GError **error)
>  {
> -	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device);
> -	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
> -	NMSettingConnection *s_con;
> -	NMSettingWired *s_wired;
> -	const char *connection_type;
> -	gboolean is_pppoe = FALSE, is_bond = FALSE;
> -	const GByteArray *mac;
> -	gboolean try_mac = TRUE;
> -	const GSList *mac_blacklist, *mac_blacklist_iter;
> -
> -	s_con = nm_connection_get_setting_connection (connection);
> -	g_assert (s_con);
> -
> -	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, 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 = nm_connection_get_setting_wired (connection);
> -	/* Wired setting is optional for PPPoE */
> -	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.");
> -		return FALSE;
> -	}
> -
> -	if (s_wired) {
> -		if (!match_subchans (self, s_wired, &try_mac)) {
> -			g_set_error (error,
> -			             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> -			             "The connection's s390 subchannels did not match this device.");
> -			return FALSE;
> -		}
> -
> -		mac = nm_setting_wired_get_mac_address (s_wired);
> -		if (try_mac && mac && memcmp (mac->data, &priv->perm_hw_addr, ETH_ALEN)) {
> -			g_set_error (error,
> -			             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> -			             "The connection's MAC address did not match this device.");
> -			return FALSE;
> -		}
> -
> -		/* Check for MAC address blacklist */
> -		mac_blacklist = nm_setting_wired_get_mac_address_blacklist (s_wired);
> -		for (mac_blacklist_iter = mac_blacklist; mac_blacklist_iter;
> -		     mac_blacklist_iter = g_slist_next (mac_blacklist_iter)) {
> -			struct ether_addr addr;
> -
> -			if (!ether_aton_r (mac_blacklist_iter->data, &addr)) {
> -				g_warn_if_reached ();
> -				continue;
> -			}
> -			if (memcmp (&addr, &priv->perm_hw_addr, ETH_ALEN) == 0) {
> -				g_set_error (error,
> -				             NM_ETHERNET_ERROR, NM_ETHERNET_ERROR_CONNECTION_INCOMPATIBLE,
> -				             "The connection's MAC address (%s) is blacklisted in %s.",
> -				             (char *) mac_blacklist_iter->data, NM_SETTING_WIRED_MAC_ADDRESS_BLACKLIST);
> -				return FALSE;
> -			}
> -		}
> -	}
> -
> -	// FIXME: check bitrate against device capabilities
> -
> -	return TRUE;
> +	return match_ethernet_connection (device, connection, TRUE, error);
>  }
>  
>  static gboolean
> @@ -1754,29 +1715,6 @@ spec_match_list (NMDevice *device, const GSList *specs)
>  }
>  
>  static gboolean
> -wired_match_config (NMDevice *self, NMConnection *connection)
> -{
> -	NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self);
> -	NMSettingWired *s_wired;
> -	const GByteArray *s_ether;
> -	gboolean try_mac = TRUE;
> -
> -	s_wired = nm_connection_get_setting_wired (connection);
> -	if (!s_wired)
> -		return FALSE;
> -
> -	if (!match_subchans (NM_DEVICE_ETHERNET (self), s_wired, &try_mac))
> -		return FALSE;
> -
> -	/* MAC address check */
> -	s_ether = nm_setting_wired_get_mac_address (s_wired);
> -	if (try_mac && s_ether && memcmp (s_ether->data, priv->perm_hw_addr, ETH_ALEN))
> -		return FALSE;
> -
> -	return TRUE;
> -}
> -
> -static gboolean
>  ip4_match_config (NMDevice *self, NMConnection *connection)
>  {
>  	NMSettingIP4Config *s_ip4;
> @@ -1854,19 +1792,11 @@ static NMConnection *
>  connection_match_config (NMDevice *self, const GSList *connections)
>  {
>  	GSList *iter;
> -	NMSettingConnection *s_con;
>  
>  	for (iter = (GSList *) connections; iter; iter = g_slist_next (iter)) {
>  		NMConnection *candidate = NM_CONNECTION (iter->data);
> -		const char *iface;
> -
> -		iface = nm_connection_get_iface_name (candidate);
> -		if (iface && strcmp (nm_device_get_iface (self), iface))
> -			continue;
>  
> -		s_con = nm_connection_get_setting_connection (candidate);
> -		g_assert (s_con);
> -		if (strcmp (nm_setting_connection_get_connection_type (s_con), NM_SETTING_WIRED_SETTING_NAME))
> +		if (!match_ethernet_connection (self, candidate, FALSE, NULL))
>  			continue;
>  
>  		/* Can't assume 802.1x or PPPoE connections; they have too much state
> @@ -1876,9 +1806,6 @@ connection_match_config (NMDevice *self, const GSList *connections)
>  		    || nm_connection_get_setting_pppoe (candidate))
>  			continue;
>  
> -		if (!wired_match_config (self, candidate))
> -			continue;
> -
>  		if (!ip4_match_config (self, candidate))
>  			continue;
>