Re: [PATCH 1/7] bonding: Add nm_connection_get_virtual_iface_name() to abstract kernel interface binding

[Dan Williams <dcbw redhat com>]

On Tue, 2011-11-22 at 12:19 +0100, Thomas Graf wrote:
> Some connection types such as bonding, bridging and VLAN require
> specific virtual kernel interfaces identified by name to be auto
> connected to the connection.
> 
> The function nm_connection_get_virtual_iface_name() returns the name
> of the kernel interface if the connection type requires this
> functionatlity.
> 
> Each connection base type settings class can implement the function
> get_virtual_iface_name() if rqeuired.
> 
> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  libnm-util/libnm-util.ver    |    3 ++
>  libnm-util/nm-connection.c   |   51 ++++++++++++++++++++++++++++++++++++++++++
>  libnm-util/nm-connection.h   |    4 +++
>  libnm-util/nm-setting-bond.c |    9 +++++++
>  libnm-util/nm-setting.c      |   20 ++++++++++++++++
>  libnm-util/nm-setting.h      |    5 +++-
>  src/nm-device-ethernet.c     |   25 ++------------------
>  src/settings/nm-settings.c   |    7 +++--
>  8 files changed, 98 insertions(+), 26 deletions(-)
> 
> diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver
> index fbc33bc..5a3e52c 100644
> --- a/libnm-util/libnm-util.ver
> +++ b/libnm-util/libnm-util.ver
> @@ -17,6 +17,7 @@ global:
>  	nm_connection_get_setting_802_1x;
>  	nm_connection_get_setting_bluetooth;
>  	nm_connection_get_setting_bond;
> +	nm_connection_get_setting_base_type;
>  	nm_connection_get_setting_by_name;
>  	nm_connection_get_setting_cdma;
>  	nm_connection_get_setting_connection;
> @@ -32,6 +33,7 @@ global:
>  	nm_connection_get_setting_wired;
>  	nm_connection_get_setting_wireless;
>  	nm_connection_get_setting_wireless_security;
> +	nm_connection_get_virtual_iface_name;
>  	nm_connection_get_type;
>  	nm_connection_get_uuid;
>  	nm_connection_is_type;
> @@ -296,6 +298,7 @@ global:
>  	nm_setting_ip6_config_remove_dns;
>  	nm_setting_ip6_config_remove_dns_search;
>  	nm_setting_ip6_config_remove_route;
> +	nm_setting_get_virtual_iface_name;
>  	nm_setting_need_secrets;
>  	nm_setting_new_from_hash;
>  	nm_setting_olpc_mesh_error_get_type;
> diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c
> index 4003cb7..f89a186 100644
> --- a/libnm-util/nm-connection.c
> +++ b/libnm-util/nm-connection.c
> @@ -490,6 +490,33 @@ nm_connection_get_setting_by_name (NMConnection *connection, const char *name)
>  	return type ? nm_connection_get_setting (connection, type) : NULL;
>  }
>  
> +/**
> + * nm_connection_get_setting_base_type:
> + * @connection: a #NMConnection
> + *
> + * Returns: (transfer none): the #NMSetting of the connection base type
> + */
> +NMSetting *
> +nm_connection_get_setting_base_type (NMConnection *connection)

Could we rename this to nm_connection_get_type_setting()?  And it looks
like it's not used anywhere but internally, so can we keep it private to
libnm-glib until something in NM uses it?

Other than that, looks good.

Dan

