Re: [PATCH 8/9] ifcfg: Ignore higher level configs for bonding slaves

[Dan Williams <dcbw redhat com>]

On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:
> Functionatlity is implemented on NMSettingConnection level in case
> other device types require this as well.
> 
> Adds a dummy ip4_config when high level config is disabled. This is
> required in order not to fall back to using DHCP.

I'm not 100% sold on this; is there a reason we couldn't just set the IP
configuration to disabled here?  I feel like this is something we can
detect programmatically so it's perhaps not something we should put into
the setting API itself.

What we could do here instead is override the IP config stuff in the
Ethernet device class and check if the connection being activated is a
bond slave.  We already have to do that (later) for activating the
master connection when the slave is activated.

Dan

> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  libnm-util/libnm-util.ver              |    1 +
>  libnm-util/nm-setting-connection.c     |   41 +++++++++++++++++++++++++++
>  libnm-util/nm-setting-connection.h     |   19 +++++++-----
>  src/settings/plugins/ifcfg-rh/reader.c |   47 ++++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+), 8 deletions(-)
> 
> diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver
> index 0c7aab0..03f445b 100644
> --- a/libnm-util/libnm-util.ver
> +++ b/libnm-util/libnm-util.ver
> @@ -194,6 +194,7 @@ global:
>  	nm_setting_connection_get_autoconnect;
>  	nm_setting_connection_get_connection_type;
>  	nm_setting_connection_get_id;
> +	nm_setting_connection_get_ignore_config;
>  	nm_setting_connection_get_num_permissions;
>  	nm_setting_connection_get_master;
>  	nm_setting_connection_get_permission;
> diff --git a/libnm-util/nm-setting-connection.c b/libnm-util/nm-setting-connection.c
> index 5ca6b60..c3f581b 100644
> --- a/libnm-util/nm-setting-connection.c
> +++ b/libnm-util/nm-setting-connection.c
> @@ -102,6 +102,7 @@ typedef struct {
>  	gboolean autoconnect;
>  	guint64 timestamp;
>  	gboolean read_only;
> +	gboolean ignore_config;
>  } NMSettingConnectionPrivate;
>  
>  enum {
> @@ -114,6 +115,7 @@ enum {
>  	PROP_TIMESTAMP,
>  	PROP_READ_ONLY,
>  	PROP_MASTER,
> +	PROP_IGNORE_CONFIG,
>  
>  	LAST_PROP
>  };
> @@ -496,6 +498,22 @@ nm_setting_connection_get_master (NMSettingConnection *setting)
>  	return NM_SETTING_CONNECTION_GET_PRIVATE (setting)->master;
>  }
>  
> +/**
> + * nm_setting_connection_get_ignore_config:
> + * @setting: the #NMSettingConnection
> + *
> + * Returns the #NMSettingConnection:ignore-config property of the connection.
> + *
> + * Returns: %TRUE if the connection ignores high level configs, %FALSE if it does not
> + **/
> +gboolean
> +nm_setting_connection_get_ignore_config (NMSettingConnection *setting)
> +{
> +	g_return_val_if_fail (NM_IS_SETTING_CONNECTION (setting), TRUE);
> +
> +	return NM_SETTING_CONNECTION_GET_PRIVATE (setting)->ignore_config;
> +}
> +
>  static gint
>  find_setting_by_name (gconstpointer a, gconstpointer b)
>  {
> @@ -667,6 +685,9 @@ set_property (GObject *object, guint prop_id,
>  		g_free (priv->master);
>  		priv->master = g_value_dup_string (value);
>  		break;
> +	case PROP_IGNORE_CONFIG:
> +		priv->ignore_config = g_value_get_boolean (value);
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>  		break;
> @@ -715,6 +736,9 @@ get_property (GObject *object, guint prop_id,
>  	case PROP_MASTER:
>  		g_value_set_string (value, nm_setting_connection_get_master (setting));
>  		break;
> +	case PROP_IGNORE_CONFIG:
> +		g_value_set_boolean (value, nm_setting_connection_get_ignore_config (setting));
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>  		break;
> @@ -915,4 +939,21 @@ nm_setting_connection_class_init (NMSettingConnectionClass *setting_class)
>  						  "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:ignore-config:
> +	 *
> +	 * %TRUE if the connection ignores high level configuration such as
> +	 * ip configuration.
> +	 **/
> +	g_object_class_install_property
> +	    (object_class, PROP_IGNORE_CONFIG,
> +	     g_param_spec_boolean (NM_SETTING_CONNECTION_IGNORE_CONFIG,
> +	                      "Ignore-Config",
> +	                      "If TRUE, the connection will ignore any high level "
> +			      "configuration such as \"ip configuration\". This is "
> +			      "typically the case for bonding or bridging slave "
> +			      "connections.",
> +	                      FALSE,
> +	                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | 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 0768bd6..a7ff659 100644
> --- a/libnm-util/nm-setting-connection.h
> +++ b/libnm-util/nm-setting-connection.h
> @@ -68,14 +68,15 @@ GType nm_setting_connection_error_get_type (void);
>  #define NM_SETTING_CONNECTION_ERROR nm_setting_connection_error_quark ()
>  GQuark nm_setting_connection_error_quark (void);
>  
> -#define NM_SETTING_CONNECTION_ID          "id"
> -#define NM_SETTING_CONNECTION_UUID        "uuid"
> -#define NM_SETTING_CONNECTION_TYPE        "type"
> -#define NM_SETTING_CONNECTION_AUTOCONNECT "autoconnect"
> -#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_ID            "id"
> +#define NM_SETTING_CONNECTION_UUID          "uuid"
> +#define NM_SETTING_CONNECTION_TYPE          "type"
> +#define NM_SETTING_CONNECTION_AUTOCONNECT   "autoconnect"
> +#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_IGNORE_CONFIG "ignore-config"
>  
>  /**
>   * NMSettingConnection:
> @@ -122,6 +123,8 @@ void        nm_setting_connection_remove_permission    (NMSettingConnection *set
>                                                          guint32 idx);
>  const char *nm_setting_connection_get_master           (NMSettingConnection *setting);
>  
> +gboolean    nm_setting_connection_get_ignore_config    (NMSettingConnection *setting);
> +
>  G_END_DECLS
>  
>  #endif /* NM_SETTING_CONNECTION_H */
> diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
> index 1b12069..961fa6c 100644
> --- a/src/settings/plugins/ifcfg-rh/reader.c
> +++ b/src/settings/plugins/ifcfg-rh/reader.c
> @@ -143,6 +143,11 @@ make_connection_setting (const char *file,
>  	if (value) {
>  		g_object_set (s_con, NM_SETTING_CONNECTION_MASTER, value, NULL);
>  		g_free (value);
> +
> +		/* Slave should ignore any high level configurations */
> +		g_object_set (s_con,
> +			      NM_SETTING_CONNECTION_IGNORE_CONFIG, TRUE,
> +			      NULL);
>  	}
>  
>  	value = svGetValue (ifcfg, "USERS", FALSE);
> @@ -1117,6 +1122,24 @@ error:
>  	return success;
>  }
>  
> +static NMSetting *
> +make_disabled_ip4_setting (shvarFile *ifcfg, GError **error)
> +{
> +	NMSettingIP4Config *s_ip4 = NULL;
> +
> +	s_ip4 = (NMSettingIP4Config *) nm_setting_ip4_config_new ();
> +	if (!s_ip4) {
> +		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
> +		             "Could not allocate IP4 setting");
> +		return NULL;
> +	}
> +
> +	g_object_set (s_ip4,
> +	              NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_DISABLED,
> +		      NULL);
> +
> +	return NM_SETTING (s_ip4);
> +}
>  
>  static NMSetting *
>  make_ip4_setting (shvarFile *ifcfg,
> @@ -3490,6 +3513,17 @@ is_bond_device (const char *name, shvarFile *parsed)
>  	return FALSE;
>  }
>  
> +static gboolean
> +connection_ignore_config (NMConnection *connection)
> +{
> +	NMSettingConnection *s_con;
> +
> +	s_con = NM_SETTING_CONNECTION( nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION));
> +	g_assert (s_con);
> +
> +	return nm_setting_connection_get_ignore_config (s_con);
> +}
> +
>  enum {
>  	IGNORE_REASON_NONE = 0x00,
>  	IGNORE_REASON_BRIDGE = 0x01,
> @@ -3672,6 +3706,18 @@ connection_from_file (const char *filename,
>  		goto done;
>  	}
>  
> +	/* Some connections such as bond slaves deliberately ignore the high level config */
> +	if (connection_ignore_config (connection)) {
> +		s_ip4 = make_disabled_ip4_setting (parsed, &error);
> +		if (error) {
> +			g_object_unref (connection);
> +			connection = NULL;
> +		} else
> +			nm_connection_add_setting (connection, s_ip4);
> +
> +		goto verify;
> +	}
> +
>  	s_ip6 = make_ip6_setting (parsed, network_file, iscsiadm_path, &error);
>  	if (error) {
>  		g_object_unref (connection);
> @@ -3710,6 +3756,7 @@ connection_from_file (const char *filename,
>  	}
>  	g_free (bootproto);
>  
> +verify:
>  	if (!nm_connection_verify (connection, &error)) {
>  		g_object_unref (connection);
>  		connection = NULL;