Re: [PATCH 5/8] bonding: Implement bonding slaves

[Dan Williams <dcbw redhat com>]

On Tue, 2011-10-18 at 13:48 +0200, Thomas Graf wrote:
> Adds a MASTER= directive to ifcfg-rh allowing a connection to define
> itself as bonding slave.
> 
> Adds a connection property "master" which contains the in-kernel device
> name or UUID of the master connection.
> 
> Adds a connection property "slave-type" which defines the type of slave
> this connection represents. Currently this is only set by bonding but
> eventually this will be used by VLAN and bridging.

Could we drop this property and just use the 'type' setting of the
connection to determine what type of slave this is?  ie, if 'master' is
set and 'type == 'bond'' then this is a bond slave, no?  It seems like
most of the time the 'slave-type' value will correspond to the same
value as 'type'.  Thoughts?

Dan

> Enforces that no bonding slave connection has any IPv4 or IPv6
> configuration set.
> 
> Changes make_ip4_setting() to take a universal flag indicating whether
> to allow disabling ip4 config or not and use it for both, ip6 and
> bonding special case.
> 
> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  libnm-util/libnm-util.ver              |    3 +
>  libnm-util/nm-setting-connection.c     |  140 ++++++++++++++++++++++++++++++++
>  libnm-util/nm-setting-connection.h     |   11 +++-
>  src/settings/plugins/ifcfg-rh/reader.c |   38 +++++++--
>  4 files changed, 185 insertions(+), 7 deletions(-)
> 
> diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver
> index 0ee5917..b4d6912 100644
> --- a/libnm-util/libnm-util.ver
> +++ b/libnm-util/libnm-util.ver
> @@ -197,11 +197,14 @@ global:
>  	nm_setting_connection_get_connection_type;
>  	nm_setting_connection_get_id;
>  	nm_setting_connection_get_num_permissions;
> +	nm_setting_connection_get_master;
>  	nm_setting_connection_get_permission;
> +	nm_setting_connection_get_slave_type;
>  	nm_setting_connection_get_read_only;
>  	nm_setting_connection_get_timestamp;
>  	nm_setting_connection_get_type;
>  	nm_setting_connection_get_uuid;
> +	nm_setting_connection_is_slave_type;
>  	nm_setting_connection_new;
>  	nm_setting_connection_permissions_user_allowed;
>  	nm_setting_connection_remove_permission;
> diff --git a/libnm-util/nm-setting-connection.c b/libnm-util/nm-setting-connection.c
> index e9030b1..ede63fb 100644
> --- a/libnm-util/nm-setting-connection.c
> +++ b/libnm-util/nm-setting-connection.c
> @@ -72,6 +72,7 @@ nm_setting_connection_error_get_type (void)
>  			ENUM_ENTRY (NM_SETTING_CONNECTION_ERROR_INVALID_PROPERTY, "InvalidProperty"),
>  			ENUM_ENTRY (NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY, "MissingProperty"),
>  			ENUM_ENTRY (NM_SETTING_CONNECTION_ERROR_TYPE_SETTING_NOT_FOUND, "TypeSettingNotFound"),
> +			ENUM_ENTRY (NM_SETTING_CONNECTION_ERROR_IP_CONFIG_FOUND, "IpConfigFound"),
>  			{ 0, 0, 0 }
>  		};
>  		etype = g_enum_register_static ("NMSettingConnectionError", values);
> @@ -97,6 +98,8 @@ typedef struct {
>  	char *id;
>  	char *uuid;
>  	char *type;
> +	char *master;
> +	char *slave_type;
>  	GSList *permissions; /* list of Permission structs */
>  	gboolean autoconnect;
>  	guint64 timestamp;
> @@ -112,6 +115,8 @@ enum {
>  	PROP_AUTOCONNECT,
>  	PROP_TIMESTAMP,
>  	PROP_READ_ONLY,
> +	PROP_MASTER,
> +	PROP_SLAVE_TYPE,
>  
>  	LAST_PROP
>  };
> @@ -478,6 +483,59 @@ nm_setting_connection_get_read_only (NMSettingConnection *setting)
>  	return NM_SETTING_CONNECTION_GET_PRIVATE (setting)->read_only;
>  }
>  
> +/**
> + * nm_setting_connection_get_master:
> + * @setting: the #NMSettingConnection
> + *
> + * Returns the #NMSettingConnection:master property of the connection.
> + *
> + * Returns: Name of the master device
> + */
> +const char *
> +nm_setting_connection_get_master (NMSettingConnection *setting)
> +{
> +	g_return_val_if_fail (NM_IS_SETTING_CONNECTION (setting), NULL);
> +
> +	return NM_SETTING_CONNECTION_GET_PRIVATE (setting)->master;
> +}
> +
> +/**
> + * nm_setting_connection_get_slave_type:
> + * @setting: the #NMSettingConnection
> + *
> + * Returns the #NMSettingConnection:slave-type property of the connection.
> + *
> + * Returns: Type of the slave device
> + */
> +const char *
> +nm_setting_connection_get_slave_type (NMSettingConnection *setting)
> +{
> +	g_return_val_if_fail (NM_IS_SETTING_CONNECTION (setting), NULL);
> +
> +	return NM_SETTING_CONNECTION_GET_PRIVATE (setting)->slave_type;
> +}
> +
> +/**
> + * nm_setting_connection_is_slave_type:
> + * @setting: the #NMSettingConnection
> + * @type: the name of the slave type
> + *
> + * Returns: TRUE if connection is of specified slave type
> + */
> +gboolean
> +nm_setting_connection_is_slave_type (NMSettingConnection *setting,
> +				     const char *type)
> +{
> +	NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting);
> +
> +	g_return_val_if_fail (NM_IS_SETTING_CONNECTION (setting), FALSE);
> +
> +	if (!priv->slave_type)
> +		return FALSE;
> +
> +	return !strcmp (priv->slave_type, type);
> +}
> +
>  static gint
>  find_setting_by_name (gconstpointer a, gconstpointer b)
>  {
> @@ -559,6 +617,46 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
>  		return FALSE;
>  	}
>  
> +	/*
> +	 * Bonding: Slaves are not allowed to have any IP configuration.
> +	 */
> +	if (priv->slave_type && all_settings &&
> +	    !strcmp(priv->slave_type, NM_SETTING_BOND_SETTING_NAME)) {
> +		GSList *list;
> +		
> +		list = g_slist_find_custom (all_settings, NM_SETTING_IP4_CONFIG_SETTING_NAME,
> +					    find_setting_by_name);
> +		if (list) {
> +			NMSettingIP4Config *s_ip4 = g_slist_nth_data (list, 0);
> +			g_assert (s_ip4);
> +
> +			if (strcmp (nm_setting_ip4_config_get_method (s_ip4),
> +				    NM_SETTING_IP4_CONFIG_METHOD_DISABLED)) {
> +				g_set_error (error,
> +					     NM_SETTING_CONNECTION_ERROR,
> +					     NM_SETTING_CONNECTION_ERROR_IP_CONFIG_FOUND,
> +					     "No IP configuration allowed for bonding slave");
> +				return FALSE;
> +			}
> +		}
> +
> +		list = g_slist_find_custom (all_settings, NM_SETTING_IP6_CONFIG_SETTING_NAME,
> +					    find_setting_by_name);
> +		if (list) {
> +			NMSettingIP6Config *s_ip6 = g_slist_nth_data (list, 0);
> +			g_assert (s_ip6);
> +
> +			if (strcmp (nm_setting_ip6_config_get_method (s_ip6),
> +				    NM_SETTING_IP6_CONFIG_METHOD_IGNORE)) {
> +				g_set_error (error,
> +					     NM_SETTING_CONNECTION_ERROR,
> +					     NM_SETTING_CONNECTION_ERROR_IP_CONFIG_FOUND,
> +					     "No IPv6 configuration allowed for bonding slave");
> +				return FALSE;
> +			}
> +		}
> +	}
> +
>  	return TRUE;
>  }
>  
> @@ -591,6 +689,8 @@ finalize (GObject *object)
>  	g_free (priv->id);
>  	g_free (priv->uuid);
>  	g_free (priv->type);
> +	g_free (priv->master);
> +	g_free (priv->slave_type);
>  	nm_utils_slist_free (priv->permissions, (GDestroyNotify) permission_free);
>  
>  	G_OBJECT_CLASS (nm_setting_connection_parent_class)->finalize (object);
> @@ -644,6 +744,14 @@ set_property (GObject *object, guint prop_id,
>  	case PROP_READ_ONLY:
>  		priv->read_only = g_value_get_boolean (value);
>  		break;
> +	case PROP_MASTER:
> +		g_free (priv->master);
> +		priv->master = g_value_dup_string (value);
> +		break;
> +	case PROP_SLAVE_TYPE:
> +		g_free (priv->slave_type);
> +		priv->slave_type = g_value_dup_string (value);
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>  		break;
> @@ -689,6 +797,12 @@ get_property (GObject *object, guint prop_id,
>  	case PROP_READ_ONLY:
>  		g_value_set_boolean (value, nm_setting_connection_get_read_only (setting));
>  		break;
> +	case PROP_MASTER:
> +		g_value_set_string (value, nm_setting_connection_get_master (setting));
> +		break;
> +	case PROP_SLAVE_TYPE:
> +		g_value_set_string (value, nm_setting_connection_get_slave_type (setting));
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>  		break;
> @@ -876,4 +990,30 @@ nm_setting_connection_class_init (NMSettingConnectionClass *setting_class)
>  	                      "cannot yet write updated connections back out.",
>  	                      FALSE,
>  	                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE | NM_SETTING_PARAM_FUZZY_IGNORE));
> +
> +	/**
> +	 * NMSettingConnection:master:
> +	 *
> +	 * Name of the master deviec or UUID of the master connection
> +	 **/
> +	g_object_class_install_property
> +		(object_class, PROP_MASTER,
> +		 g_param_spec_string (NM_SETTING_CONNECTION_MASTER,
> +						  "Master",
> +						  "Name of the master device or UUID of the master connection",
> +						  NULL,
> +						  G_PARAM_READWRITE | NM_SETTING_PARAM_SERIALIZE | NM_SETTING_PARAM_FUZZY_IGNORE));
> +
> +	/**
> +	 * NMSettingConnection:slave-type:
> +	 *
> +	 * Type of slave device or NULL if of not a slave.
> +	 **/
> +	g_object_class_install_property
> +		(object_class, PROP_SLAVE_TYPE,
> +		 g_param_spec_string (NM_SETTING_CONNECTION_SLAVE_TYPE,
> +						  "Slave-Type",
> +						  "Type of slave device or NULL if not a slave",
> +						  NULL,
> +						  G_PARAM_READWRITE | NM_SETTING_PARAM_SERIALIZE | NM_SETTING_PARAM_FUZZY_IGNORE));
>  }
> diff --git a/libnm-util/nm-setting-connection.h b/libnm-util/nm-setting-connection.h
> index 1ec5bf1..d82049c 100644
> --- a/libnm-util/nm-setting-connection.h
> +++ b/libnm-util/nm-setting-connection.h
> @@ -49,6 +49,8 @@ G_BEGIN_DECLS
>   * @NM_SETTING_CONNECTION_ERROR_TYPE_SETTING_NOT_FOUND: the #NMSetting object
>   *   referenced by the setting name contained in the
>   *   #NMSettingConnection:type property was not present in the #NMConnection
> + * @NM_SETTING_CONNECTION_ERROR_IP_CONFIG_FOUND: ip configuration is not
> + *   allowed to be present.
>   *
>   * Describes errors that may result from operations involving a
>   * #NMSettingConnection.
> @@ -59,7 +61,8 @@ typedef enum
>  	NM_SETTING_CONNECTION_ERROR_UNKNOWN = 0,
>  	NM_SETTING_CONNECTION_ERROR_INVALID_PROPERTY,
>  	NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY,
> -	NM_SETTING_CONNECTION_ERROR_TYPE_SETTING_NOT_FOUND
> +	NM_SETTING_CONNECTION_ERROR_TYPE_SETTING_NOT_FOUND,
> +	NM_SETTING_CONNECTION_ERROR_IP_CONFIG_FOUND,
>  } NMSettingConnectionError;
>  
>  #define NM_TYPE_SETTING_CONNECTION_ERROR (nm_setting_connection_error_get_type ()) 
> @@ -75,6 +78,8 @@ GQuark nm_setting_connection_error_quark (void);
>  #define NM_SETTING_CONNECTION_TIMESTAMP   "timestamp"
>  #define NM_SETTING_CONNECTION_READ_ONLY   "read-only"
>  #define NM_SETTING_CONNECTION_PERMISSIONS "permissions"
> +#define NM_SETTING_CONNECTION_MASTER      "master"
> +#define NM_SETTING_CONNECTION_SLAVE_TYPE  "slave-type"
>  
>  /**
>   * NMSettingConnection:
> @@ -119,6 +124,10 @@ gboolean    nm_setting_connection_add_permission       (NMSettingConnection *set
>                                                          const char *detail);
>  void        nm_setting_connection_remove_permission    (NMSettingConnection *setting,
>                                                          guint32 idx);
> +const char *nm_setting_connection_get_master           (NMSettingConnection *setting);
> +gboolean    nm_setting_connection_is_slave_type        (NMSettingConnection *setting,
> +							const char *type);
> +const char *nm_setting_connection_get_slave_type       (NMSettingConnection *setting);
>  
>  G_END_DECLS
>  
> diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
> index a327e79..cee4100 100644
> --- a/src/settings/plugins/ifcfg-rh/reader.c
> +++ b/src/settings/plugins/ifcfg-rh/reader.c
> @@ -139,6 +139,14 @@ make_connection_setting (const char *file,
>  	              svTrueValue (ifcfg, "ONBOOT", TRUE),
>  	              NULL);
>  
> +	value = svGetValue (ifcfg, "MASTER", FALSE);
> +	if (value) {
> +		g_object_set (s_con, NM_SETTING_CONNECTION_MASTER, value, NULL);
> +		g_object_set (s_con, NM_SETTING_CONNECTION_SLAVE_TYPE,
> +			      NM_SETTING_BOND_SETTING_NAME, NULL);
> +		g_free (value);
> +	}
> +
>  	value = svGetValue (ifcfg, "USERS", FALSE);
>  	if (value) {
>  		char **items, **iter;
> @@ -1116,7 +1124,7 @@ static NMSetting *
>  make_ip4_setting (shvarFile *ifcfg,
>                    const char *network_file,
>                    const char *iscsiadm_path,
> -                  gboolean valid_ip6_config,
> +                  gboolean can_disable_ip4,
>                    GError **error)
>  {
>  	NMSettingIP4Config *s_ip4 = NULL;
> @@ -1231,7 +1239,7 @@ make_ip4_setting (shvarFile *ifcfg,
>  		    && !tmp_ip4_0 && !tmp_prefix_0 && !tmp_netmask_0
>  		    && !tmp_ip4_1 && !tmp_prefix_1 && !tmp_netmask_1
>  		    && !tmp_ip4_2 && !tmp_prefix_2 && !tmp_netmask_2) {
> -			if (valid_ip6_config)
> +			if (can_disable_ip4)
>  				/* Nope, no IPv4 */
>  				method = NM_SETTING_IP4_CONFIG_METHOD_DISABLED;
>  			else
> @@ -3471,6 +3479,21 @@ bond_connection_from_ifcfg (const char *file,
>  }
>  
>  static gboolean
> +disabling_ip4_config_allowed (NMConnection *connection)
> +{
> +	NMSettingConnection *s_con;
> +
> +	s_con = nm_connection_get_setting_connection (connection);
> +	g_assert (s_con);
> +
> +	/* bonding slaves are allowed to have no ip configuration */
> +	if (nm_setting_connection_is_slave_type (s_con, NM_SETTING_BOND_SETTING_NAME))
> +		return TRUE;
> +
> +	return FALSE;
> +}
> +
> +static gboolean
>  is_bond_device (const char *name, shvarFile *parsed)
>  {
>  	g_return_val_if_fail (name != NULL, FALSE);
> @@ -3508,7 +3531,7 @@ connection_from_file (const char *filename,
>  	NMSetting *s_ip4, *s_ip6;
>  	const char *ifcfg_name = NULL;
>  	gboolean nm_controlled = TRUE;
> -	gboolean ip6_used = FALSE;
> +	gboolean can_disable_ip4 = FALSE;
>  	GError *error = NULL;
>  	guint32 ignore_reason = IGNORE_REASON_NONE;
>  
> @@ -3677,10 +3700,13 @@ connection_from_file (const char *filename,
>  		nm_connection_add_setting (connection, s_ip6);
>  		method = nm_setting_ip6_config_get_method (NM_SETTING_IP6_CONFIG (s_ip6));
>  		if (method && strcmp (method, NM_SETTING_IP6_CONFIG_METHOD_IGNORE))
> -			ip6_used = TRUE;
> +			can_disable_ip4 = TRUE;
>  	}
>  
> -	s_ip4 = make_ip4_setting (parsed, network_file, iscsiadm_path, ip6_used, &error);
> +	if (disabling_ip4_config_allowed (connection))
> +		can_disable_ip4 = TRUE;
> +
> +	s_ip4 = make_ip4_setting (parsed, network_file, iscsiadm_path, can_disable_ip4, &error);
>  	if (error) {
>  		g_object_unref (connection);
>  		connection = NULL;
> @@ -3697,7 +3723,7 @@ connection_from_file (const char *filename,
>  	    && !g_ascii_strcasecmp (bootproto, "ibft")) {
>  		NMSettingConnection *s_con;
>  
> -		s_con = (NMSettingConnection *) nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION);
> +		s_con = nm_connection_get_setting_connection (connection);
>  		g_assert (s_con);
>  
>  		g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL);