Re: [PATCH 6/6] VLAN: create NMDeviceVLAN and activate it

[Dan Williams <dcbw redhat com>]

On Wed, 2011-11-16 at 17:19 +0800, Weiping Pan wrote:
> On 11/16/2011 02:10 PM, Dan Williams wrote:
> > On Fri, 2011-10-21 at 09:52 +0800, Weiping Pan wrote:
> >> When NetworkManager receives the message that a VLAN device is created in kernel,
> >> it should create corresponding NMDeviceVLAN object, and then activate it.
> > Instead of creating a new VLAN setting and passing it into
> > nm_device_vlan_new(), can't we just pass the interface name in?
> Ok.
> > The second issue is that this code would automatically detect VLAN
> > interfaces created outside NM; that's not extremely useful because the
> > tool that created the interface probably wants to set it up too, and if
> > it doesn't, then it should probably just be telling NM to set it up
> > instead of creating the VLAN interface and then telling NM to configure
> > it.  So we need to figure out what to do here so we don't step on
> > anyones toes and we interoperate in a way people expect.
> >
> > At the moment my thought is to make NM ignore any VLAN interfaces it
> > doesn't create - the simplest choice and the clearest from an
> > interoperability standpoint.
> >
> > If we recognize externally created VLAN interfaces, chances are that
> > we'll overwrite whatever the thing that created it did.  I'm not sure
> > there's a point to creating a VLAN and then telling NM to configure it,
> > so why not just have NM create the VLAN and configure it.  Taking over
> > existing configured devices is pretty tricky in most cases, and we only
> > support it for wired interfaces right now (and not for wired interfaces
> > that use 802.1x) because when taking over management of an interface we
> > need to take over *all* of it, including DHCP leases,
> > 802.1x/wpa_supplicnat, PPP control, etc.  And we don't do all of that
> > yet...
> >
> > Dan
> >
> I thought NetworkManager should control all interfaces of a host,
> no matter how they are created, then we can monitor states and traffics 
> of all interfaces.

NM recognizes most interfaces, but only exposes ones it can control over
D-Bus.  At the moment it doesn't make a lot of sense to expose an
interface over D-Bus that you can't actually do anything with (like
activate, deactivate, etc).  It may make sense to expose them for
statistics and such, but what I'd rather do is add full support for
those interfaces too, so that we don't expose the interface to D-Bus
clients and then say "well, you can get stats for it, but if you want to
actually control it you have to use shell scripts" or something like
that.  At that point, the client might as well just use shell scripts
for monitoring too.

But it is an interesting topic and something I'd like to explore more in
the future.  We may eventually find and expose all interfaces just for
monitoring like you suggest, but we need to more fully work out the
implications of that, like interaction with other tools and such.

Dan

