Re: [PATCH 2/8] bonding: Add nm_connection_get_iface_name() to abstract kernel interface binding

[Dan Williams <dcbw redhat com>]

On Mon, 2011-11-21 at 17:50 +0100, Thomas Graf wrote:
> Some connection types such as bonding, bridging and VLAN require
> specific kernel interfaces identified by name to be auto connected
> to the connection.
> 
> The function nm_connection_get_iface_name() returns the name of the
> kernel interface if the connection type requires this functionatlity.
> 
> This allows defining all connection types requiring this to specify
> their needs in a single central place.

Would you mind renaming this to nm_connection_get_virtual_iface_name()
instead?  To make it clear that this interface name is only used for
virtual interfaces and is not relevant for base types that have backing
hardware.

Also the ideal implementation here would actually grab the "base
setting" for the connection and ask it for it's virtual iface name, ie:

const char *
nm_connection_get_virtual_iface_name (NMConnection *connection)
{
	NMSettingConnection *s_con;
	NMSetting *base;

	g_return_val_if_fail (connection != NULL, NULL);

	s_con = nm_connection_get_setting_connection (connection);
	base = nm_connection_get_setting_by_name (connection, nm_setting_connection_get_connection_type (s_con));
	g_return_val_if_fail (base != NULL, NULL);

	return nm_setting_get_virtual_iface_name (base);
}

where nm_setting_get_virtual_iface_name() is a virtual function exactly
like nm_setting_need_secrets() and settings that want to override it can
do so, while others that don't have any virtual iface name just don't
implement it, and thus nm_setting_get_virtual_iface_name() would return
NULL.  You'd use _reserved1 in the NMSettingClass structure for the
function pointer.

The one downside is that we only have one more reserved member left in
NMSettingClass after this.

More below...

> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  libnm-util/libnm-util.ver  |    1 +
>  libnm-util/nm-connection.c |   28 ++++++++++++++++++++++++++++
>  libnm-util/nm-connection.h |    2 ++
>  src/nm-device-ethernet.c   |   25 +++----------------------
>  src/settings/nm-settings.c |    7 ++++---
>  5 files changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver
> index 637ae82..4cef1b4 100644
> --- a/libnm-util/libnm-util.ver
> +++ b/libnm-util/libnm-util.ver
> @@ -12,6 +12,7 @@ global:
>  	nm_connection_error_quark;
>  	nm_connection_for_each_setting_value;
>  	nm_connection_get_id;
> +	nm_connection_get_iface_name;
>  	nm_connection_get_path;
>  	nm_connection_get_setting;
>  	nm_connection_get_setting_802_1x;
> diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c
> index 4003cb7..2a28888 100644
> --- a/libnm-util/nm-connection.c
> +++ b/libnm-util/nm-connection.c
> @@ -1183,6 +1183,34 @@ nm_connection_get_path (NMConnection *connection)
>  }
>  
>  /**
> + * nm_connection_get_iface_name:
> + * @connection: The #NMConnection
> + *
> + * Returns the name of the kernel interface which needs to be connected
> + * to this connection. This function abstracts all connection types which
> + * require this functionality. For all other connection types, this
> + * function will return NULL.
> + *
> + * Returns: Name of the kernel interface or NULL
> + */
> +const char *
> +nm_connection_get_iface_name (NMConnection *connection)
> +{
> +	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
> +
> +	if (nm_connection_is_type (connection, NM_SETTING_BOND_SETTING_NAME)) {
> +		NMSettingBond *s_bond;
> +
> +		s_bond = nm_connection_get_setting_bond (connection);
> +		g_assert (s_bond);
> +
> +		return nm_setting_bond_get_interface_name (s_bond);
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
>   * nm_connection_new:
>   *
>   * Creates a new #NMConnection object with no #NMSetting objects.
> diff --git a/libnm-util/nm-connection.h b/libnm-util/nm-connection.h
> index ad1017f..60ba54e 100644
> --- a/libnm-util/nm-connection.h
> +++ b/libnm-util/nm-connection.h
> @@ -163,6 +163,8 @@ void          nm_connection_set_path      (NMConnection *connection,
>  
>  const char *  nm_connection_get_path      (NMConnection *connection);
>  
> +const char *  nm_connection_get_iface_name (NMConnection *connection);
> +
>  gboolean      nm_connection_is_type (NMConnection *connection, const char *type);
>  
>  void          nm_connection_for_each_setting_value (NMConnection *connection,
> diff --git a/src/nm-device-ethernet.c b/src/nm-device-ethernet.c
> index fc2c1bd..9390ecf 100644
> --- a/src/nm-device-ethernet.c
> +++ b/src/nm-device-ethernet.c
> @@ -601,22 +601,6 @@ nm_device_ethernet_get_address (NMDeviceEthernet *self, struct ether_addr *addr)
>  	memcpy (addr, &priv->hw_addr, sizeof (priv->hw_addr));
>  }
>  
> -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_connection_get_setting_bond (connection);
> -	if (s_bond && !strcmp (devname, nm_setting_bond_get_interface_name (s_bond)))
> -		return TRUE;
> -
> -	return FALSE;
> -}
> -
>  /* Returns speed in Mb/s */
>  static guint32
>  nm_device_ethernet_get_speed (NMDeviceEthernet *self)
> @@ -899,7 +883,7 @@ real_get_best_auto_connection (NMDevice *dev,
>  		NMConnection *connection = NM_CONNECTION (iter->data);
>  		NMSettingConnection *s_con;
>  		NMSettingWired *s_wired;
> -		const char *connection_type;
> +		const char *connection_type, *iface;
>  		gboolean is_pppoe = FALSE;
>  		const GSList *mac_blacklist, *mac_blacklist_iter;
>  		gboolean mac_blacklist_found = FALSE;
> @@ -909,12 +893,9 @@ real_get_best_auto_connection (NMDevice *dev,
>  
>  		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;
> -
> +		iface = nm_connection_get_iface_name (candidate);
> +		if (iface && strcmp (nm_device_get_iface (dev), iface))

g_strcmp0() is NULL safe.

>  			continue;
> -		}
>  	
>  		if (!strcmp (connection_type, NM_SETTING_PPPOE_SETTING_NAME))
>  			is_pppoe = TRUE;
> diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
> index 07d5a29..bf72459 100644
> --- a/src/settings/nm-settings.c
> +++ b/src/settings/nm-settings.c
> @@ -1237,13 +1237,14 @@ have_connection_for_device (NMSettings *self, GByteArray *mac, NMDevice *device)
>  	g_hash_table_iter_init (&iter, priv->connections);
>  	while (g_hash_table_iter_next (&iter, NULL, &data)) {
>  		NMConnection *connection = NM_CONNECTION (data);
> -		const char *ctype;
> +		const char *ctype, *iface;
>  
>  		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)) {
> +		iface = nm_connection_get_iface_name (connection);
> +		if (iface) {
> +			if (!strcmp (iface, nm_device_get_iface (device))) {

Same here, g_strcmp0() saves an indent block.

>  				ret = TRUE;
>  				break;
>  			} else

Dan