Re: [PATCH 2/9] bonding: bonding master device creation

[Dan Williams <dcbw redhat com>]

On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:
> Creates virtual devices as needed when the manager is initialized or as
> needed if connections are added on the fly.
> 
> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  src/nm-manager.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  src/nm-system.c  |   31 ++++++++++++++++
>  src/nm-system.h  |    3 ++
>  3 files changed, 130 insertions(+), 5 deletions(-)
> 
> diff --git a/src/nm-manager.c b/src/nm-manager.c
> index 9200f34..b930f17 100644
> --- a/src/nm-manager.c
> +++ b/src/nm-manager.c
> @@ -925,12 +925,97 @@ get_active_connections (NMManager *manager, NMConnection *filter)
>  /* Settings stuff via NMSettings                                   */
>  /*******************************************************************/
>  
> +static gboolean
> +connection_needs_virtual_device (NMConnection *connection)
> +{
> +	NMSettingConnection *connection_setting;
> +	const char *type;
> +
> +	connection_setting = (NMSettingConnection *)
> +		nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION);
> +	g_assert(connection_setting);

One convention the code uses (except for really old code) is to prefix
the setting variables with "s_", like:

NMSettingConnection *s_con;
NMSettingWireless *s_wifi;

instead of eg "connection_setting" which was used long ago, but turns
out to be really long.

And for NM >= 0.9 we can do:

s_con = nm_connection_get_setting_connection (connection);

which is also shorter than nm_connection_get_setting (connection, XXX);
and the required cast.  But obviously that makes the patches harder to
backport for now.

> +	type = nm_setting_connection_get_connection_type (connection_setting);
> +	g_assert (type);
> +
> +	if (!strcmp (type, NM_SETTING_BOND_SETTING_NAME))
> +		return TRUE;

I'm also tempted to say we should just add:

gboolean nm_connection_is_type (NMConnection *connection, const char *type);

that does this for us, since this pattern is used in quite a few places
in the code.

> +	return FALSE;
> +}
> +
> +static gboolean
> +system_update_virtual_device (NMConnection *connection)
> +{
> +	NMSettingConnection *connection_setting;
> +	NMSetting *type_setting;
> +	const char *type;
> +
> +	connection_setting = (NMSettingConnection *)
> +		nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION);
> +	g_assert(connection_setting);
> +
> +	type = nm_setting_connection_get_connection_type (connection_setting);
> +	g_assert (type);
> +
> +	type_setting = nm_connection_get_setting_by_name (connection, type);
> +
> +	if (!strcmp (type, NM_SETTING_BOND_SETTING_NAME))
> +		return nm_system_add_bonding_master (NM_SETTING_BOND (type_setting));
> +
> +	return TRUE;
> +}
> +
>  static void
> -connections_changed (NMSettings *settings,
> +system_create_virtual_devices (NMSettings *settings)
> +{
> +	GSList *iter, *connections;
> +
> +	nm_log_info (LOGD_CORE, "Creating virtual devices");
> +
> +	connections = nm_settings_get_connections (settings);
> +	for (iter = connections; iter; iter = g_slist_next (iter)) {
> +		NMConnection *connection = NM_CONNECTION (iter->data);
> +
> +		if (connection_needs_virtual_device (connection))
> +			system_update_virtual_device (connection);
> +	}
> +
> +	g_slist_free (connections);
> +}
> +
> +static void
> +connection_added (NMSettings *settings,
>                       NMSettingsConnection *connection,
>                       NMManager *manager)
>  {
>  	bluez_manager_resync_devices (manager);
> +
> +	if (connection_needs_virtual_device (NM_CONNECTION (connection)))
> +		system_update_virtual_device (NM_CONNECTION (connection));
> +}
> +
> +static void
> +connection_changed (NMSettings *settings,
> +                     NMSettingsConnection *connection,
> +                     NMManager *manager)
> +{
> +	bluez_manager_resync_devices (manager);
> +
> +	/* FIXME: Some virtual devices may need to be updated in the future. */

This would cover renaming the bond; which gets interesting.  If the user
renames it, we don't get notification of the *old* name here, so we'd
need to keep track of all bonds NM created.  NM shouldn't remove *all*
bond interfaces that don't have a backing connection because the user
can always create new interfaces outside NM and that would be just mean
to have NM remove them.

So instead this gets more complicated.  We'd probably want to build up a
static list/hash of bond interface names that were created by NM and
then when we get changed/deleted info, run through the connection list,
create new bond interfaces that aren't in the name list/hash, and remove
interfaces that are in the list/hash but no longer have a backing
connection.  Not a common use-case, but easy enough to trigger that we
want it to work.

> +}
> +
> +static void
> +connection_removed (NMSettings *settings,
> +                     NMSettingsConnection *connection,
> +                     NMManager *manager)
> +{
> +	bluez_manager_resync_devices (manager);
> +
> +	/*
> +	 * Do not delete existing virtual devices to keep connectivity up.
> +	 * Virtual devices are reused when NetworkManager is restarted.
> +	 */

Well, we probably do want to remove the bond here, but when we're
shutting down NM is when we don't want to kill the bonds.  ie, we want
to make sure that 'service NetworkManager restart' preserves all
connectivity; that's what the 'assumed' stuff throughout the code is,
and what the manager does during startup when it finds a connection it
can take over.  There's various bits of the code that indicate when
we're shutting down, and what we could do here is figure out the
shutdown sequence and just disconnect the 'connection_removed' signal
handler early in shutdown so we don't trigger it and kill bond
interfaces then.

>  }
>  
>  static void
> @@ -2823,6 +2908,12 @@ nm_manager_start (NMManager *self)
>  
>  	nm_udev_manager_query_devices (priv->udev_mgr);
>  	bluez_manager_resync_devices (self);
> +
> +	/*
> +	 * Connections added before the manager is started do not emit
> +	 * connection-added signals thus devices have to be created manually.
> +	 */
> +	system_create_virtual_devices (priv->settings);
>  }