> +{
> +	NMSettingConnection *s_con;
> +	const char *type;
> +	NMSetting *base;
> +
> +	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
> +
> +	s_con = nm_connection_get_setting_connection (connection);
> +	g_assert (s_con);
> +
> +	type = nm_setting_connection_get_connection_type (s_con);
> +	g_assert (type);
> +
> +	base = nm_connection_get_setting_by_name (connection, type);
> +	g_assert (base);
> +
> +	return base;
> +}
> +
>  static gboolean
>  validate_permissions_type (GHashTable *hash, GError **error)
>  {
> @@ -1183,6 +1210,30 @@ nm_connection_get_path (NMConnection *connection)
>  }
>  
>  /**
> + * nm_connection_get_virtual_iface_name:
> + * @connection: The #NMConnection
> + *
> + * Returns the name of the virtual kernel interface which the connection
> + * needs to use if specified in the settings. 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_virtual_iface_name (NMConnection *connection)
> +{
> +	NMSetting *base;
> +
> +	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
> +
> +	base = nm_connection_get_setting_base_type (connection);
> +	g_assert (base);
> +
> +	return nm_setting_get_virtual_iface_name (base);
> +}
> +
> +/**
>   * 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..a0e167c 100644
> --- a/libnm-util/nm-connection.h
> +++ b/libnm-util/nm-connection.h
> @@ -129,6 +129,8 @@ NMSetting    *nm_connection_get_setting   (NMConnection *connection,
>  NMSetting    *nm_connection_get_setting_by_name (NMConnection *connection,
>                                                   const char   *name);
>  
> +NMSetting    *nm_connection_get_setting_base_type (NMConnection *connection);
> +
>  gboolean      nm_connection_replace_settings (NMConnection *connection,
>                                                GHashTable *new_settings,
>                                                GError **error);
> @@ -163,6 +165,8 @@ void          nm_connection_set_path      (NMConnection *connection,
>  
>  const char *  nm_connection_get_path      (NMConnection *connection);
>  
> +const char *  nm_connection_get_virtual_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/libnm-util/nm-setting-bond.c b/libnm-util/nm-setting-bond.c
> index 582e1f9..3aa9cf3 100644
> --- a/libnm-util/nm-setting-bond.c
> +++ b/libnm-util/nm-setting-bond.c
> @@ -287,6 +287,14 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
>  	return TRUE;
>  }
>  
> +static const char *
> +get_virtual_iface_name (NMSetting *setting)
> +{
> +	NMSettingBond *self = NM_SETTING_BOND (setting);
> +
> +	return nm_setting_bond_get_interface_name (self);
> +}
> +
>  static void
>  nm_setting_bond_init (NMSettingBond *setting)
>  {
> @@ -389,6 +397,7 @@ nm_setting_bond_class_init (NMSettingBondClass *setting_class)
>  	object_class->get_property = get_property;
>  	object_class->finalize     = finalize;
>  	parent_class->verify       = verify;
> +	parent_class->get_virtual_iface_name = get_virtual_iface_name;
>  
>  	/* Properties */
>  	/**
> diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c
> index 2b3bdc5..9402bc9 100644
> --- a/libnm-util/nm-setting.c
> +++ b/libnm-util/nm-setting.c
> @@ -992,6 +992,26 @@ nm_setting_to_string (NMSetting *setting)
>  	return g_string_free (string, FALSE);
>  }
>  
> +/**
> + * nm_setting_get_virtual_iface_name:
> + * @setting: the #NMSetting
> + *
> + * Returns the name of the virtual kernel interface which the connection
> + * needs to use if specified in the settings.
> + *
> + * Returns: (transfer full) (element-type utf8): Name of the virtual interface
> + **/
> +const char *
> +nm_setting_get_virtual_iface_name (NMSetting *setting)
> +{
> +	g_return_val_if_fail (NM_IS_SETTING (setting), NULL);
> +
> +	if (NM_SETTING_GET_CLASS (setting)->get_virtual_iface_name)
> +		return NM_SETTING_GET_CLASS (setting)->get_virtual_iface_name (setting);
> +
> +	return NULL;
> +}
> +
>  /*****************************************************************************/
>  
>  static void
> diff --git a/libnm-util/nm-setting.h b/libnm-util/nm-setting.h
> index 3edf2a1..843b791 100644
> --- a/libnm-util/nm-setting.h
> +++ b/libnm-util/nm-setting.h
> @@ -203,9 +203,10 @@ typedef struct {
>  	                                         NMSettingClearSecretsWithFlagsFn func,
>  	                                         gpointer user_data);
>  
> +	const char *(*get_virtual_iface_name) (NMSetting *setting);
> +
>  	/* Padding for future expansion */
>  	void (*_reserved1) (void);
> -	void (*_reserved2) (void);
>  } NMSettingClass;
>  
>  /**
> @@ -308,6 +309,8 @@ gboolean    nm_setting_set_secret_flags (NMSetting *setting,
>                                           NMSettingSecretFlags flags,
>                                           GError **error);
>  
> +const char *nm_setting_get_virtual_iface_name (NMSetting *setting);
> +
>  G_END_DECLS
>  
>  #endif /* NM_SETTING_H */
> diff --git a/src/nm-device-ethernet.c b/src/nm-device-ethernet.c
> index fc2c1bd..59ea178 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_virtual_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;
> diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
> index 07d5a29..3ecfb91 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_virtual_iface_name (connection);
> +		if (iface) {
> +			if (!strcmp (iface, nm_device_get_iface (device))) {
>  				ret = TRUE;
>  				break;
>  			} else