Re: [PATCH 4/6] VLAN: add NMDeviceVLAN

[Dan Williams <dcbw redhat com>]

On Sun, 2011-11-20 at 16:54 +0800, Weiping Pan wrote:
> On 11/18/2011 04:28 AM, Dan Williams wrote:
> > On Wed, 2011-11-16 at 16:54 +0800, Weiping Pan wrote:
> >> On 11/16/2011 01:54 PM, Dan Williams wrote:
> >>> On Fri, 2011-10-21 at 09:52 +0800, Weiping Pan wrote:
> >>>> NMDeviceVLAN represents a VLAN device in NetworkManager.
> >>>> When a VLAN device is created in kernel, NetworkManager will receive the event
> >>>> and create a corresponding NMDeviceVLAN.
> >>> Like the setting, let's use NMDeviceVlan here for consistency.
> >>>
> >> Ok.
> >>>> Signed-off-by: Weiping Pan<wpan redhat com>
> >>>> ---
> >>>>   include/NetworkManager.h |    1 +
> >>>>   src/Makefile.am          |    2 +
> >>>>   src/nm-device-vlan.c     |  330 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>   src/nm-device-vlan.h     |   59 ++++++++
> >>>>   4 files changed, 392 insertions(+), 0 deletions(-)
> >>>>   create mode 100644 src/nm-device-vlan.c
> >>>>   create mode 100644 src/nm-device-vlan.h
> >>>>
> >>>> diff --git a/include/NetworkManager.h b/include/NetworkManager.h
> >>>> index 3522dd2..87d7d7f 100644
> >>>> --- a/include/NetworkManager.h
> >>>> +++ b/include/NetworkManager.h
> >>>> @@ -114,6 +114,7 @@ typedef enum {
> >>>>   	NM_DEVICE_TYPE_OLPC_MESH = 6,
> >>>>   	NM_DEVICE_TYPE_WIMAX     = 7,
> >>>>   	NM_DEVICE_TYPE_MODEM     = 8,
> >>>> +	NM_DEVICE_TYPE_VLAN      = 9,
> >>>>   } NMDeviceType;
> >>>>
> >>>>   /**
> >>>> diff --git a/src/Makefile.am b/src/Makefile.am
> >>>> index cbcfdc6..e086024 100644
> >>>> --- a/src/Makefile.am
> >>>> +++ b/src/Makefile.am
> >>>> @@ -126,6 +126,8 @@ NetworkManager_SOURCES = \
> >>>>   		nm-device-bt.h \
> >>>>   		nm-device-modem.h \
> >>>>   		nm-device-modem.c \
> >>>> +		nm-device-vlan.h \
> >>>> +		nm-device-vlan.c \
> >>>>   		nm-wifi-ap.c \
> >>>>   		nm-wifi-ap.h \
> >>>>   		nm-wifi-ap-utils.c \
> >>>> diff --git a/src/nm-device-vlan.c b/src/nm-device-vlan.c
> >>>> new file mode 100644
> >>>> index 0000000..23abdbc
> >>>> --- /dev/null
> >>>> +++ b/src/nm-device-vlan.c
> >>>> @@ -0,0 +1,330 @@
> >>>> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> >>>> +/* NetworkManager -- Network link manager
> >>>> + *
> >>>> + * This program is free software; you can redistribute it and/or modify
> >>>> + * it under the terms of the GNU General Public License as published by
> >>>> + * the Free Software Foundation; either version 2 of the License, or
> >>>> + * (at your option) any later version.
> >>>> + *
> >>>> + * This program is distributed in the hope that it will be useful,
> >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>> + * GNU General Public License for more details.
> >>>> + *
> >>>> + * You should have received a copy of the GNU General Public License along
> >>>> + * with this program; if not, write to the Free Software Foundation, Inc.,
> >>>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> >>>> + *
> >>>> + * Copyright (C) 2011 Red Hat, Inc.
> >>>> + */
> >>>> +
> >>>> +#include<string.h>
> >>>> +#include<glib.h>
> >>>> +
> >>>> +#include "nm-system.h"
> >>>> +#include "nm-device-vlan.h"
> >>>> +#include "nm-device-interface.h"
> >>>> +#include "nm-device-private.h"
> >>>> +#include "nm-properties-changed-signal.h"
> >>>> +#include "nm-marshal.h"
> >>>> +#include "nm-logging.h"
> >>>> +
> >>>> +static void device_interface_init (NMDeviceInterface *iface_class);
> >>>> +
> >>>> +G_DEFINE_TYPE_EXTENDED (NMDeviceVLAN, nm_device_vlan, NM_TYPE_DEVICE, 0,
> >>>> +                        G_IMPLEMENT_INTERFACE (NM_TYPE_DEVICE_INTERFACE, device_interface_init))
> >>>> +
> >>>> +#define NM_DEVICE_VLAN_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_DEVICE_VLAN, NMDeviceVLANPrivate))
> >>>> +
> >>>> +typedef struct {
> >>>> +	char *interface_name;
> >>>> +	guint32 vlan_id;
> >>>> +	guint32 vlan_flags;
> >>>> +	char *vlan_priority_ingress_map;
> >>>> +	char *vlan_priority_egress_map;
> >>>> +} NMDeviceVLANPrivate;
> >>>> +
> >>>> +enum {
> >>>> +	PROP_0,
> >>>> +	PROP_INTERFACE_NAME,
> >>>> +	PROP_VLAN_ID,
> >>>> +	PROP_VLAN_FLAGS,
> >>>> +	PROP_VLAN_PRIORITY_INGRESS_MAP,
> >>>> +	PROP_VLAN_PRIORITY_EGRESS_MAP,
> >>>> +	LAST_PROP
> >>>> +};
> >>> So for properties on the device (which get exported over D-Bus) it's
> >>> really only useful to have properties that clients are going to be
> >>> interested in; are all these properties interesting ones, or are they
> >>> mainly implementation details?  Clients can get most of these properties
> >>> from the NMSettingVlan itself since they aren't going to change while
> >>> the interface is activated.  Especially INTERFACE_NAME; that's already
> >>> handled by the parent class' 'interface' property.  Basically, if a
> >>> property isn't going to change while the interface is activated/up, it
> >>> probably shouldn't be a property of the device, but clients can get it
> >>> via the connection data itself.  We can always add device properties
> >>> later if we find clients want them for some reason, but we can't really
> >>> take them away.
> >>>
> >> When a vlan device is up, the client can only change
> >> +	PROP_VLAN_FLAGS,
> >> +	PROP_VLAN_PRIORITY_INGRESS_MAP,
> >> +	PROP_VLAN_PRIORITY_EGRESS_MAP,
> >>
> >> I test them with command vconfig.
> >>
> >> I thought the client could change the name of vlan device,
> >> either through "ip link set p2p4 name pwp", or  rtnl_link_set_name(),
> >> but both methods can work when the device is down.
> >> So, I can remove PROP_INTERFACE_NAME and PROP_VLAN_ID here.
> > Are flags likely to change?  How likely is it that these values would
> > change at runtime?  If we do expect them to change, and maybe expect to
> > add methods on the D-Bus interface to change ingress/egress priority
> > while the device is up, then we should rename the NMSettingVlan
> > properties to "default-priority-ingress-map" because the value from
> > NMSettingVlan is going to be the *initial* value, but could change
> > later.  Obviously in this case the NMConnection wouldn't change
> > on-the-fly (it's static configuration of course.
> >
> > There's a difference between the configuration values and the runtime
> > values.  The configuration stuff (ie, what's in the NMSetting and what's
> > stored on-disk) wouldn't be expected to change much.  But runtime
> > attributes of the interface might change and if those attributes are
> > non-essential to the network which that interface is connected to (by
> > which I mean, changing the attribute doesn't imply a change of the
> > network you're connected to  like SSIDs for wifi or MCC/MNC for 3G) then
> > we may want to expose them as properties.  We haven't done that yet in
> > NM, but there's no specific reason for that other than we haven't had
> > any particular need yet.
> >
> > I think that the ID and name are probably essential properties in that
> > they imply connection to a specific network, and given that the
> > interface must be down to change them, that probably does imply a
> > deactivate/activate cycle in NM to do so.  ie, you'd modify the
> > connection (either using a GUI editor or by editing the ifcfg file) and
> > then tell NM to bounce the interface
> Yes, these three values are not very likely to change,
> and ID and interface name can't be changed if the device is up,
> so we do not need a special NMDeviceVlan, just use NMDeviceEthernet like 
> bonding.

And I just remembered we can apparently also use VLANs on WiFi
interfaces, and having an NMDeviceVlan would preclude that...

Dan