Hmm, we should investigate that; this is fine but we should probably
make sure that the manager is all set up by the time.  Looking at the
code it looks like most plugins don't load the connections until later,
*but* checking the unmanaged devices actually causes the connections to
be loaded, so yeah, they'll be loaded before the manager is created.

Could fix that by moving some of the bits of add_plugin() to a new
function that might trigger loading, and then calling that from the
manager when the manager is ready.  We just want NMSettings object setup
to fail immediately if a specified plugin cannot be loaded, we don't
care about failing immediately if the plugin can't load unmanaged info
or connections.

But we can do that later.

>  static gboolean
> @@ -3095,13 +3186,13 @@ nm_manager_get (NMSettings *settings,
>  	g_signal_connect (priv->settings, "notify::" NM_SETTINGS_HOSTNAME,
>  	                  G_CALLBACK (system_hostname_changed_cb), singleton);
>  	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_ADDED,
> -	                  G_CALLBACK (connections_changed), singleton);
> +	                  G_CALLBACK (connection_added), singleton);
>  	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_UPDATED,
> -	                  G_CALLBACK (connections_changed), singleton);
> +	                  G_CALLBACK (connection_changed), singleton);
>  	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_REMOVED,
> -	                  G_CALLBACK (connections_changed), singleton);
> +	                  G_CALLBACK (connection_removed), singleton);
>  	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_VISIBILITY_CHANGED,
> -	                  G_CALLBACK (connections_changed), singleton);
> +	                  G_CALLBACK (connection_changed), singleton);
>  
>  	dbus_g_connection_register_g_object (bus, NM_DBUS_PATH, G_OBJECT (singleton));
>  
> diff --git a/src/nm-system.c b/src/nm-system.c
> index 473fcec..2d0393a 100644
> --- a/src/nm-system.c
> +++ b/src/nm-system.c
> @@ -1175,3 +1175,34 @@ nm_system_device_set_priority (int ifindex,
>  		rtnl_route_put (found);
>  	}
>  }
> +
> +/**
> + * nm_system_add_bonding_master:
> + * @setting: bonding setting
> + *
> + * Adds a virtual bonding device if it does not exist yet.
> + *
> + * Returns: %TRUE on success, %FALSE on failure
> + */
> +gboolean
> +nm_system_add_bonding_master(NMSettingBond *setting)
> +{
> +	struct nl_sock *sock;
> +	const char *name;
> +	int err;
> +
> +	sock = nm_netlink_get_default_handle ();
> +	name = nm_setting_bond_get_device_name (setting);
> +	g_assert (name);
> +
> +	/* Existing bonding devices with matching name will be reused */
> +	err = rtnl_link_bond_add (sock, name, NULL);
> +	if (err < 0) {
> +		nm_log_err (LOGD_DEVICE, "(%s): error %d returned from "
> +			    "rtnl_link_bond_add(): %s",
> +			    name, err, nl_geterror (err));
> +		return FALSE;
> +	}

I'm a bit wary of name collisions here; maybe that's just something we
suck up.  I suppose we define that you can't have two different bond
configurations with the same bond name or something?

Dan

> +	return TRUE;
> +}
> diff --git a/src/nm-system.h b/src/nm-system.h
> index ae4e7d9..a606503 100644
> --- a/src/nm-system.h
> +++ b/src/nm-system.h
> @@ -30,6 +30,7 @@
>  #include <glib.h>
>  #include "nm-device.h"
>  #include "nm-ip4-config.h"
> +#include "nm-setting-bond.h"
>  
>  /* Prototypes for system/distribution dependent functions,
>   * implemented in the backend files in backends/ directory
> @@ -89,4 +90,6 @@ gboolean		nm_system_iface_set_mtu                 (int ifindex, guint32 mtu);
>  
>  gboolean		nm_system_iface_set_mac                 (int ifindex, const struct ether_addr *mac);
>  
> +gboolean		nm_system_add_bonding_master	(NMSettingBond *setting);
> +
>  #endif