GNOME.org
 

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

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.

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

> 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;

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

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

> +} 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().

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

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

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

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

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

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