GNOME.org
 

Re: [PATCH 2/6] VLAN: add ifcfg-vlan parser

On Wed, 2011-11-16 at 16:21 +0800, Weiping Pan wrote:
> On 11/16/2011 01:35 PM, Dan Williams wrote:
> > On Fri, 2011-10-21 at 09:52 +0800, Weiping Pan wrote:
> >> The example of ifcfg-vlan is as followed:
> >>
> >> VLAN=yes
> >> TYPE=vlan
> >> DEVICE=vlan43
> >> PHYSDEV=eth9
> > Ideally here we also accept a MAC address or NM connection UUID as
> > PHYSDEV so you don't have to manually keep all the interface names in
> > sync.
> >
> Yes. I will put it in TODO list.
> >> REORDER_HDR=0
> >> VLAN_FLAGS=GVRP,LOOSE_BINDING
> >> VLAN_INGRESS_PRIORITY_MAP=0:1,2:5
> >> VLAN_EGRESS_PRIORITY_MAP=12:3,14:7
> > Are these new variables?  ifup (F16) seems to think the only ones that
> > exist are VLAN (yes/no), REORDER_HDR, and GVRP.
> >
> But vconfig supports ingress/egress priority mapping,
> so I think NetworkManager should support them, too.

Yes, no objection to the support, I was just curious about it as I
couldn't find anywhere in the existing ifcfg scripts that this value was
defined.  So having NM define that as an ifcfg variable is fine.

