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

[Thomas Graf <tgraf redhat com>]

On Tue, 2011-10-11 at 09:18 -0500, Dan Williams wrote:
> On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:

> > +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.

OK

> 
> > +	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.

Agreed


> > +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.

Yep, I haven't looked into this yet. Your proposal seems fine.

> > +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.

Do we really want to delete the virtual kernel devices?

> > @@ -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.

I would prefer that as well but wanted to keep it simple for now to ease
a possible backport.

> > +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?

I don't see any other way but I think it's currently possible to have
two connections for the same bonding device. I'll verify and add a
check to prevent that.