Re: [PATCH 9/9] device: Enslave/release a link before bringing it up/down if belongs to a master

[Dan Williams <dcbw redhat com>]

On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:
> Signed-off-by: Thomas Graf <tgraf redhat com>

This one looks fine.

Dan

> ---
>  src/nm-device.c |   16 ++++
>  src/nm-system.c |  205 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  src/nm-system.h |    3 +
>  3 files changed, 214 insertions(+), 10 deletions(-)
> 
> diff --git a/src/nm-device.c b/src/nm-device.c
> index 02cd2a0..95024cc 100644
> --- a/src/nm-device.c
> +++ b/src/nm-device.c
> @@ -924,6 +924,7 @@ nm_device_activate_stage1_device_prepare (gpointer user_data)
>  	const char *iface;
>  	NMActStageReturn ret;
>  	NMDeviceStateReason reason = NM_DEVICE_STATE_REASON_NONE;
> +	NMDevice *master;
>  
>  	/* Clear the activation source ID now that this stage has run */
>  	activation_source_clear (self, FALSE, 0);
> @@ -934,6 +935,17 @@ nm_device_activate_stage1_device_prepare (gpointer user_data)
>  	nm_log_info (LOGD_DEVICE, "Activation (%s) Stage 1 of 5 (Device Prepare) started...", iface);
>  	nm_device_state_changed (self, NM_DEVICE_STATE_PREPARE, NM_DEVICE_STATE_REASON_NONE);
>  
> +	if ((master = nm_device_get_master (self))) {
> +		nm_device_hw_take_down (self, TRUE);
> +
> +		if (!nm_system_iface_enslave (self, master)) {
> +			nm_device_state_changed (self, NM_DEVICE_STATE_FAILED, reason);
> +			goto out;
> +		}
> +
> +		nm_device_hw_bring_up (self, TRUE, NULL);
> +	}
> +
>  	ret = NM_DEVICE_GET_CLASS (self)->act_stage1_prepare (self, &reason);
>  	if (ret == NM_ACT_STAGE_RETURN_POSTPONE) {
>  		goto out;
> @@ -2849,6 +2861,7 @@ nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
>  	NMDevice *self = NM_DEVICE (device);
>  	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
>  	NMDeviceStateReason ignored = NM_DEVICE_STATE_REASON_NONE;
> +	NMDevice *master;
>  	gboolean tried_ipv6 = FALSE;
>  	int ifindex, family;
>  
> @@ -2884,6 +2897,9 @@ nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
>  	if (NM_DEVICE_GET_CLASS (self)->deactivate)
>  		NM_DEVICE_GET_CLASS (self)->deactivate (self);
>  
> +	if ((master = nm_device_get_master (self)))
> +		nm_system_iface_release (self, master);
> +
>  	/* Tear down an existing activation request */
>  	clear_act_request (self);
>  
> diff --git a/src/nm-system.c b/src/nm-system.c
> index 1b8f224..4721bb4 100644
> --- a/src/nm-system.c
> +++ b/src/nm-system.c
> @@ -41,6 +41,8 @@
>  #include <glib.h>
>  #include <ctype.h>
>  #include <linux/if.h>
> +#include <linux/sockios.h>
> +#include <linux/if_bonding.h>
>  
>  #include "nm-system.h"
>  #include "nm-device.h"
> @@ -56,6 +58,7 @@
>  #include <netlink/netlink.h>
>  #include <netlink/utils.h>
>  #include <netlink/route/link.h>
> +#include <netlink/route/link/bonding.h>
>  
>  static void nm_system_device_set_priority (int ifindex,
>                                             NMIP4Config *config,
> @@ -644,15 +647,8 @@ nm_system_iface_set_up (int ifindex,
>  	return success;
>  }
>  
> -/**
> - * nm_system_iface_is_up:
> - * @ifindex: interface index
> - *
> - * Returns: %TRUE if the interface is up, %FALSE if it was down or the check
> - * failed.
> - **/
> -gboolean
> -nm_system_iface_is_up (int ifindex)
> +guint32
> +nm_system_iface_get_flags (int ifindex)
>  {
>  	const char *iface;
>  	struct rtnl_link *l;
> @@ -672,7 +668,20 @@ nm_system_iface_is_up (int ifindex)
>  	flags = rtnl_link_get_flags (l);
>  	rtnl_link_put (l);
>  
> -	return flags & IFF_UP;
> +	return flags;
> +}
> +
> +/**
> + * nm_system_iface_is_up:
> + * @ifindex: interface index
> + *
> + * Returns: %TRUE if the interface is up, %FALSE if it was down or the check
> + * failed.
> + **/
> +gboolean
> +nm_system_iface_is_up (int ifindex)
> +{
> +	return nm_system_iface_get_flags (ifindex) & IFF_UP;
>  }
>  
>  /**
> @@ -1207,6 +1216,182 @@ nm_system_add_bonding_master(NMSettingBond *setting)
>  	return TRUE;
>  }
>  
> +static gboolean
> +nm_system_iface_compat_enslave(NMDevice *slave, const char *master_name)
> +{
> +	struct ifreq ifr;
> +	int fd;
> +	gboolean ret = FALSE;
> +
> +	memset (&ifr, 0, sizeof(ifr));
> +
> +	fd = socket (PF_INET, SOCK_DGRAM, 0);
> +	if (fd < 0) {
> +		nm_log_err (LOGD_DEVICE, "couldn't open control socket.");
> +		return FALSE;
> +	}
> +
> +	strncpy (ifr.ifr_name, master_name, IFNAMSIZ);
> +	strncpy (ifr.ifr_slave, nm_device_get_iface (slave), IFNAMSIZ);
> +
> +	if (ioctl(fd, SIOCBONDENSLAVE, &ifr) < 0 &&
> +	    ioctl(fd, BOND_ENSLAVE_OLD, &ifr) < 0) {
> +		nm_log_err (LOGD_DEVICE, "(%s): error %d returned from "
> +			    "ioctl(SIOCBONDENSLAVE): %s",
> +			    master_name, errno, strerror(errno));
> +		goto errout;
> +	}
> +
> +	ret = TRUE;
> +
> +errout:
> +	close (fd);
> +
> +	return ret;
> +}
> +
> +/**
> + * nm_system_iface_enslave:
> + * @slave: Slave device
> + * @master: Master device
> + *
> + * Enslaves the 'slave' to 'master. This function targets implementing a
> + * generic interface to attaching all kinds of slaves to masters. Currently
> + * only bonding is properly supported due to the backwards compatibility
> + * function being bonding specific.
> + *
> + * The slave device needs to be down as a prerequirement.
> + *
> + * Returns: %TRUE on success, or %FALSE
> + */
> +gboolean
> +nm_system_iface_enslave(NMDevice *slave, NMDevice *master)
> +{
> +	struct nl_sock *sock;
> +	const char *master_name;
> +	int err, master_ifindex, slave_ifindex;
> +
> +	master_name = nm_device_get_iface (master);
> +	if (!master_name)
> +		return FALSE;
> +
> +	sock = nm_netlink_get_default_handle ();
> +
> +	master_ifindex = nm_netlink_iface_to_index (master_name);
> +	g_assert (master_ifindex > 0);
> +
> +	if (!(nm_system_iface_get_flags (master_ifindex) & IFF_MASTER)) {
> +		nm_log_err (LOGD_DEVICE, "(%s): %s is not a master",
> +			    master_name, master_name);
> +		return FALSE;
> +	}
> +
> +	slave_ifindex = nm_device_get_ifindex (slave);
> +	g_assert(slave_ifindex > 0);
> +
> +	g_assert (!nm_system_iface_is_up (slave_ifindex));
> +
> +	if (nm_system_iface_get_flags (slave_ifindex) & IFF_SLAVE) {
> +		nm_log_err (LOGD_DEVICE, "(%s): %s is already a slave",
> +			    master_name, nm_device_get_iface (slave));
> +		return FALSE;
> +	}
> +
> +	err = rtnl_link_bond_enslave_ifindex (sock, master_ifindex, slave_ifindex);
> +	if (err == -NLE_OPNOTSUPP)
> +		return nm_system_iface_compat_enslave(slave, master_name);
> +
> +	if (err < 0) {
> +		nm_log_err (LOGD_DEVICE, "(%s): error %d returned from "
> +		            "rtnl_link_bond_enslave(): %s",
> +			    master_name, err, nl_geterror(err));
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +static gboolean
> +nm_system_iface_compat_release(NMDevice *device, const char *master_name)
> +{
> +	struct ifreq ifr;
> +	int fd;
> +	gboolean ret = FALSE;
> +
> +	memset (&ifr, 0, sizeof(ifr));
> +
> +	fd = socket (PF_INET, SOCK_DGRAM, 0);
> +	if (fd < 0) {
> +		nm_log_err (LOGD_DEVICE, "couldn't open control socket.");
> +		return FALSE;
> +	}
> +
> +	strncpy (ifr.ifr_name, master_name, IFNAMSIZ);
> +	strncpy (ifr.ifr_slave, nm_device_get_iface (device), IFNAMSIZ);
> +
> +	if (ioctl(fd, SIOCBONDRELEASE, &ifr) < 0 &&
> +	    ioctl(fd, BOND_RELEASE_OLD, &ifr) < 0) {
> +		nm_log_err (LOGD_DEVICE, "(%s): error %d returned from "
> +			    "ioctl(SIOCBONDRELEASE): %s",
> +			    master_name, errno, strerror(errno));
> +		goto errout;
> +	}
> +
> +	ret = TRUE;
> +
> +errout:
> +	close (fd);
> +
> +	return ret;
> +}
> +
> +/**
> + * nm_system_iface_release:
> + * @slave: Slave device
> + * @maser: Master device
> + *
> + * Releases the 'slave' which is attached to 'master. This function targets
> + * implementing a generic interface to releasing all kinds of slaves. Currently
> + * only bonding is properly supported due to the backwards compatibility
> + * function being bonding specific.
> + *
> + * Returns: %TRUE on success, or %FALSE
> + */
> +gboolean
> +nm_system_iface_release(NMDevice *slave, NMDevice *master)
> +{
> +	struct nl_sock *sock;
> +	const char *master_name;
> +	int err, slave_ifindex;
> +
> +	master_name = nm_device_get_iface (master);
> +	if (!master_name)
> +		return TRUE;
> +
> +	sock = nm_netlink_get_default_handle ();
> +
> +	slave_ifindex = nm_device_get_ifindex (slave);
> +	g_assert(slave_ifindex > 0);
> +
> +	/* Only release if this is actually a slave */
> +	if (!(nm_system_iface_get_flags (slave_ifindex) & IFF_SLAVE))
> +		goto out;
> +
> +	err = rtnl_link_bond_release_ifindex (sock, slave_ifindex);
> +	if (err == -NLE_OPNOTSUPP)
> +		return nm_system_iface_compat_release(slave, master_name);
> +
> +	if (err < 0) {
> +		nm_log_err (LOGD_DEVICE, "(%s): error %d returned from "
> +		            "rtnl_link_bond_release(): %s",
> +			    master_name, err, nl_geterror(err));
> +		return FALSE;
> +	}
> +
> +out:
> +	return TRUE;
> +}
> +
>  /**
>   * nm_system_get_link_type:
>   * @name: name of link
> diff --git a/src/nm-system.h b/src/nm-system.h
> index 1fe91f6..339dfa7 100644
> --- a/src/nm-system.h
> +++ b/src/nm-system.h
> @@ -84,6 +84,7 @@ gboolean        nm_system_iface_set_up                  (int ifindex,
>                                                           gboolean up,
>                                                           gboolean *no_firmware);
>  
> +guint32		nm_system_iface_get_flags		(int ifindex);
>  gboolean        nm_system_iface_is_up                   (int ifindex);
>  
>  gboolean		nm_system_iface_set_mtu                 (int ifindex, guint32 mtu);
> @@ -91,6 +92,8 @@ 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);
> +gboolean		nm_system_iface_enslave		(NMDevice *slave, NMDevice *master);
> +gboolean		nm_system_iface_release		(NMDevice *slave, NMDevice *master);
>  
>  char *			nm_system_get_link_type		(const char *name);
>