> >> ONBOOT=yes
> >> BOOTPROTO=static
> >> IPADDR=192.168.43.149
> >> NETMASK=255.255.255.0
> >>
> >> And we try to make it compitable with the format used by initscripts,
> >> and there is no need to change anything in ifcfg-eth9.
> >>
> >> Pay attention the format of DEVICE here is 'vlan+id', and id is 0-4095.
> >>
> >> Signed-off-by: Weiping Pan<wpan redhat com>
> >> ---
> >>   libnm-util/Makefile.am                             |    2 +
> >>   libnm-util/libnm-util.ver                          |   11 +
> >>   libnm-util/nm-connection.c                         |   25 ++-
> >>   libnm-util/nm-connection.h                         |    2 +
> >>   libnm-util/nm-setting-vlan.c                       |  351 ++++++++++++++++++++
> >>   libnm-util/nm-setting-vlan.h                       |   93 +++++
> >>   src/settings/plugins/ifcfg-rh/common.h             |    1 +
> >>   src/settings/plugins/ifcfg-rh/reader.c             |  143 ++++++++-
> >>   .../network-scripts/ifcfg-test-vlan-interface      |   11 +-
> >>   9 files changed, 622 insertions(+), 17 deletions(-)
> >>   create mode 100644 libnm-util/nm-setting-vlan.c
> >>   create mode 100644 libnm-util/nm-setting-vlan.h
> >>
> >> diff --git a/libnm-util/Makefile.am b/libnm-util/Makefile.am
> >> index 1529b4a..a6cf684 100644
> >> --- a/libnm-util/Makefile.am
> >> +++ b/libnm-util/Makefile.am
> >> @@ -17,6 +17,7 @@ libnm_util_include_HEADERS = 		\
> >>   	nm-setting-bluetooth.h		\
> >>   	nm-setting-connection.h		\
> >>   	nm-setting-ip4-config.h		\
> >> +	nm-setting-vlan.h		\
> >>   	nm-setting-ip6-config.h		\
> >>   	nm-setting-ppp.h		\
> >>   	nm-setting-pppoe.h		\
> >> @@ -46,6 +47,7 @@ libnm_util_la_csources = \
> >>   	nm-setting-bluetooth.c		\
> >>   	nm-setting-connection.c		\
> >>   	nm-setting-ip4-config.c		\
> >> +	nm-setting-vlan.c		\
> >>   	nm-setting-ip6-config.c		\
> >>   	nm-setting-ppp.c		\
> >>   	nm-setting-pppoe.c		\
> >> diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver
> >> index 53c2482..9f3033e 100644
> >> --- a/libnm-util/libnm-util.ver
> >> +++ b/libnm-util/libnm-util.ver
> >> @@ -30,6 +30,7 @@ global:
> >>   	nm_connection_get_setting_wired;
> >>   	nm_connection_get_setting_wireless;
> >>   	nm_connection_get_setting_wireless_security;
> >> +	nm_connection_get_setting_vlan;
> >>   	nm_connection_get_type;
> >>   	nm_connection_get_uuid;
> >>   	nm_connection_is_type;
> >> @@ -449,6 +450,16 @@ global:
> >>   	nm_utils_wifi_find_next_channel;
> >>   	nm_utils_wifi_freq_to_channel;
> >>   	nm_utils_wifi_is_channel_valid;
> >> +	nm_setting_vlan_error_get_type;
> >> +	nm_setting_vlan_error_quark;
> >> +	nm_setting_vlan_get_type;
> >> +	nm_setting_vlan_new;
> >> +	nm_setting_vlan_get_interface_name;
> >> +	nm_setting_vlan_get_vlan_slave;
> >> +	nm_setting_vlan_get_vlan_id;
> >> +	nm_setting_vlan_get_vlan_flags;
> >> +	nm_setting_vlan_get_vlan_priority_ingress_map;
> >> +	nm_setting_vlan_get_vlan_priority_egress_map;
> > We probably want to kill the second 'vlan' here since these functions
> > are already part of the vlan setting, we can just name them:
> >
> > 	nm_setting_vlan_get_slave;
> > 	nm_setting_vlan_get_id;
> > 	nm_setting_vlan_get_flags;
> > 	nm_setting_vlan_get_priority_ingress_map;
> > 	nm_setting_vlan_get_priority_egress_map;
> >
> Ok. I will remove the duplicate "vlan".
> 
> >>   local:
> >>   	*;
> >>   };
> >> diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c
> >> index e3bbeae..2137352 100644
> >> --- a/libnm-util/nm-connection.c
> >> +++ b/libnm-util/nm-connection.c
> >> @@ -44,6 +44,7 @@
> >>   #include "nm-setting-wireless-security.h"
> >>   #include "nm-setting-vpn.h"
> >>   #include "nm-setting-olpc-mesh.h"
> >> +#include "nm-setting-vlan.h"
> >>
> >>   #include "nm-setting-serial.h"
> >>   #include "nm-setting-gsm.h"
> >> @@ -135,7 +136,7 @@ static guint signals[LAST_SIGNAL] = { 0 };
> >>
> >>   static GHashTable *registered_settings = NULL;
> >>
> >> -#define DEFAULT_MAP_SIZE 16
> >> +#define DEFAULT_MAP_SIZE 17
> >>
> >>   static struct SettingInfo {
> >>   	const char *name;
> >> @@ -242,6 +243,11 @@ register_default_settings (void)
> >>   	                      NM_SETTING_WIMAX_ERROR,
> >>   	                      1, TRUE);
> >>
> >> +	register_one_setting (NM_SETTING_VLAN_SETTING_NAME,
> >> +	                      NM_TYPE_SETTING_VLAN,
> >> +	                      NM_SETTING_VLAN_ERROR,
> >> +	                      1, TRUE);
> >> +
> >>   	register_one_setting (NM_SETTING_WIRELESS_SECURITY_SETTING_NAME,
> >>   	                      NM_TYPE_SETTING_WIRELESS_SECURITY,
> >>   	                      NM_SETTING_WIRELESS_SECURITY_ERROR,
> >> @@ -1543,6 +1549,23 @@ nm_connection_get_setting_wireless_security (NMConnection *connection)
> >>   	return (NMSettingWirelessSecurity *) nm_connection_get_setting (connection, NM_TYPE_SETTING_WIRELESS_SECURITY);
> >>   }
> >>
> >> +/**
> >> + * nm_connection_get_setting_vlan:
> >> + * @connection: the #NMConnection
> >> + *
> >> + * A shortcut to return any #NMSettingVLAN the connection might contain.
> >> + *
> >> + * Returns: (transfer none): an #NMSettingVLAN if the connection contains one, otherwise NULL
> >> + **/
> >> +NMSettingVLAN *
> >> +nm_connection_get_setting_vlan (NMConnection *connection)
> >> +{
> >> +	g_return_val_if_fail (connection != NULL, NULL);
> >> +	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
> >> +
> >> +	return (NMSettingVLAN *) nm_connection_get_setting (connection, NM_TYPE_SETTING_VLAN);
> >> +}
> >> +
> >>   /*************************************************************/
> >>
> >>   static void
> >> diff --git a/libnm-util/nm-connection.h b/libnm-util/nm-connection.h
> >> index 069dc84..e573b8c 100644
> >> --- a/libnm-util/nm-connection.h
> >> +++ b/libnm-util/nm-connection.h
> >> @@ -45,6 +45,7 @@
> >>   #include<nm-setting-wired.h>
> >>   #include<nm-setting-wireless.h>
> >>   #include<nm-setting-wireless-security.h>
> >> +#include<nm-setting-vlan.h>
> >>
> >>   G_BEGIN_DECLS
> >>
> >> @@ -196,6 +197,7 @@ NMSettingWimax *           nm_connection_get_setting_wimax             (NMConnec
> >>   NMSettingWired *           nm_connection_get_setting_wired             (NMConnection *connection);
> >>   NMSettingWireless *        nm_connection_get_setting_wireless          (NMConnection *connection);
> >>   NMSettingWirelessSecurity *nm_connection_get_setting_wireless_security (NMConnection *connection);
> >> +NMSettingVLAN *            nm_connection_get_setting_vlan              (NMConnection *connection);
> >>
> >>   G_END_DECLS
> >>
> >> diff --git a/libnm-util/nm-setting-vlan.c b/libnm-util/nm-setting-vlan.c
> >> new file mode 100644
> >> index 0000000..7f104b3
> >> --- /dev/null
> >> +++ b/libnm-util/nm-setting-vlan.c
> >> @@ -0,0 +1,351 @@
> >> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> >> +
> >> +/*
> >> + * Weiping Pan<wpan redhat com>
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library 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
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, write to the
> >> + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> >> + * Boston, MA 02110-1301 USA.
> >> + *
> >> + * (C) Copyright 2011 Red Hat, Inc.
> >> + */
> >> +#include<stdio.h>
> >> +#include<string.h>
> >> +
> >> +#include<dbus/dbus-glib.h>
> >> +#include "nm-setting-vlan.h"
> >> +#include "nm-param-spec-specialized.h"
> >> +#include "nm-utils.h"
> >> +#include "nm-dbus-glib-types.h"
> >> +
> >> +/**
> >> + * SECTION:nm-setting-vlan
> >> + * @short_description: Describes connection properties for VLAN devices
> >> + * @include: nm-setting-vlan.h
> >> + *
> >> + * The #NMSettingVLAN object is a #NMSetting subclass that describes properties
> >> + * necessary for connection to VLAN devices.
> >> + **/
> >> +
> >> +/**
> >> + * nm_setting_vlan_error_quark:
> >> + * Registers an error quark for #NMSettingVLAN if necessary.
> >> + * Returns: the error quark used for #NMSettingVLAN errors.
> >> + **/
> >> +GQuark
> >> +nm_setting_vlan_error_quark (void)
> >> +{
> >> +	static GQuark quark;
> >> +
> >> +	if (G_UNLIKELY (!quark))
> >> +		quark = g_quark_from_static_string ("nm-setting-vlan-error-quark");
> >> +	return quark;
> >> +}
> >> +
> >> +/* This should really be standard. */
> >> +#define ENUM_ENTRY(NAME, DESC) { NAME, "" #NAME "", DESC }
> >> +
> >> +GType
> >> +nm_setting_vlan_error_get_type (void)
> >> +{
> >> +	static GType etype = 0;
> >> +
> >> +	if (etype == 0) {
> >> +		static const GEnumValue values[] = {
> >> +			/* Unknown error. */
> >> +			ENUM_ENTRY (NM_SETTING_VLAN_ERROR_UNKNOWN, "UnknownError"),
> >> +			/* The specified property was invalid. */
> >> +			ENUM_ENTRY (NM_SETTING_VLAN_ERROR_INVALID_PROPERTY, "InvalidProperty"),
> >> +			/* The specified property was missing and is required. */
> >> +			ENUM_ENTRY (NM_SETTING_VLAN_ERROR_MISSING_PROPERTY, "MissingProperty"),
> >> +			{ 0, 0, 0 }
> >> +		};
> >> +		etype = g_enum_register_static ("NMSettingVLANError", values);
> >> +	}
> >> +	return etype;
> >> +}
> >> +
> >> +G_DEFINE_TYPE (NMSettingVLAN, nm_setting_vlan, NM_TYPE_SETTING)
> > Lets actually use NMSettingVlan here instead of VLAN to keep consistency
> > with others like NMSettingWimax, NMSettingWifi, etc.  I made a mistake
> > with NMSettingVPN...
> Ok, I will use "vlan" and "XXXVlan" to keep consistency in the future.
> >> +#define NM_SETTING_VLAN_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_SETTING_VLAN, NMSettingVLANPrivate))
> >> +
> >> +typedef struct {
> >> +	char *interface_name;
> >> +	char *vlan_slave;
> >> +	guint32 vlan_id;
> >> +	guint32 vlan_flags;
> >> +	char *vlan_priority_ingress_map;
> >> +	char *vlan_priority_egress_map;
> > I'll comment on this later, but we probably want to store these
> > differently; instead of a string, if the format will always be
> > "<uint>:<uint>" it might make sense to store them that way so that at
> > least we can manipulate them as tuples rather than having to parse
> > strings in the future.  Will the ingress/egress mappings always be
> > <uint>:<uint>  or are there other possibilities?
> The ingress/egress mappings will always be <uint>:<uint>.

Ok, great.  Makes our life simpler.

> >> +} NMSettingVLANPrivate;
> >> +
> >> +enum {
> >> +	PROP_0,
> >> +	PROP_VLAN_DEVICE,
> >> +	PROP_VLAN_SLAVE,
> >> +	PROP_VLAN_ID,
> >> +	PROP_VLAN_FLAGS,
> >> +	PROP_VLAN_NAME_TYPE,
> >> +	PROP_VLAN_PRIORITY_INGRESS_MAP,
> >> +	PROP_VLAN_PRIORITY_EGRESS_MAP,
> >> +	LAST_PROP
> >> +};
> >> +
> >> +/**
> >> + * nm_setting_vlan_new:
> >> + *
> >> + * Creates a new #NMSettingVLAN object with default values.
> >> + *
> >> + * Returns: (transfer full): the new empty #NMSettingVLAN object
> >> + **/
> >> +NMSetting *
> >> +nm_setting_vlan_new (void)
> >> +{
> >> +	return (NMSetting *)g_object_new(NM_TYPE_SETTING_VLAN, NULL);
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_interface_name:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:interface_name property of the setting
> >> + **/
> >> +const char *
> >> +nm_setting_vlan_get_interface_name(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), NULL);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->interface_name;
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_vlan_slave:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:vlan_slave property of the setting
> >> + **/
> >> +const char *
> >> +nm_setting_vlan_get_vlan_slave(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), NULL);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->vlan_slave;
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_vlan_id:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:vlan_id property of the setting
> >> + **/
> >> +guint32
> >> +nm_setting_vlan_get_vlan_id(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), 0);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->vlan_id;
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_vlan_flags:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:vlan_flags property of the setting
> >> + **/
> >> +guint32
> >> +nm_setting_vlan_get_vlan_flags(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), 0);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->vlan_flags;
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_vlan_priority_ingress_map:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:vlan_priority_ingress_map property of the setting
> >> + **/
> >> +const char *
> >> +nm_setting_vlan_get_vlan_priority_ingress_map(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), NULL);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->vlan_priority_ingress_map;
> >> +}
> >> +
> >> +/**
> >> + * nm_setting_vlan_get_vlan_priority_egress_map:
> >> + * @setting: the #NMSettingVLAN
> >> + *
> >> + * Returns: the #NMSettingVLAN:vlan_priority_egress_map property of the setting
> >> + **/
> >> +const char *
> >> +nm_setting_vlan_get_vlan_priority_egress_map(NMSettingVLAN *setting)
> >> +{
> >> +	g_return_val_if_fail (NM_IS_SETTING_VLAN(setting), NULL);
> >> +	return NM_SETTING_VLAN_GET_PRIVATE(setting)->vlan_priority_egress_map;
> >> +}
> >> +
> >> +static void
> >> +nm_setting_vlan_init (NMSettingVLAN *setting)
> >> +{
> >> +	NMSettingVLANPrivate *priv = NULL;
> >> +
> >> +	priv = NM_SETTING_VLAN_GET_PRIVATE(setting);
> >> +	priv->vlan_id = 0;
> >> +	priv->vlan_flags = 0;
> >> +
> >> +	priv->vlan_priority_ingress_map = NULL;
> > The private structure actually gets memset() to 0, so we don't need to
> > explicitly initialize anything.  Properties can also get explicitly set
> > to their default value during construction if you pass G_PARAM_CONSTRUCT
> > as one of the flags when calling g_param_spec_xxxx().
> >
> Ok.
> >> +
> >> +	priv->vlan_priority_egress_map = NULL;
> >> +	g_object_set (setting, NM_SETTING_NAME, NM_SETTING_VLAN_SETTING_NAME, NULL);
> >> +
> >> +	return;
> >> +}
> >> +
> >> +static void
> >> +finalize (GObject *object)
> >> +{
> >> +	NMSettingVLAN *setting = NM_SETTING_VLAN (object);
> >> +	NMSettingVLANPrivate *priv = NM_SETTING_VLAN_GET_PRIVATE (setting);
> >> +
> >> +	g_free(priv->interface_name);
> >> +	g_free(priv->vlan_slave);
> >> +	g_free(priv->vlan_priority_ingress_map);
> >> +	g_free(priv->vlan_priority_egress_map);
> >> +	return;
> >> +}
> >> +
> >> +static void
> >> +set_property (GObject *object, guint prop_id,
> >> +		    const GValue *value, GParamSpec *pspec)
> >> +{
> >> +	NMSettingVLAN *setting = NM_SETTING_VLAN (object);
> >> +	NMSettingVLANPrivate *priv = NM_SETTING_VLAN_GET_PRIVATE (setting);
> >> +
> >> +	switch (prop_id) {
> >> +	case PROP_VLAN_DEVICE:
> >> +		g_free (priv->interface_name);
> >> +		priv->interface_name = g_value_dup_string (value);
> >> +		break;
> >> +	case PROP_VLAN_SLAVE:
> >> +		g_free (priv->vlan_slave);
> >> +		priv->vlan_slave = g_value_dup_string (value);
> >> +		break;
> >> +	case PROP_VLAN_ID:
> >> +		priv->vlan_id = g_value_get_uint(value);
> >> +		break;
> >> +	case PROP_VLAN_FLAGS:
> >> +		priv->vlan_flags |= g_value_get_uint(value);
> > You probably just want "priv->vlan_flags = xxx" here instead of OR-ing
> > with the existing value.
> >
> Ok.
> >> +		break;
> >> +	case PROP_VLAN_PRIORITY_INGRESS_MAP:
> >> +		priv->vlan_priority_ingress_map = g_value_dup_string(value);
> >> +		break;
> >> +	case PROP_VLAN_PRIORITY_EGRESS_MAP:
> >> +		priv->vlan_priority_egress_map = g_value_dup_string(value);
> >> +		break;
> >> +	default:
> >> +		break;
> >> +	}
> >> +}
> >> +
> >> +static void
> >> +get_property (GObject *object, guint prop_id,
> >> +		    GValue *value, GParamSpec *pspec)
> >> +{
> >> +	NMSettingVLAN *setting = NM_SETTING_VLAN (object);
> >> +
> >> +	switch (prop_id) {
> >> +	case PROP_VLAN_DEVICE:
> >> +		g_value_set_string(value, nm_setting_vlan_get_interface_name(setting));
> >> +		break;
> >> +	case PROP_VLAN_SLAVE:
> >> +		g_value_set_string(value, nm_setting_vlan_get_vlan_slave(setting));
> >> +		break;
> >> +	case PROP_VLAN_ID:
> >> +		g_value_set_uint(value, nm_setting_vlan_get_vlan_id(setting));
> >> +		break;
> >> +	case PROP_VLAN_FLAGS:
> >> +		g_value_set_uint(value, nm_setting_vlan_get_vlan_flags(setting));
> >> +		break;
> >> +	case PROP_VLAN_PRIORITY_INGRESS_MAP:
> >> +		g_value_set_string(value, nm_setting_vlan_get_vlan_priority_ingress_map(setting));
> >> +		break;
> >> +	case PROP_VLAN_PRIORITY_EGRESS_MAP:
> >> +		g_value_set_string(value, nm_setting_vlan_get_vlan_priority_egress_map(setting));
> >> +		break;
> >> +	default:
> >> +		break;
> >> +	}
> >> +}
> >> +
> >> +static void
> >> +nm_setting_vlan_class_init (NMSettingVLANClass *setting_class)
> >> +{
> >> +	GObjectClass *object_class = G_OBJECT_CLASS (setting_class);
> >> +	g_type_class_add_private (setting_class, sizeof (NMSettingVLANPrivate));
> >> +
> >> +	/* virtual methods */
> >> +	object_class->set_property = set_property;
> >> +	object_class->get_property = get_property;
> >> +	object_class->finalize     = finalize;
> >> +
> >> +	/* Properties */
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_DEVICE,
> >> +		g_param_spec_string(	NM_SETTING_VLAN_INTERFACE_NAME,
> >> +					"InterfaceName",
> >> +					"The vlan device name in kernel",
> >> +					NULL,
> >> +					G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_SLAVE,
> >> +		g_param_spec_string(	NM_SETTING_VLAN_VLAN_SLAVE,
> >> +					"vlan slave",
> >> +					"The underlying physical ethernet device",
> >> +					NULL,
> >> +					G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_ID,
> >> +		 g_param_spec_uint (NM_SETTING_VLAN_VLAN_ID,
> >> +						"vlan id",
> >> +						"vlan id",
> >> +						0,
> >> +						G_MAXUINT32,
> >> +						0,
> >> +						G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_FLAGS,
> >> +		 g_param_spec_uint (NM_SETTING_VLAN_VLAN_FLAGS,
> >> +						"vlan flags",
> >> +						"vlan flags",
> >> +						0,
> >> +						G_MAXUINT32,
> >> +						0,
> >> +						G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_PRIORITY_INGRESS_MAP,
> >> +		g_param_spec_string(	NM_SETTING_VLAN_VLAN_INGRESS_PRIORITY_MAP,
> >> +					"vlan ingress priority mapping",
> >> +					"vlan ingress priority mapping",
> >> +					NULL,
> >> +					G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +
> >> +	g_object_class_install_property
> >> +		(object_class, PROP_VLAN_PRIORITY_EGRESS_MAP,
> >> +		g_param_spec_string(	NM_SETTING_VLAN_VLAN_EGRESS_PRIORITY_MAP,
> >> +					"vlan egress priority mapping",
> >> +					"vlan egress priority mapping",
> >> +					NULL,
> >> +					G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_SERIALIZE));
> >> +}
> >> diff --git a/libnm-util/nm-setting-vlan.h b/libnm-util/nm-setting-vlan.h
> >> new file mode 100644
> >> index 0000000..c354db8
> >> --- /dev/null
> >> +++ b/libnm-util/nm-setting-vlan.h
> >> @@ -0,0 +1,93 @@
> >> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> >> +
> >> +/*
> >> + * Weiping Pan<wpan redhat com>
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library 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
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, write to the
> >> + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> >> + * Boston, MA 02110-1301 USA.
> >> + *
> >> + * (C) Copyright 2011 Red Hat, Inc.
> >> + */
> >> +
> >> +#ifndef NM_SETTING_VLAN_H
> >> +#define NM_SETTING_VLAN_H
> >> +
> >> +#include "nm-setting.h"
> >> +#include<linux/if_vlan.h>
> >> +
> >> +G_BEGIN_DECLS
> >> +
> >> +#define NM_TYPE_SETTING_VLAN            (nm_setting_vlan_get_type ())
> >> +#define NM_SETTING_VLAN(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_SETTING_VLAN, NMSettingVLAN))
> >> +#define NM_SETTING_VLAN_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_SETTING_VLANCONFIG, NMSettingVLANClass))
> >> +#define NM_IS_SETTING_VLAN(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), NM_TYPE_SETTING_VLAN))
> >> +#define NM_IS_SETTING_VLAN_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((obj), NM_TYPE_SETTING_VLAN))
> >> +#define NM_SETTING_VLAN_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_SETTING_VLAN, NMSettingVLANClass))
> >> +
> >> +#define NM_SETTING_VLAN_SETTING_NAME			"VLAN"
> > Lowercase here, ie 'vlan', for consistency with the rest of the
> > settings.
> >
> >> +/**
> >> + * NMSettingVlanError:
> >> + * @NM_SETTING_VLAN_ERROR_UNKNOWN: unknown or unclassified error
> >> + * @NM_SETTING_VLAN_ERROR_INVALID_PROPERTY: the property was invalid
> >> + * @NM_SETTING_VLAN_ERROR_MISSING_PROPERTY: the property was missing and is
> >> + * required
> >> + */
> >> +typedef enum {
> >> +	NM_SETTING_VLAN_ERROR_UNKNOWN = 0,
> >> +	NM_SETTING_VLAN_ERROR_INVALID_PROPERTY,
> >> +	NM_SETTING_VLAN_ERROR_MISSING_PROPERTY
> >> +} NMSettingVlanError;
> >> +
> >> +#define NM_TYPE_SETTING_VLAN_ERROR (nm_setting_vlan_error_get_type ())
> >> +GType nm_setting_vlan_error_get_type (void);
> >> +
> >> +#define NM_SETTING_VLAN_ERROR nm_setting_vlan_error_quark ()
> >> +GQuark nm_setting_vlan_error_quark (void);
> >> +
> >> +GType nm_setting_vlan_get_type (void);
> >> +
> >> +#define NM_SETTING_VLAN_INTERFACE_NAME 			"interface-name"
> >> +#define NM_SETTING_VLAN_VLAN_SLAVE 			"vlan-slave"
> >> +#define NM_SETTING_VLAN_VLAN_ID				"vlan-id"
> >> +#define NM_SETTING_VLAN_VLAN_FLAGS 			"vlan-flags"
> >> +#define NM_SETTING_VLAN_VLAN_INGRESS_PRIORITY_MAP 	"vlan-priority-ingress-map"
> >> +#define NM_SETTING_VLAN_VLAN_EGRESS_PRIORITY_MAP 	"vlan-priority-egress-map"
> > Same thing as with the accessor functions for these, lets just name them
> > as follows since they are already namespaced as part of the VLAN
> > setting:
> >
> > #define NM_SETTING_VLAN_SLAVE 			"slave"
> > #define NM_SETTING_VLAN_ID			"id"
> > #define NM_SETTING_VLAN_FLAGS 			"flags"
> >
> > We should also probably have an enum for the 'flags' somewhere, ie
> > called NMVlanFlags; I see later patches use VLAN_FLAG_REORDER_HDR but I
> > can't find a definition of that anywhere?  And lets make it a typedef
> > too so we don't have to keep writing 'enum NMVlanFlags' everywhere.
> >
> The definition is in /usr/include/linux/if_vlan.h.
>   33 enum vlan_flags {
>   34         VLAN_FLAG_REORDER_HDR   = 0x1,
>   35         VLAN_FLAG_GVRP          = 0x2,
>   36         VLAN_FLAG_LOOSE_BINDING = 0x4,
>   37 };

Ok, but that's kernel headers and we're letting that leak into the NM
public API.  I don't expect this to change in the future, but since it's
a "private" kernel header (things like this have changed before, ex WEXT
-> nl80211 for wifi) we should probably define an NM-specific enum type
for these flags and  mirror the kernel definitions.  I'd rather to do
that now than have to deal with the kernel stuff changing in the future.

> So we can make use of it instead of creating another 'enum NMVlanFlags'.
> >> +typedef struct {
> >> +	NMSetting parent;
> >> +} NMSettingVLAN;
> >> +
> >> +typedef struct {
> >> +	NMSettingClass parent;
> >> +
> >> +	/* Padding for future expansion */
> >> +	void (*_reserved1) (void);
> >> +	void (*_reserved2) (void);
> >> +	void (*_reserved3) (void);
> >> +	void (*_reserved4) (void);
> >> +} NMSettingVLANClass;
> >> +
> >> +NMSetting *nm_setting_vlan_new(void);
> >> +const char *nm_setting_vlan_get_interface_name(NMSettingVLAN *setting);
> >> +const char *nm_setting_vlan_get_vlan_slave(NMSettingVLAN *setting);
> >> +guint32 nm_setting_vlan_get_vlan_id(NMSettingVLAN *setting);
> >> +guint32 nm_setting_vlan_get_vlan_flags(NMSettingVLAN *setting);
> > Once we get an enum for flags, we should return that enum type from
> > nm_setting_vlan_get_vlan_flags() too.
> But this flag is the OR of three different enum type, so maybe 
> nm_setting_vlan_get_vlan_flags()
> should return gunit32.

True.  Leave it as a guint32 then.

Dan

> >> +const char *nm_setting_vlan_get_vlan_priority_ingress_map(NMSettingVLAN *setting);
> >> +const char *nm_setting_vlan_get_vlan_priority_egress_map(NMSettingVLAN *setting);
> >> +
> >> +G_END_DECLS
> >> +
> >> +#endif /* NM_SETTING_VLAN_H */
> >> diff --git a/src/settings/plugins/ifcfg-rh/common.h b/src/settings/plugins/ifcfg-rh/common.h
> >> index 74a77c4..90f9584 100644
> >> --- a/src/settings/plugins/ifcfg-rh/common.h
> >> +++ b/src/settings/plugins/ifcfg-rh/common.h
> >> @@ -44,6 +44,7 @@
> >>   #define TYPE_ETHERNET "Ethernet"
> >>   #define TYPE_WIRELESS "Wireless"
> >>   #define TYPE_BRIDGE   "Bridge"
> >> +#define TYPE_VLAN     "VLAN"
> >>
> >>   #define SECRET_FLAG_AGENT "user"
> >>   #define SECRET_FLAG_NOT_SAVED "ask"
> >> diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
> >> index 910cca3..7ba9509 100644
> >> --- a/src/settings/plugins/ifcfg-rh/reader.c
> >> +++ b/src/settings/plugins/ifcfg-rh/reader.c
> >> @@ -46,6 +46,7 @@
> >>   #include<NetworkManager.h>
> >>   #include<nm-setting-connection.h>
> >>   #include<nm-setting-ip4-config.h>
> >> +#include<nm-setting-vlan.h>
> >>   #include<nm-setting-ip6-config.h>
> >>   #include<nm-setting-wired.h>
> >>   #include<nm-setting-wireless.h>
> >> @@ -3298,6 +3299,130 @@ wired_connection_from_ifcfg (const char *file,
> >>   	return connection;
> >>   }
> >>
> >> +static NMSetting *
> >> +make_vlan_setting (shvarFile *ifcfg,
> >> +                    const char *file,
> >> +                    gboolean nm_controlled,
> >> +                    char **unmanaged,
> >> +                    NMSetting8021x **s_8021x,
> >> +                    GError **error)
> >> +{
> >> +	NMSettingVLAN *s_vlan = NULL;
> >> +	char *value = NULL;
> >> +	char *interface_name = NULL;
> >> +	char *vlan_slave = NULL;
> >> +	char *p = NULL;
> >> +	guint32 vlan_id = 0;
> >> +	guint32 vlan_flags = 0;
> >> +	char *vlan_priority_ingress_map = NULL;
> >> +	char *vlan_priority_egress_map = NULL;
> >> +
> >> +
> >> +	s_vlan = NM_SETTING_VLAN(nm_setting_vlan_new());
> >> +
> >> +	interface_name = svGetValue (ifcfg, "DEVICE", FALSE);
> >> +	if (!interface_name)
> >> +		goto error_return;
> >> +	else {
> >> +		if (!g_str_has_prefix(interface_name, "vlan"))
> >> +			goto free_interface_name;
> >> +
> >> +		g_object_set(s_vlan, NM_SETTING_VLAN_INTERFACE_NAME, interface_name, NULL);
> >> +
> >> +		p = interface_name + 4;
> >> +		vlan_id = g_ascii_strtoull(p, NULL, 10);
> >> +		if (vlan_id>= 0&&  vlan_id<  4096)
> >> +			g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_ID, vlan_id, NULL);
> >> +		else
> >> +			goto free_interface_name;
> >> +	}
> >> +
> >> +	vlan_slave = svGetValue (ifcfg, "PHYSDEV", FALSE);
> >> +	if (vlan_slave) {
> >> +		g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_SLAVE, vlan_slave, NULL);
> >> +		g_free(vlan_slave);
> >> +	}
> >> +
> >> +	value = svGetValue (ifcfg, "REORDER_HDR", FALSE);
> >> +	if (value)
> >> +		vlan_flags |= VLAN_FLAG_REORDER_HDR;
> > I couldn't find the definition of VLAN_FLAG_REORDER_HDR; did I just miss
> > that?
> >
> The definition is in /usr/include/linux/if_vlan.h.
> >> +	g_free(value);
> >> +
> >> +	value = svGetValue (ifcfg, "VLAN_FLAGS", FALSE);
> >> +	if (g_strcmp0("VLAN_FLAG_GVRP", value)) {
> >> +		vlan_flags |= VLAN_FLAG_GVRP;
> >> +	} else if (g_strcmp0("VLAN_FLAG_LOOSE_BINDING", value)) {
> >> +		vlan_flags |= VLAN_FLAG_LOOSE_BINDING;
> >> +	}
> >> +	g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_FLAGS, vlan_flags, NULL);
> >> +	g_free(value);
> >> +
> >> +	vlan_priority_ingress_map = svGetValue (ifcfg, "VLAN_INGRESS_PRIORITY_MAP", FALSE);
> >> +	if (vlan_priority_ingress_map) {
> >> +		g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_INGRESS_PRIORITY_MAP, vlan_priority_ingress_map, NULL);
> >> +		g_free(vlan_priority_ingress_map);
> >> +	}
> > So like I was talking about earlier, it's probably just easier to parse
> > the ingress/egress priorities when reading the ifcfg file in, rather
> > than having them as a string.  That probably means adding list iterator
> > functions like we have for IP addresses, nameservers, etc, and creating
> > a small opaque structure for them (though we could just store them
> > internally as a 32-bit number and pack both of the<uint>:<uint>  into
> > that, and save a lot of allocation).  So we'd then get something like:
> >
> > guint32   nm_setting_vlan_get_num_ingress_priorities (NMSettingVlan *setting);
> > gboolean  nm_setting_vlan_get_ingress_priority       (NMSettingVlan *setting, guint32 i, guint32 *out_a, guint32 *out_b);
> > gboolean  nm_setting_vlan_add_ingress_priority       (NMSettingVlan *setting, guint32 a, b);
> > void      nm_setting_vlan_remove_ingress_priority    (NMSettingVlan *setting, guint32 i);
> > void      nm_setting_vlan_clear_ingress_priorities   (NMSettingVlan *setting);
> >
> > for both ingress and egress; that'll be a lot easier to work with
> > internally in NM since we'll only have to parse it once and then we can
> > just iterate through and pull them out.
> >
> > It's a bit more complicated than that for the actual GObject properties
> > though (and thus D-Bus marshalling).  I vote we keep it simple and make
> > the GObject property type DBUS_TYPE_G_LIST_OF_STRING.  This makes it
> > easier to use from D-Bus, but it also means we need converters for the
> > get_property() and set_property() functions in nm-setting-vlan.c.  These
> > would take a GSList<string>  and parse each "<uint>:<uint>" element into
> > the right integers to pass to nm_setting_vlan_add_ingress_priority().
> > Then in set_property() we'd do the reverse and build up a GSList of
> > allocated strings using the ingress/egress priorites and use that in the
> > call to g_value_set_boxed ().  There are some examples of that in
> > various places, like the IPv4 config where we box/unbox (ie
> > marshall/demarshall) the property.  I can explain more here if you want.
> >
> > Thanks,
> > Dan
> >
> Ok, thanks
> Weiping Pan
> >> +	vlan_priority_egress_map = svGetValue (ifcfg, "VLAN_EGRESS_PRIORITY_MAP", FALSE);
> >> +	if (vlan_priority_egress_map) {
> >> +		g_object_set(s_vlan, NM_SETTING_VLAN_VLAN_EGRESS_PRIORITY_MAP, vlan_priority_egress_map, NULL);
> >> +		g_free(vlan_priority_egress_map);
> >> +	}
> >> +	return (NMSetting *) s_vlan;
> >> +
> >> +free_interface_name:
> >> +	g_free(interface_name);
> >> +error_return:
> >> +	g_object_unref(s_vlan);
> >> +	return NULL;
> >> +}
> >> +
> >> +static NMConnection *
> >> +vlan_connection_from_ifcfg (const char *file,
> >> +			     shvarFile *ifcfg,
> >> +			     gboolean nm_controlled,
> >> +			     char **unmanaged,
> >> +			     GError **error)
> >> +{
> >> +	NMConnection *connection = NULL;
> >> +	NMSetting *con_setting = NULL;
> >> +	NMSetting *vlan_setting = NULL;
> >> +	NMSetting8021x *s_8021x = NULL;
> >> +
> >> +	g_return_val_if_fail (file != NULL, NULL);
> >> +	g_return_val_if_fail (ifcfg != NULL, NULL);
> >> +
> >> +	connection = nm_connection_new ();
> >> +	if (!connection) {
> >> +		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
> >> +			     "Failed to allocate new connection for %s.", file);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	con_setting = make_connection_setting (file, ifcfg, NM_SETTING_VLAN_SETTING_NAME, NULL);
> >> +	if (!con_setting) {
> >> +		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
> >> +			     "Failed to create connection setting.");
> >> +		g_object_unref (connection);
> >> +		return NULL;
> >> +	}
> >> +	nm_connection_add_setting (connection, con_setting);
> >> +
> >> +	vlan_setting = make_vlan_setting (ifcfg, file, nm_controlled, unmanaged,&s_8021x, error);
> >> +	if (!vlan_setting) {
> >> +		g_object_unref (connection);
> >> +		return NULL;
> >> +	}
> >> +	nm_connection_add_setting (connection, vlan_setting);
> >> +
> >> +	if (!nm_connection_verify (connection, error)) {
> >> +		g_object_unref (connection);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	return connection;
> >> +}
> >> +
> >>   static gboolean
> >>   is_wireless_device (const char *iface)
> >>   {
> >> @@ -3342,7 +3467,6 @@ is_wireless_device (const char *iface)
> >>   enum {
> >>   	IGNORE_REASON_NONE = 0x00,
> >>   	IGNORE_REASON_BRIDGE = 0x01,
> >> -	IGNORE_REASON_VLAN = 0x02,
> >>   };
> >>
> >>   NMConnection *
> >> @@ -3456,7 +3580,7 @@ connection_from_file (const char *filename,
> >>   		g_free (lower);
> >>   	}
> >>
> >> -	/* Ignore BRIDGE= and VLAN= connections for now too (rh #619863) */
> >> +	/* Ignore BRIDGE= connections for now too (rh #619863) */
> >>   	tmp = svGetValue (parsed, "BRIDGE", FALSE);
> >>   	if (tmp) {
> >>   		g_free (tmp);
> >> @@ -3464,24 +3588,17 @@ connection_from_file (const char *filename,
> >>   		ignore_reason = IGNORE_REASON_BRIDGE;
> >>   	}
> >>
> >> -	if (nm_controlled) {
> >> -		tmp = svGetValue (parsed, "VLAN", FALSE);
> >> -		if (tmp) {
> >> -			g_free (tmp);
> >> -			nm_controlled = FALSE;
> >> -			ignore_reason = IGNORE_REASON_VLAN;
> >> -		}
> >> -	}
> >> -
> >>   	/* Construct the connection */
> >>   	if (!strcasecmp (type, TYPE_ETHERNET))
> >>   		connection = wired_connection_from_ifcfg (filename, parsed, nm_controlled, unmanaged,&error);
> >>   	else if (!strcasecmp (type, TYPE_WIRELESS))
> >>   		connection = wireless_connection_from_ifcfg (filename, parsed, nm_controlled, unmanaged,&error);
> >> -	else if (!strcasecmp (type, TYPE_BRIDGE)) {
> >> +	else if (!strcasecmp (type, TYPE_BRIDGE))
> >>   		g_set_error (&error, IFCFG_PLUGIN_ERROR, 0,
> >>   		             "Bridge connections are not yet supported");
> >> -	} else {
> >> +	else if (!strcasecmp (type, TYPE_VLAN))
> >> +		connection = vlan_connection_from_ifcfg (filename, parsed, nm_controlled, unmanaged,&error);
> >> +	else {
> >>   		g_set_error (&error, IFCFG_PLUGIN_ERROR, 0,
> >>   		             "Unknown connection type '%s'", type);
> >>   	}
> >> diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
> >> index 6c84185..d6d3f99 100644
> >> --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
> >> +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
> >> @@ -1,7 +1,12 @@
> >> -DEVICE=eth1.43
> >>   VLAN=yes
> >> +TYPE=vlan
> >> +DEVICE=vlan43
> >> +PHYSDEV=eth9
> >> +REORDER_HDR=0
> >> +VLAN_FLAGS=GVRP,LOOSE_BINDING
> >> +VLAN_INGRESS_PRIORITY_MAP=0:1,2:5
> >> +VLAN_EGRESS_PRIORITY_MAP=12:3,14:7
> >>   ONBOOT=yes
> >> -BOOTPROTO=none
> >> +BOOTPROTO=static
> >>   IPADDR=192.168.43.149
> >>   NETMASK=255.255.255.0
> >> -
> >
>
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]