Re: [PATCH] bonding: Prefix bonding connections with "Bond" and append slave suffix

[Dan Williams <dcbw redhat com>]

On Thu, 2011-11-10 at 14:16 +0100, Thomas Graf wrote:
> Moves the logic of naming connections into its own function. Allows each
> connection type to provide a "hard" prefix which will always be used.
> Bonding uses this to prefix all bonding connections with "Bond".
> 
> If a DEVICE= line is available, append it to the end of connection name
> for easier identification of the real device behind it.
> 
> Appends the suffix "[slave-of <MASTER>]" to all connections which are
> configured as a slave of a bond.

If NAME exists, we should use that verbatim and not attempt to change
it; otherwise if you have no NAME, NM will create one, and then it may
get saved, and then next time NM reads the file in you'll get something
like "Bond Bond (bond0) (bond0)".

So I took that part out but kept the block that creates the new name if
none exists.  Basically if NAME exists that should be exactly what gets
used as the connection ID.  NM should create a usable one if none
exists, but if one exists because a user added that, they get to make
sure it's what they want.

Dan

> Examples:
> 
>   myName                  -> myName (eth0)
>   System eth0             -> System eth0
>   myName2                 -> Bond myName2 (bond0)
>   System bond0            -> Bond bond0
>   myName                  -> myName (eth0) [slave-of bond0]
>   System eth0             -> System eth0 [slave-of bond0]
> 
> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  src/settings/plugins/ifcfg-rh/reader.c |   99 ++++++++++++++++++++++----------
>  1 files changed, 68 insertions(+), 31 deletions(-)
> 
> diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
> index 3cde2be..1f1c00c 100644
> --- a/src/settings/plugins/ifcfg-rh/reader.c
> +++ b/src/settings/plugins/ifcfg-rh/reader.c
> @@ -78,16 +78,68 @@ get_int (const char *str, int *value)
>  	return TRUE;
>  }
>  
> +static char *
> +make_connection_name (shvarFile *ifcfg, const char *ifcfg_name,
> +                      const char *suggested, const char *prefix)
> +{
> +	char *full_name = NULL, *name, *device;
> +
> +	name = svGetValue (ifcfg, "NAME", FALSE);
> +	device = svGetValue (ifcfg, "DEVICE", FALSE);
> +
> +	if (name && strlen (name)) {
> +		if (device) {
> +			if (prefix)
> +				full_name = g_strdup_printf ("%s %s (%s)", prefix, name, device);
> +			else
> +				full_name = g_strdup_printf ("%s (%s)", name, device);
> +		} else {
> +			if (prefix)
> +				full_name = g_strdup_printf ("%s %s", prefix, name);
> +			else
> +				full_name = g_strdup (name);
> +		}
> +	} else {
> +		/*
> +		 * No name was specified, construct a default connection name based
> +		 * on the information we have.
> +		 */
> +		if (!prefix)
> +			prefix = reader_get_prefix();
> +
> +		/* For cosmetic reasons, if the suggested name is the same as
> +		 * the ifcfg files name, don't use it.  Mainly for wifi so that
> +		 * the SSID is shown in the connection ID instead of just "wlan0".
> +		 */
> +		if (suggested && strcmp (ifcfg_name, suggested)) {
> +			if (device)
> +				full_name = g_strdup_printf ("%s %s (%s)", prefix, suggested, device);
> +			else
> +				full_name = g_strdup_printf ("%s %s (%s)", prefix, suggested, ifcfg_name);
> +		} else {
> +			if (device && strcmp (ifcfg_name, device))
> +				full_name = g_strdup_printf ("%s %s (%s)", prefix, ifcfg_name, device);
> +			else
> +				full_name = g_strdup_printf ("%s %s", prefix, ifcfg_name);
> +		}
> +	}
> +
> +	g_free (name);
> +	g_free (device);
> +
> +	return full_name;
> +}
> +
>  static NMSetting *
>  make_connection_setting (const char *file,
>                           shvarFile *ifcfg,
>                           const char *type,
> -                         const char *suggested)
> +                         const char *suggested,
> +                         const char *prefix)
>  {
>  	NMSettingConnection *s_con;
>  	const char *ifcfg_name = NULL;
> -	char *new_id = NULL, *uuid = NULL, *zone = NULL, *value;
> -	char *ifcfg_id;
> +	char *new_id, *uuid = NULL, *zone = NULL, *value;
>  
>  	ifcfg_name = utils_get_ifcfg_name (file, TRUE);
>  	if (!ifcfg_name)
> @@ -95,32 +147,9 @@ make_connection_setting (const char *file,
>  
>  	s_con = NM_SETTING_CONNECTION (nm_setting_connection_new ());
>  
> -	/* Try the ifcfg file's internally defined name if available */
> -	ifcfg_id = svGetValue (ifcfg, "NAME", FALSE);
> -	if (ifcfg_id && strlen (ifcfg_id))
> -		g_object_set (s_con, NM_SETTING_CONNECTION_ID, ifcfg_id, NULL);
> -
> -	if (!nm_setting_connection_get_id (s_con)) {
> -		if (suggested) {
> -			/* For cosmetic reasons, if the suggested name is the same as
> -			 * the ifcfg files name, don't use it.  Mainly for wifi so that
> -			 * the SSID is shown in the connection ID instead of just "wlan0".
> -			 */
> -			if (strcmp (ifcfg_name, suggested)) {
> -				new_id = g_strdup_printf ("%s %s (%s)", reader_get_prefix (), suggested, ifcfg_name);
> -				g_object_set (s_con, NM_SETTING_CONNECTION_ID, new_id, NULL);
> -			}
> -		}
> -
> -		/* Use the ifcfg file's name as a last resort */
> -		if (!nm_setting_connection_get_id (s_con)) {
> -			new_id = g_strdup_printf ("%s %s", reader_get_prefix (), ifcfg_name);
> -			g_object_set (s_con, NM_SETTING_CONNECTION_ID, new_id, NULL);
> -		}
> -	}
> -
> +	new_id = make_connection_name (ifcfg, ifcfg_name, suggested, prefix);
> +	g_object_set (s_con, NM_SETTING_CONNECTION_ID, new_id, NULL);
>  	g_free (new_id);
> -	g_free (ifcfg_id);
>  
>  	/* Try for a UUID key before falling back to hashing the file name */
>  	uuid = svGetValue (ifcfg, "UUID", FALSE);
> @@ -142,9 +171,17 @@ make_connection_setting (const char *file,
>  
>  	value = svGetValue (ifcfg, "MASTER", FALSE);
>  	if (value) {
> +		const char *id;
> +
>  		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);
> +
> +		/* Add a suffix to all slaves: "<NAME> [slave-of <MASTER>]" */
> +		id = nm_setting_connection_get_id (s_con);
> +		new_id = g_strdup_printf ("%s [slave-of %s]", id, value);
> +		g_object_set (s_con, NM_SETTING_CONNECTION_ID, new_id, NULL);
> +
>  		g_free (value);
>  	}
>  
> @@ -3051,7 +3088,7 @@ wireless_connection_from_ifcfg (const char *file,
>  	/* Connection */
>  	con_setting = make_connection_setting (file, ifcfg,
>  	                                       NM_SETTING_WIRELESS_SETTING_NAME,
> -	                                       printable_ssid);
> +	                                       printable_ssid, NULL);
>  	g_free (printable_ssid);
>  	if (!con_setting) {
>  		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
> @@ -3290,7 +3327,7 @@ wired_connection_from_ifcfg (const char *file,
>  		return NULL;
>  	}
>  
> -	con_setting = make_connection_setting (file, ifcfg, NM_SETTING_WIRED_SETTING_NAME, NULL);
> +	con_setting = make_connection_setting (file, ifcfg, NM_SETTING_WIRED_SETTING_NAME, NULL, NULL);
>  	if (!con_setting) {
>  		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
>  		             "Failed to create connection setting.");
> @@ -3454,7 +3491,7 @@ bond_connection_from_ifcfg (const char *file,
>  		return NULL;
>  	}
>  
> -	con_setting = make_connection_setting (file, ifcfg, NM_SETTING_BOND_SETTING_NAME, NULL);
> +	con_setting = make_connection_setting (file, ifcfg, NM_SETTING_BOND_SETTING_NAME, NULL, _("Bond"));
>  	if (!con_setting) {
>  		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
>  		             "Failed to create connection setting.");