> Ok, let me ignore external created vlan interfaces at present,
> we can handle them in the future.
> 
> thanks
> Weiping Pan
> >> Before activating a VLAN device, we should make sure that the underlying
> >> physical ethernet device is up.
> >> For example, before activating eth0.100, we should make sure that eth0 is up.
> >> NetworkManager calls g_udev_client_query_by_subsystem() to get the device
> >> list, and then activates each devices on that device list.
> >> And luckily I found that the physical ethernet device is prior to VLAN device
> >> on that device list, so there is nothing to do to make sure the activating
> >> sequence between a VLAN device and its underlying physical ethernet device.
> >>
> >> Signed-off-by: Weiping Pan<wpan redhat com>
> >> ---
> >>   src/nm-system.c       |   31 ++++++++++++++
> >>   src/nm-system.h       |    1 +
> >>   src/nm-udev-manager.c |  105 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   3 files changed, 136 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/src/nm-system.c b/src/nm-system.c
> >> index 8696000..62d0961 100644
> >> --- a/src/nm-system.c
> >> +++ b/src/nm-system.c
> >> @@ -1436,3 +1436,34 @@ free_cache:
> >>   	nl_cache_free(cache);
> >>   	return FALSE;
> >>   }
> >> +
> >> +/**
> >> + * nm_system_get_link_type:
> >> + * @name: name of link
> >> + *
> >> + * Lookup virtual link type. The returned string is allocated and needs
> >> + * to be freed after usage.
> >> + *
> >> + * Returns: Name of virtual link type or NULL if not a virtual link.
> >> + **/
> >> +char *
> >> +nm_system_get_link_type(const char *name)
> >> +{
> >> +	struct rtnl_link *result;
> >> +	struct nl_sock *nlh;
> >> +	char *type;
> >> +
> >> +	nlh = nm_netlink_get_default_handle ();
> >> +	if (!nlh)
> >> +		return NULL;
> >> +
> >> +	if (rtnl_link_get_kernel (nlh, 0, name,&result)<  0)
> >> +		return NULL;
> >> +
> >> +	if ((type = rtnl_link_get_type (result)))
> >> +		type = g_strdup(type);
> >> +
> >> +	rtnl_link_put (result);
> >> +
> >> +	return type;
> >> +}
> >> diff --git a/src/nm-system.h b/src/nm-system.h
> >> index 93f942c..2f0e65c 100644
> >> --- a/src/nm-system.h
> >> +++ b/src/nm-system.h
> >> @@ -92,4 +92,5 @@ gboolean		nm_system_iface_set_mac                 (int ifindex, const struct eth
> >>   gboolean		nm_system_add_vlan_device(NMSettingVLAN *setting);
> >>   gboolean		nm_system_del_vlan_device(NMSettingVLAN *setting);
> >>
> >> +char *			nm_system_get_link_type		(const char *name);
> >>   #endif
> >> diff --git a/src/nm-udev-manager.c b/src/nm-udev-manager.c
> >> index e8c6b82..3634423 100644
> >> --- a/src/nm-udev-manager.c
> >> +++ b/src/nm-udev-manager.c
> >> @@ -42,6 +42,12 @@
> >>   #include "nm-device-wimax.h"
> >>   #endif
> >>
> >> +#include "nm-system.h"
> >> +#include "nm-device-vlan.h"
> >> +#include "nm-setting-vlan.h"
> >> +#include "nm-netlink-monitor.h"
> >> +#include<netlink/route/link/vlan.h>
> >> +
> >>   typedef struct {
> >>   	GUdevClient *client;
> >>
> >> @@ -388,6 +394,90 @@ is_wimax (const char *driver)
> >>   	return g_strcmp0 (driver, "i2400m_usb") == 0;
> >>   }
> >>
> >> +static gboolean
> >> +is_vlan (const char *name)
> >> +{
> >> +	gboolean ret = FALSE;
> >> +	struct rtnl_link *search_link = NULL;
> >> +	struct nl_sock *nlh = NULL;
> >> +	struct nl_cache *cache = NULL;
> >> +
> >> +	nlh = nm_netlink_get_default_handle();
> >> +	if (!nlh)
> >> +		return FALSE;
> >> +
> >> +	if (rtnl_link_alloc_cache(nlh,&cache)<  0)
> >> +		return FALSE;
> >> +
> >> +	if (!cache)
> >> +		return FALSE;
> >> +
> >> +	search_link = rtnl_link_get_by_name(cache, name);
> >> +	if (!search_link)
> >> +		goto free_cache;
> >> +
> >> +	if (rtnl_link_is_vlan(search_link))
> >> +		ret = TRUE;
> >> +
> >> +	rtnl_link_put(search_link);
> >> +
> >> +	return ret;
> >> +
> >> +free_cache:
> >> +	nl_cache_free(cache);
> >> +	return FALSE;
> >> +}
> >> +
> >> +static NMSettingVLAN *
> >> +create_nm_settingvlan_from_gudev_device (GUdevDevice *device)
> >> +{
> >> +	struct rtnl_link *search_link = NULL;
> >> +	struct nl_sock *nlh = NULL;
> >> +	struct nl_cache *cache = NULL;
> >> +
> >> +	const char *interface_name;
> >> +	int vlan_id;
> >> +	int vlan_flags;
> >> +
> >> +	NMSettingVLAN * s_vlan = NULL;
> >> +
> >> +	interface_name = g_udev_device_get_name (device);
> >> +	g_assert (interface_name);
> >> +	nlh = nm_netlink_get_default_handle();
> >> +	if (!nlh)
> >> +		return FALSE;
> >> +
> >> +	if (rtnl_link_alloc_cache(nlh,&cache)<  0)
> >> +		return FALSE;
> >> +
> >> +	if (!cache)
> >> +		return FALSE;
> >> +
> >> +	search_link = rtnl_link_get_by_name(cache, interface_name);
> >> +	if (!search_link)
> >> +		goto free_cache;
> >> +
> >> +	if (rtnl_link_is_vlan(search_link)) {
> >> +		vlan_id = rtnl_link_vlan_get_id(search_link);
> >> +		vlan_flags = rtnl_link_vlan_get_flags(search_link);
> >> +		// TODO
> >> +		// ingress and egress priority mapping
> >> +	}
> >> +
> >> +	rtnl_link_put(search_link);
> >> +
> >> +	s_vlan = NM_SETTING_VLAN(nm_setting_vlan_new());
> >> +	g_object_set(s_vlan, NM_SETTING_VLAN_INTERFACE_NAME, interface_name, NULL);
> >> +	g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_ID, vlan_id, NULL);
> >> +	g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_FLAGS, vlan_flags, NULL);
> >> +
> >> +	return s_vlan;
> >> +
> >> +free_cache:
> >> +	nl_cache_free(cache);
> >> +	return NULL;
> >> +}
> >> +
> >>   static GObject *
> >>   device_creator (NMUdevManager *manager,
> >>                   GUdevDevice *udev_device,
> >> @@ -430,7 +520,15 @@ device_creator (NMUdevManager *manager,
> >>   	}
> >>
> >>   	if (!driver) {
> >> -		if (g_str_has_prefix (ifname, "easytether")) {
> >> +		char *type = NULL;
> >> +
> >> +		type = nm_system_get_link_type (ifname);
> >> +		if (type) {
> >> +			if (g_strcmp0 (type, "vlan") == 0)
> >> +				driver = "8021q";
> >> +
> >> +			g_free(type);
> >> +		} else if (g_str_has_prefix (ifname, "easytether")) {
> >>   			driver = "easytether";
> >>   		} else {
> >>   			nm_log_warn (LOGD_HW, "%s: couldn't determine device driver; ignoring...", path);
> >> @@ -452,6 +550,11 @@ device_creator (NMUdevManager *manager,
> >>   #if WITH_WIMAX
> >>   		device = (GObject *) nm_device_wimax_new (path, ifname, driver);
> >>   #endif
> >> +	} else if (is_vlan(ifname)) {
> >> +		NMSettingVLAN *s_vlan = NULL;
> >> +		s_vlan = (NMSettingVLAN *)create_nm_settingvlan_from_gudev_device(udev_device);
> >> +		device = (GObject *) nm_device_vlan_new(s_vlan);
> >> +		g_object_unref(s_vlan);
> >>   	} else
> >>   		device = (GObject *) nm_device_ethernet_new (path, ifname, driver);
> >>
> >
>