[PATCH 1/1] add configurable support for bridge forward delay

[PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]


the default learning time of a bridge -- 15 seconds -- appears
unacceptably long, especially for clients that disconnect quite
often after a short time (e.g. web-browsers);
this patch tries to remedy this by setting the the waiting period to a
value of zero (by default) or -- if the administrator wishes a larger
value -- to whatever the General section of the network.conf holds in
the variable ForwardDelay;
---
 network/bridge.c  |   97
++++++++++++++++++++++++++++++++++++++++++++++++++++-
 network/bridge.h  |    3 +-
 network/manager.c |   19 +++++++++--
 3 files changed, 114 insertions(+), 5 deletions(-)

[-- Attachment #2: 771fcb1abb3b5ba53a589816ad8bbb1d21e23a01.diff --]
[-- Type: text/x-patch, Size: 5947 bytes --]

diff --git a/network/bridge.c b/network/bridge.c
index f3528ad..842cd02 100644
--- a/network/bridge.c
+++ b/network/bridge.c
@@ -25,6 +25,8 @@
 #include <config.h>
 #endif
 
+#include <stdio.h>
+#include <stdlib.h>
 #include <errno.h>
 #include <unistd.h>
 #include <string.h>
@@ -33,6 +35,7 @@
 #include <sys/types.h>
 #include <net/if.h>
 #include <linux/sockios.h>
+#include <linux/if_bridge.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/l2cap.h>
@@ -43,10 +46,95 @@
 #include "common.h"
 
 static int bridge_socket = -1;
+static unsigned long int forward_delay = 1500;
 static const char *gn_bridge = NULL;
 static const char *nap_bridge = NULL;
 
-int bridge_init(const char *gn_iface, const char *nap_iface)
+/**
+ * convert the given string to the corresponding value in 
+ * jiffies; small helper function since a bridge's forward
+ * delay is expressed in jiffies;
+ *
+ * @param str	string to convert
+ * @param val	converted value
+ * @return	0 on success, -ERANGE otherwise
+ */
+static int __str_to_jiffies(const char *str, unsigned long int *val)
+{
+	double d;
+
+	if (!val)
+		return -EINVAL;
+
+	errno = 0;
+	d = strtod(str, NULL);
+
+	if (errno) {
+		error("unable to convert string to double: %s (%d)",
+				strerror(errno), errno);
+		return -errno;
+	}
+
+	if (0 > d)
+		d *= -1;
+	*val = d * 100UL;
+
+	return 0;
+}
+
+/**
+ * set the forward_delay of a bridge;
+ *
+ * @param id 		id of the bridge to manipulate
+ * @param delay		value of the forward delay to use (jiffies!)
+ * @return		0 on success,
+ * 			-EINVAL if id was invalid
+ * 			-errno if fprintf or ioctl failed
+ */
+static int __set_br_fwd_delay(const int id, const unsigned long int delay)
+{
+	int ret, *ptr = NULL;
+	static int done[] = { 0, 0 };
+	struct ifreq ifr;
+	const char *name;
+	unsigned long data[4] = { BRCTL_SET_BRIDGE_FORWARD_DELAY,
+			delay, 0, 0 };
+
+	name = bridge_get_name(id);
+	/* invalid id */
+	if (!name)
+		return -EINVAL;
+
+	if (BNEP_SVC_NAP == id)
+		ptr = &done[0];
+	else if (BNEP_SVC_GN == id)
+		ptr = &done[1];
+	else
+		return -EINVAL;
+
+	/* operation already done... */
+	if (1 == *ptr)
+		return 0;
+
+	strncpy(ifr.ifr_name, name, IFNAMSIZ);
+	ifr.ifr_data = (char *)&data;
+	// FIXME ioctl is marked deprecated, and might disappear...
+	ret = ioctl(bridge_socket, SIOCDEVPRIVATE, &ifr);
+
+	if (0 > ret)
+		/* failed to set forward_delay */
+		ret = -errno;
+	else {
+		/* success, remember we set forward_delay */
+		*ptr = 1;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+int bridge_init(const char *gn_iface, const char *nap_iface,
+	const char *fwd_delay)
 {
 #if 0
 	struct stat st;
@@ -64,6 +152,9 @@ int bridge_init(const char *gn_iface, const char *nap_iface)
 	gn_bridge = gn_iface;
 	nap_bridge = nap_iface;
 
+	/* ignore return value, default is 1500 anyway... */
+	__str_to_jiffies(fwd_delay, &forward_delay);
+
 	return 0;
 }
 
@@ -132,6 +223,10 @@ int bridge_add_interface(int id, const char *dev)
 	err = bnep_if_up(name, id);
 	if (err < 0)
 		return err;
+	
+	err = __set_br_fwd_delay(id, forward_delay);
+	if (err < 0)
+		return err;
 
 	return 0;
 }
diff --git a/network/bridge.h b/network/bridge.h
index f241148..4c18baf 100644
--- a/network/bridge.h
+++ b/network/bridge.h
@@ -21,7 +21,8 @@
  *
  */
 
-int bridge_init(const char *gn_iface, const char *nap_iface);
+int bridge_init(const char *gn_iface, const char *nap_iface,
+	const char *forward_delay);
 void bridge_cleanup(void);
 
 int bridge_create(int id);
diff --git a/network/manager.c b/network/manager.c
index bc84de0..47eb641 100644
--- a/network/manager.c
+++ b/network/manager.c
@@ -46,6 +46,7 @@
 #define IFACE_PREFIX "bnep%d"
 #define GN_IFACE  "pan0"
 #define NAP_IFACE "pan1"
+#define FORWARD_DELAY "0"
 
 static struct btd_adapter_driver network_panu_server_driver;
 static struct btd_adapter_driver network_gn_server_driver;
@@ -58,6 +59,7 @@ static struct network_conf {
 	gboolean server_enabled;
 	gboolean security;
 	char *iface_prefix;
+	char *forward_delay;
 	char *panu_script;
 	char *gn_script;
 	char *nap_script;
@@ -68,6 +70,7 @@ static struct network_conf {
 	.server_enabled = TRUE,
 	.security = TRUE,
 	.iface_prefix = NULL,
+	.forward_delay = NULL,
 	.panu_script = NULL,
 	.gn_script = NULL,
 	.nap_script = NULL,
@@ -78,6 +81,7 @@ static struct network_conf {
 static void conf_cleanup(void)
 {
 	g_free(conf.iface_prefix);
+	g_free(conf.forward_delay);
 	g_free(conf.panu_script);
 	g_free(conf.gn_script);
 	g_free(conf.nap_script);
@@ -122,6 +126,13 @@ static void read_config(const char *file)
 		g_clear_error(&err);
 	}
 
+	conf.forward_delay = g_key_file_get_string(keyfile, "General",
+						"ForwardDelay", &err);
+	if (err) {
+		debug("%s: %s", file, err->message);
+		g_clear_error(&err);
+	}
+
 #if 0
 	conf.panu_script = g_key_file_get_string(keyfile, "PANU Role",
 						"Script", &err);
@@ -176,13 +187,15 @@ done:
 		conf.gn_iface = g_strdup(GN_IFACE);
 	if (!conf.nap_iface)
 		conf.nap_iface = g_strdup(NAP_IFACE);
+	if (!conf.forward_delay)
+		conf.forward_delay = g_strdup(FORWARD_DELAY);
 
 	debug("Config options: InterfacePrefix=%s, PANU_Script=%s, "
 		"GN_Script=%s, NAP_Script=%s, GN_Interface=%s, "
-		"NAP_Interface=%s, Security=%s",
+		"NAP_Interface=%s, Security=%s, ForwardDelay=%s",
 		conf.iface_prefix, conf.panu_script, conf.gn_script,
 		conf.nap_script, conf.gn_iface, conf.nap_iface,
-		conf.security ? "true" : "false");
+		conf.security ? "true" : "false", conf.forward_delay);
 }
 
 static int network_probe(struct btd_device *device, GSList *uuids, uint16_t id)
@@ -343,7 +356,7 @@ int network_manager_init(DBusConnection *conn)
 	 * (setup connection request) contains the destination service
 	 * field that defines which service the source is connecting to.
 	 */
-	if (bridge_init(conf.gn_iface, conf.nap_iface) < 0) {
+	if (bridge_init(conf.gn_iface, conf.nap_iface, conf.forward_delay) < 0) {
 		error("Can't init bridge module");
 		return -1;
 	}

[PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]


the default learning time of a bridge -- 15 seconds -- appears
unacceptably long, especially for clients that disconnect quite
often after a short time (e.g. web-browsers);
this patch tries to remedy this by setting the the waiting period to a
value of zero (by default) or -- if the administrator wishes a larger
value -- to whatever the General section of the network.conf holds in
the variable ForwardDelay;
---
 Makefile.am       |    2 +-
 acinclude.m4      |    6 +++
 configure.ac      |    1 +
 network/bridge.c  |  117
++++++++++++++++++++++++++++++++++++++++++++++++++++-
 network/bridge.h  |    3 +-
 network/manager.c |   19 +++++++-
 6 files changed, 142 insertions(+), 6 deletions(-)

[-- Attachment #2: 46dfe45150c2a82d911ddabc81e13c3dc7b8c164.diff --]
[-- Type: text/x-patch, Size: 7771 bytes --]

diff --git a/Makefile.am b/Makefile.am
index f8111a9..0d8e2e1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -305,7 +305,7 @@ EXTRA_DIST += doc/manager-api.txt \
 
 AM_YFLAGS = -d
 
-AM_CFLAGS = @DBUS_CFLAGS@ @GLIB_CFLAGS@ @CAPNG_CFLAGS@ \
+AM_CFLAGS = @DBUS_CFLAGS@ @GLIB_CFLAGS@ @CAPNG_CFLAGS@ @SYSFS_CFLAGS@ \
 		-DBLUETOOTH_PLUGIN_BUILTIN -DPLUGINDIR=\""$(plugindir)"\"
 
 INCLUDES = -I$(builddir)/lib -I$(builddir)/src -I$(srcdir)/src \
diff --git a/acinclude.m4 b/acinclude.m4
index 7ce2588..f2b6aa2 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -109,6 +109,12 @@ AC_DEFUN([AC_PATH_GLIB], [
 	AC_SUBST(GLIB_LIBS)
 ])
 
+AC_DEFUN([AC_PATH_SYSFS], [
+	PKG_CHECK_MODULES(SYSFS, libsysfs >= 1.3.0, dummy=yes,
+				AC_MSG_ERROR(libsysfs library version 1.3.0 or later is required))
+	AC_SUBST(SYSFS_CFLAGS)
+])
+
 AC_DEFUN([AC_PATH_GSTREAMER], [
 	PKG_CHECK_MODULES(GSTREAMER, gstreamer-0.10 gstreamer-plugins-base-0.10, gstreamer_found=yes, gstreamer_found=no)
 	AC_SUBST(GSTREAMER_CFLAGS)
diff --git a/configure.ac b/configure.ac
index 199d2ad..afff4f9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -35,6 +35,7 @@ AC_FUNC_PPOLL
 AC_CHECK_LIB(dl, dlopen, dummy=yes,
 			AC_MSG_ERROR(dynamic linking loader is required))
 
+AC_PATH_SYSFS
 AC_PATH_DBUS
 AC_PATH_GLIB
 AC_PATH_ALSA
diff --git a/network/bridge.c b/network/bridge.c
index f3528ad..1dc09ca 100644
--- a/network/bridge.c
+++ b/network/bridge.c
@@ -25,6 +25,8 @@
 #include <config.h>
 #endif
 
+#include <stdio.h>
+#include <stdlib.h>
 #include <errno.h>
 #include <unistd.h>
 #include <string.h>
@@ -33,6 +35,8 @@
 #include <sys/types.h>
 #include <net/if.h>
 #include <linux/sockios.h>
+#include <linux/if_bridge.h>
+#include <sysfs/libsysfs.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/l2cap.h>
@@ -43,10 +47,114 @@
 #include "common.h"
 
 static int bridge_socket = -1;
+static unsigned long int forward_delay = 1499;
 static const char *gn_bridge = NULL;
 static const char *nap_bridge = NULL;
 
-int bridge_init(const char *gn_iface, const char *nap_iface)
+/**
+ * convert the given string to the corresponding value in 
+ * jiffies; small helper function since a brige's forward
+ * delay is expressed in jiffies;
+ *
+ * @param str	string to convert
+ * @param val	converted value
+ * @return	0 on success, -ERANGE otherwise
+ */
+static int __str_to_jiffies(const char *str, unsigned long int *val)
+{
+	double d;
+
+	if (!val)
+		return -EINVAL;
+
+	errno = 0;
+	d = strtod(str, NULL);
+
+	if (errno) {
+		error("unable to convert string to double: %s (%d)",
+				strerror(errno), errno);
+		return -errno;
+	}
+
+	if (0 > d)
+		d *= -1;
+	*val = d * 100UL;
+
+	return 0;
+}
+
+/**
+ * set the forward_delay of a bridge;
+ *
+ * @param id 		id of the bridge to manipulate
+ * @param delay		value of the forward delay to use (jiffies!)
+ * @return		0 on success,
+ * 			-EINVAL if id was invalid
+ * 			-errno if fprintf or ioctl failed
+ */
+static int __set_br_fwd_delay(const int id, const unsigned long int delay)
+{
+	char path[SYSFS_PATH_MAX];
+	FILE *fp; 
+	int ret, *ptr = NULL;
+	const char *name, *sysfs_mnt;
+	static int done[] = { 0, 0 };
+
+	name = bridge_get_name(id);
+	/* invalid id */
+	if (!name)
+		return -EINVAL;
+
+	if (BNEP_SVC_NAP == id)
+		ptr = &done[0];
+	else if (BNEP_SVC_NAP == id)
+		ptr = &done[1];
+
+	/* invalid id again */
+	if (NULL == ptr)
+		return -EINVAL;
+
+	/* operation already done... */
+	if (1 == *ptr)
+		return 0;
+
+	/* there's two ways to set the forward_delay, the new one,
+	 * - use the sysfs file for the bridge, or
+	 * - the 'old-fashioned' one, employing an obsolete ioctl */
+	sysfs_mnt = getenv(SYSFS_PATH_ENV);
+	if (!sysfs_mnt)
+		sysfs_mnt = SYSFS_MNT_PATH;
+	snprintf(path, SYSFS_PATH_MAX, "%s/" SYSFS_CLASS_NAME
+			"/net/%s/bridge/forward_delay", sysfs_mnt, name);
+	fp = fopen(path, "w");
+
+	if (fp) {
+		ret = fprintf(fp, "%ld", delay);
+		fclose(fp);
+	} else {
+		struct ifreq ifr;
+		unsigned long data[4] = { BRCTL_SET_BRIDGE_FORWARD_DELAY,
+			delay, 0, 0 };
+
+		strncpy(ifr.ifr_name, name, IFNAMSIZ);
+		ifr.ifr_data = (char *)&data;
+		ret = ioctl(bridge_socket, SIOCDEVPRIVATE, &ifr);
+	}
+
+	if (0 >= ret)
+		/* failed to set forward_delay */
+		ret = -errno;
+	else {
+		/* success, remember we set forward_delay */
+		*ptr = 1;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+int bridge_init(const char *gn_iface, const char *nap_iface,
+	const char *fwd_delay)
 {
 #if 0
 	struct stat st;
@@ -64,6 +172,9 @@ int bridge_init(const char *gn_iface, const char *nap_iface)
 	gn_bridge = gn_iface;
 	nap_bridge = nap_iface;
 
+	/* ignore return value, default is 1499 anyway... */
+	__str_to_jiffies(fwd_delay, &forward_delay);
+
 	return 0;
 }
 
@@ -132,6 +243,10 @@ int bridge_add_interface(int id, const char *dev)
 	err = bnep_if_up(name, id);
 	if (err < 0)
 		return err;
+	
+	err = __set_br_fwd_delay(id, forward_delay);
+	if (err < 0)
+		return err;
 
 	return 0;
 }
diff --git a/network/bridge.h b/network/bridge.h
index f241148..4c18baf 100644
--- a/network/bridge.h
+++ b/network/bridge.h
@@ -21,7 +21,8 @@
  *
  */
 
-int bridge_init(const char *gn_iface, const char *nap_iface);
+int bridge_init(const char *gn_iface, const char *nap_iface,
+	const char *forward_delay);
 void bridge_cleanup(void);
 
 int bridge_create(int id);
diff --git a/network/manager.c b/network/manager.c
index bc84de0..47eb641 100644
--- a/network/manager.c
+++ b/network/manager.c
@@ -46,6 +46,7 @@
 #define IFACE_PREFIX "bnep%d"
 #define GN_IFACE  "pan0"
 #define NAP_IFACE "pan1"
+#define FORWARD_DELAY "0"
 
 static struct btd_adapter_driver network_panu_server_driver;
 static struct btd_adapter_driver network_gn_server_driver;
@@ -58,6 +59,7 @@ static struct network_conf {
 	gboolean server_enabled;
 	gboolean security;
 	char *iface_prefix;
+	char *forward_delay;
 	char *panu_script;
 	char *gn_script;
 	char *nap_script;
@@ -68,6 +70,7 @@ static struct network_conf {
 	.server_enabled = TRUE,
 	.security = TRUE,
 	.iface_prefix = NULL,
+	.forward_delay = NULL,
 	.panu_script = NULL,
 	.gn_script = NULL,
 	.nap_script = NULL,
@@ -78,6 +81,7 @@ static struct network_conf {
 static void conf_cleanup(void)
 {
 	g_free(conf.iface_prefix);
+	g_free(conf.forward_delay);
 	g_free(conf.panu_script);
 	g_free(conf.gn_script);
 	g_free(conf.nap_script);
@@ -122,6 +126,13 @@ static void read_config(const char *file)
 		g_clear_error(&err);
 	}
 
+	conf.forward_delay = g_key_file_get_string(keyfile, "General",
+						"ForwardDelay", &err);
+	if (err) {
+		debug("%s: %s", file, err->message);
+		g_clear_error(&err);
+	}
+
 #if 0
 	conf.panu_script = g_key_file_get_string(keyfile, "PANU Role",
 						"Script", &err);
@@ -176,13 +187,15 @@ done:
 		conf.gn_iface = g_strdup(GN_IFACE);
 	if (!conf.nap_iface)
 		conf.nap_iface = g_strdup(NAP_IFACE);
+	if (!conf.forward_delay)
+		conf.forward_delay = g_strdup(FORWARD_DELAY);
 
 	debug("Config options: InterfacePrefix=%s, PANU_Script=%s, "
 		"GN_Script=%s, NAP_Script=%s, GN_Interface=%s, "
-		"NAP_Interface=%s, Security=%s",
+		"NAP_Interface=%s, Security=%s, ForwardDelay=%s",
 		conf.iface_prefix, conf.panu_script, conf.gn_script,
 		conf.nap_script, conf.gn_iface, conf.nap_iface,
-		conf.security ? "true" : "false");
+		conf.security ? "true" : "false", conf.forward_delay);
 }
 
 static int network_probe(struct btd_device *device, GSList *uuids, uint16_t id)
@@ -343,7 +356,7 @@ int network_manager_init(DBusConnection *conn)
 	 * (setup connection request) contains the destination service
 	 * field that defines which service the source is connecting to.
 	 */
-	if (bridge_init(conf.gn_iface, conf.nap_iface) < 0) {
+	if (bridge_init(conf.gn_iface, conf.nap_iface, conf.forward_delay) < 0) {
 		error("Can't init bridge module");
 		return -1;
 	}

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Marcel Holtmann]

Hi Thomas,

> the default learning time of a bridge -- 15 seconds -- appears
> unacceptably long, especially for clients that disconnect quite
> often after a short time (e.g. web-browsers);
> this patch tries to remedy this by setting the the waiting period to a
> value of zero (by default) or -- if the administrator wishes a larger
> value -- to whatever the General section of the network.conf holds in
> the variable ForwardDelay;
> ---
>  Makefile.am       |    2 +-
>  acinclude.m4      |    6 +++
>  configure.ac      |    1 +
>  network/bridge.c  |  117
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  network/bridge.h  |    3 +-
>  network/manager.c |   19 +++++++-
>  6 files changed, 142 insertions(+), 6 deletions(-)

using libsysfs is not acceptable. That library is deprecated.

Regards

Marcel

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Marcel Holtmann wrote:
> Hi Thomas,
> [...]
> using libsysfs is not acceptable. That library is deprecated.
Hi Marcel,

I get your point, but what's your suggestion? The ioctl is marked
deprecated, too and the problem of setting the forward delay persists.
Would it suffer your needs if I got rid of libsysfs in general and
simply use the ioctl or is there a different replacement strategy you
prefer?

Cheers,

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEAREKAAYFAktcYGIACgkQ2/ggQBUI/sn/fQCgg0rXvEVLB0W61iufCQZkKR5C
tqkAmwXu4nIi1pa226b3Gdz2YCBJL3UK
=ldEx
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Marcel Holtmann]

Hi Thomas,

> I get your point, but what's your suggestion? The ioctl is marked
> deprecated, too and the problem of setting the forward delay persists.
> Would it suffer your needs if I got rid of libsysfs in general and
> simply use the ioctl or is there a different replacement strategy you
> prefer?

if it has an ioctl, then use that one. I really don't understand why
everybody things ioctl are bad. They work just fine and are often
simpler to handle than sysfs files.

Regards

Marcel

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Luiz Augusto von Dentz]

Hi,

On Sun, Jan 24, 2010 at 5:13 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Thomas,
>
>> I get your point, but what's your suggestion? The ioctl is marked
>> deprecated, too and the problem of setting the forward delay persists.
>> Would it suffer your needs if I got rid of libsysfs in general and
>> simply use the ioctl or is there a different replacement strategy you
>> prefer?
>
> if it has an ioctl, then use that one. I really don't understand why
> everybody things ioctl are bad. They work just fine and are often
> simpler to handle than sysfs files.

The documentation on
http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge
state:

"If the bridge is being used standalone (no other bridges near by).
Then it is safe to turn the forwarding delay off (set it to zero),
before adding interface to a bridge. Then you can run DHCP client
right away." Which I guess is what the PAN spec expects for a NAP
server, so perhaps having another entry in network.conf is not really
needed or only really useful for a GN server if someone is messing
with it e.g. mesh network, but even for that purpose it just seems
wrong to expect that in a fixed amount of time it will be able to
resolve the topology, so why bother?


--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Sun, Jan 24, 2010 at 5:13 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Thomas,
>>
>>> I get your point, but what's your suggestion? The ioctl is marked
>>> deprecated, too and the problem of setting the forward delay persists.
>>> Would it suffer your needs if I got rid of libsysfs in general and
>>> simply use the ioctl or is there a different replacement strategy you
>>> prefer?
>> if it has an ioctl, then use that one. I really don't understand why
>> everybody things ioctl are bad. They work just fine and are often
>> simpler to handle than sysfs files.
> 
> The documentation on
> http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge
> state:
> 
> "If the bridge is being used standalone (no other bridges near by).
> Then it is safe to turn the forwarding delay off (set it to zero),
> before adding interface to a bridge. Then you can run DHCP client
> right away." Which I guess is what the PAN spec expects for a NAP
> server, so perhaps having another entry in network.conf is not really
> needed or only really useful for a GN server if someone is messing
> with it e.g. mesh network, but even for that purpose it just seems
> wrong to expect that in a fixed amount of time it will be able to
> resolve the topology, so why bother?
Luiz,
let me tell you why I'd like to have an extra config variable. Let's
imagine an admin who want to add the upcoming bnep interfaces to an
_already_ existing bridge (for that matter a test whether the given
interface name already exists would be nice, too) plus he knows about
the infrastructure behind the bridge and decides to set a different
forward delay. He could, of course reset the delay, once the bridge is
up. But ideally he would specify the setting in the network.conf. Hence
the extra config variable (which btw. were actually two variables by the
time I started writing the patch).

Cheers,

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEAREKAAYFAkteqUoACgkQ2/ggQBUI/sm6FQCcD8btgzXLig9UD59AE8u753bP
iFAAn1+ZiWlJ1ysVLgIq7jvpxL8u0dPm
=AtT9
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Luiz Augusto von Dentz]

Hi Thomas,

On Tue, Jan 26, 2010 at 10:35 AM, Thomas Egerer <hakke_007@gmx.de> wrote:
> Luiz,
> let me tell you why I'd like to have an extra config variable. Let's
> imagine an admin who want to add the upcoming bnep interfaces to an
> _already_ existing bridge (for that matter a test whether the given
> interface name already exists would be nice, too) plus he knows about
> the infrastructure behind the bridge and decides to set a different
> forward delay. He could, of course reset the delay, once the bridge is
> up. But ideally he would specify the setting in the network.conf. Hence
> the extra config variable (which btw. were actually two variables by the
> time I started writing the patch).

In lets say a pure network sense you are probably right, but what Im
arging about is that the PAN spec doesn't mentioned anything about
this, so in the spec sense once you accept the connection the
interface _is_ already fully functional. So either we always set the
delay to 0 or we have to wait until the interface is ready to accept
the connection, although the second alternative is doable it happens
to be after the connection is accepted that the network interface is
created and there is no way to inform the other part that there is an
arbitrary delay before the interface become functional again.


--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Luiz Augusto von Dentz wrote:
> Hi Thomas,
> 
> On Tue, Jan 26, 2010 at 10:35 AM, Thomas Egerer <hakke_007@gmx.de> wrote:
>> Luiz,
>> let me tell you why I'd like to have an extra config variable. Let's
>> imagine an admin who want to add the upcoming bnep interfaces to an
>> _already_ existing bridge (for that matter a test whether the given
>> interface name already exists would be nice, too) plus he knows about
>> the infrastructure behind the bridge and decides to set a different
>> forward delay. He could, of course reset the delay, once the bridge is
>> up. But ideally he would specify the setting in the network.conf. Hence
>> the extra config variable (which btw. were actually two variables by the
>> time I started writing the patch).
> 
> In lets say a pure network sense you are probably right, but what Im
> arging about is that the PAN spec doesn't mentioned anything about
> this, so in the spec sense once you accept the connection the
> interface _is_ already fully functional. So either we always set the
> delay to 0 or we have to wait until the interface is ready to accept
> the connection, although the second alternative is doable it happens
> to be after the connection is accepted that the network interface is
> created and there is no way to inform the other part that there is an
> arbitrary delay before the interface become functional again.
> 
> 

Luiz,

I disagree. On page 31 the specification states that a NAP/GN performs
the following (in regard to packet forwarding operations):
'Automatically learn and maintain the information required to make
frame-filtering decisions as described in [3] section 7.1 and specified
in [3] sections 7.8 and 7.9, for the support of Basic Filtering Services.'
and even if it says in the next paragraph:
'The NAP/GN is not required to perform any of the following aspects of
the 802.1D standard.'
footnote 4 mentions:
'Sophisticated Bluetooth NAP devices may choose to implement some or all
of the 802.1D features.'
In my eyes this supports my point of view since the forward delay as
part of the learning process should be a subject of configuration.
Could I convince you?

Cheers,

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAktfKeIACgkQ2/ggQBUI/skLrQCcCt0QYw2UvqP29QNtJUCNIHOE
VSYAoJ3dlsJl/Q7Q+NbgIWA7b5qxJcOo
=0yd4
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Luiz Augusto von Dentz]

Hi Thomas,

On Tue, Jan 26, 2010 at 7:44 PM, Thomas Egerer <thomas.washeim@gmx.net> wro=
te:
>
> Luiz,
>
> I disagree. On page 31 the specification states that a NAP/GN performs
> the following (in regard to packet forwarding operations):
> 'Automatically learn and maintain the information required to make
> frame-filtering decisions as described in [3] section 7.1 and specified
> in [3] sections 7.8 and 7.9, for the support of Basic Filtering Services.=
'
> and even if it says in the next paragraph:
> 'The NAP/GN is not required to perform any of the following aspects of
> the 802.1D standard.'
> footnote 4 mentions:
> 'Sophisticated Bluetooth NAP devices may choose to implement some or all
> of the 802.1D features.'
> In my eyes this supports my point of view since the forward delay as
> part of the learning process should be a subject of configuration.
> Could I convince you?

You almost convince me, but after some googling it seems
http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge#Spa=
nning_Tree_Protocol
contradicts what you are saying:

"The Spanning Tree Protocol has no authentication; all participants
are assumed to be trustworthy and correct. This assumption is not true
if bridging between a hostile environment like the Internet and a
private network. For this reason, STP is turned off by default on the
recent versions of Linux."

And even the spec itself seem to support my argument:

"2004 =97 Revised version (802.1D-2004), incorporating the extensions
802.11c, 802.1t and 802.1w, which were separately published in 2001,
and removing the original Spanning tree protocol, instead
incorporating the Rapid Spanning Tree Protocol (RSTP) from 802.1w."

Naturally someone else also notice that this arbitrary delay during
the learning phase is a bad idea after all and invented the RSTP and I
guess PAN spec was released after 2004 so that would explain why we
don't see anything about how to handle forwarding delay.


--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Luiz,

Luiz Augusto von Dentz wrote:
> Hi Thomas,
> 
> On Tue, Jan 26, 2010 at 7:44 PM, Thomas Egerer <thomas.washeim@gmx.net> wrote:
>> Luiz,
>>
>> I disagree. On page 31 the specification states that a NAP/GN performs
>> the following (in regard to packet forwarding operations):
>> 'Automatically learn and maintain the information required to make
>> frame-filtering decisions as described in [3] section 7.1 and specified
>> in [3] sections 7.8 and 7.9, for the support of Basic Filtering Services.'
>> and even if it says in the next paragraph:
>> 'The NAP/GN is not required to perform any of the following aspects of
>> the 802.1D standard.'
>> footnote 4 mentions:
>> 'Sophisticated Bluetooth NAP devices may choose to implement some or all
>> of the 802.1D features.'
>> In my eyes this supports my point of view since the forward delay as
>> part of the learning process should be a subject of configuration.
>> Could I convince you?
> 
> You almost convince me, but after some googling it seems
:D
> http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge#Spanning_Tree_Protocol
> contradicts what you are saying:
> 
> "The Spanning Tree Protocol has no authentication; all participants
> are assumed to be trustworthy and correct. This assumption is not true
> if bridging between a hostile environment like the Internet and a
> private network. For this reason, STP is turned off by default on the
> recent versions of Linux."
> 
> And even the spec itself seem to support my argument:
> 
> "2004 — Revised version (802.1D-2004), incorporating the extensions
> 802.11c, 802.1t and 802.1w, which were separately published in 2001,
> and removing the original Spanning tree protocol, instead
> incorporating the Rapid Spanning Tree Protocol (RSTP) from 802.1w."
> 
> Naturally someone else also notice that this arbitrary delay during
> the learning phase is a bad idea after all and invented the RSTP and I
> guess PAN spec was released after 2004 so that would explain why we
> don't see anything about how to handle forwarding delay.
That's at least an explanation why one cannot find how to configure the
forward delay for the bridge.
This is where the use case comes into play. You allow the user to
configure the bridge to use (which by the way is only created for GN and
not for NAP, is that correct?). If you do so one might (for various
reasons) want to choose an existing bridge (which results in a simple
error message without giving the actual reason). But if the user picks a
bridge that already exists and you use a fix, not configurable delay one
has to reset the delay -- if necessary -- to something -ne 0 after
bluetoothd was started.
If one doesn't specify the value it's set to the default, zero seconds,
no harm done, if one does specify the value this one is used. I can't
see where the problem is.

Regards,

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEAREKAAYFAktgkd0ACgkQ2/ggQBUI/skpKgCeOa18EGr9GviCRf/PUU70p1YM
gKYAnjnAeujBsKiEQRBeqeYIDZ7HRutG
=3VWl
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Thomas Egerer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Luiz

Luiz Augusto von Dentz wrote:
> Hi.
>
> On Wed, Jan 27, 2010 at 9:19 PM, Thomas Egerer <hakke_007@gmx.de> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA512
>>
>> Luiz,
>>
>> Luiz Augusto von Dentz wrote:
>>> Hi Thomas,
>>>
>>> On Tue, Jan 26, 2010 at 7:44 PM, Thomas Egerer
<thomas.washeim@gmx.net> wrote:
>>>> Luiz,
>>>>
>>>> I disagree. On page 31 the specification states that a NAP/GN performs
>>>> the following (in regard to packet forwarding operations):
>>>> 'Automatically learn and maintain the information required to make
>>>> frame-filtering decisions as described in [3] section 7.1 and specified
>>>> in [3] sections 7.8 and 7.9, for the support of Basic Filtering
Services.'
>>>> and even if it says in the next paragraph:
>>>> 'The NAP/GN is not required to perform any of the following aspects of
>>>> the 802.1D standard.'
>>>> footnote 4 mentions:
>>>> 'Sophisticated Bluetooth NAP devices may choose to implement some
or all
>>>> of the 802.1D features.'
>>>> In my eyes this supports my point of view since the forward delay as
>>>> part of the learning process should be a subject of configuration.
>>>> Could I convince you?
>>> You almost convince me, but after some googling it seems
>> :D
>>>
http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge#Spanning_Tree_Protocol
>>> contradicts what you are saying:
>>>
>>> "The Spanning Tree Protocol has no authentication; all participants
>>> are assumed to be trustworthy and correct. This assumption is not true
>>> if bridging between a hostile environment like the Internet and a
>>> private network. For this reason, STP is turned off by default on the
>>> recent versions of Linux."
>>>
>>> And even the spec itself seem to support my argument:
>>>
>>> "2004 — Revised version (802.1D-2004), incorporating the extensions
>>> 802.11c, 802.1t and 802.1w, which were separately published in 2001,
>>> and removing the original Spanning tree protocol, instead
>>> incorporating the Rapid Spanning Tree Protocol (RSTP) from 802.1w."
>>>
>>> Naturally someone else also notice that this arbitrary delay during
>>> the learning phase is a bad idea after all and invented the RSTP and I
>>> guess PAN spec was released after 2004 so that would explain why we
>>> don't see anything about how to handle forwarding delay.
>> That's at least an explanation why one cannot find how to configure the
>> forward delay for the bridge.
>> This is where the use case comes into play. You allow the user to
>> configure the bridge to use (which by the way is only created for GN and
>> not for NAP, is that correct?). If you do so one might (for various
>> reasons) want to choose an existing bridge (which results in a simple
>> error message without giving the actual reason). But if the user picks a
>> bridge that already exists and you use a fix, not configurable delay one
>> has to reset the delay -- if necessary -- to something -ne 0 after
>> bluetoothd was started.
>> If one doesn't specify the value it's set to the default, zero seconds,
>> no harm done, if one does specify the value this one is used. I can't
>> see where the problem is.
First of all, I'm feeling a bit like an idiot here. The only thing I
tried to do was improving bluez. And I'd give a rats ass about it if I
didn't use it every day. So I have personal interest to make the
software as good and usable as possible for me as well as other people.
>
> You probably didn't read this documentation:
>
>
http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge_stp
I indeed read that documentation.

> "STP is turned off by default on the recent versions of Linux"
And I sure know that. I'm using bridges quite often and 'brctl show'
lists a field called 'STP enabled' which is set to off unless you change
it manually.

> afaik it will be pretty hard to convince Marcel about adding something
> that is not enabled by default anymore
This must be a misunderstanding. I'm don't want to enable STP by default
or whatever. My intention is (see me 'foreword' from above) to make the
bluetoothd as user-friendly as even possible. And this includes for me
to check for an existing bridge (this means honoring the EEXIST return
code in bridge_create) first off. Then secondly leaving the bridge's
settings untouched or setting it to the user-defined values from
network.conf.

> And again STP is _obsolete_ according to 802.1D-2004 (we are in 2010),
> I don't know why you are spending time trying to use STP in the first
> place, If I were you I would spend my time implementing RSTP or MSTP
> that would be great not only for bluetooth but also for networking in
> general.
Again, I don't want to enable STP by default, I want people to be able
to use bluetooth in a fashion they want (which includes honoring that an
existing bridge keeps using its existing settings/set user defined
values for a newly created bridge).
I know RSTP would be great, but it's not in the kernel. Support would be
available via user-space lib, but all the project to find in this
direction seem to be dead.
So as a compromise, do you think it would be possible to 'convince'
Marcel to leave a bridge's settings untouched as long as it exists and
use the (reasonable) default values like a forward delay of zero seconds
for newly created bridges?

Cheers,

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEAREKAAYFAkth360ACgkQ2/ggQBUI/snpkwCfSmSurtaE2mQv8QwMjugC4EAi
XYUAoI342Qt4rEyqVNDV3NudSDCgujCP
=hyEd
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] add configurable support for bridge forward delay

[Luiz Augusto von Dentz]

Hi,

On Thu, Jan 28, 2010 at 9:04 PM, Thomas Egerer <hakke_007@gmx.de> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> Luiz
>
> Luiz Augusto von Dentz wrote:
>> Hi.
>>
>> On Wed, Jan 27, 2010 at 9:19 PM, Thomas Egerer <hakke_007@gmx.de> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA512
>>>
>>> Luiz,
>>>
>>> Luiz Augusto von Dentz wrote:
>>>> Hi Thomas,
>>>>
>>>> On Tue, Jan 26, 2010 at 7:44 PM, Thomas Egerer
> <thomas.washeim@gmx.net> wrote:
>>>>> Luiz,
>>>>>
>>>>> I disagree. On page 31 the specification states that a NAP/GN performs
>>>>> the following (in regard to packet forwarding operations):
>>>>> 'Automatically learn and maintain the information required to make
>>>>> frame-filtering decisions as described in [3] section 7.1 and specified
>>>>> in [3] sections 7.8 and 7.9, for the support of Basic Filtering
> Services.'
>>>>> and even if it says in the next paragraph:
>>>>> 'The NAP/GN is not required to perform any of the following aspects of
>>>>> the 802.1D standard.'
>>>>> footnote 4 mentions:
>>>>> 'Sophisticated Bluetooth NAP devices may choose to implement some
> or all
>>>>> of the 802.1D features.'
>>>>> In my eyes this supports my point of view since the forward delay as
>>>>> part of the learning process should be a subject of configuration.
>>>>> Could I convince you?
>>>> You almost convince me, but after some googling it seems
>>> :D
>>>>
> http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge#Spanning_Tree_Protocol
>>>> contradicts what you are saying:
>>>>
>>>> "The Spanning Tree Protocol has no authentication; all participants
>>>> are assumed to be trustworthy and correct. This assumption is not true
>>>> if bridging between a hostile environment like the Internet and a
>>>> private network. For this reason, STP is turned off by default on the
>>>> recent versions of Linux."
>>>>
>>>> And even the spec itself seem to support my argument:
>>>>
>>>> "2004 — Revised version (802.1D-2004), incorporating the extensions
>>>> 802.11c, 802.1t and 802.1w, which were separately published in 2001,
>>>> and removing the original Spanning tree protocol, instead
>>>> incorporating the Rapid Spanning Tree Protocol (RSTP) from 802.1w."
>>>>
>>>> Naturally someone else also notice that this arbitrary delay during
>>>> the learning phase is a bad idea after all and invented the RSTP and I
>>>> guess PAN spec was released after 2004 so that would explain why we
>>>> don't see anything about how to handle forwarding delay.
>>> That's at least an explanation why one cannot find how to configure the
>>> forward delay for the bridge.
>>> This is where the use case comes into play. You allow the user to
>>> configure the bridge to use (which by the way is only created for GN and
>>> not for NAP, is that correct?). If you do so one might (for various
>>> reasons) want to choose an existing bridge (which results in a simple
>>> error message without giving the actual reason). But if the user picks a
>>> bridge that already exists and you use a fix, not configurable delay one
>>> has to reset the delay -- if necessary -- to something -ne 0 after
>>> bluetoothd was started.
>>> If one doesn't specify the value it's set to the default, zero seconds,
>>> no harm done, if one does specify the value this one is used. I can't
>>> see where the problem is.
> First of all, I'm feeling a bit like an idiot here. The only thing I
> tried to do was improving bluez. And I'd give a rats ass about it if I
> didn't use it every day. So I have personal interest to make the
> software as good and usable as possible for me as well as other people.

Don't fell bad about this, discussions as we are having now are quite
normal in open source, actually I would say it is part of the
development process. So again, don't feel bad.

>>
>> You probably didn't read this documentation:
>>
>>
> http://www.linuxfoundation.org/collaborate/workgroups/networking/bridge_stp
> I indeed read that documentation.
>
>> "STP is turned off by default on the recent versions of Linux"
> And I sure know that. I'm using bridges quite often and 'brctl show'
> lists a field called 'STP enabled' which is set to off unless you change
> it manually.
>
>> afaik it will be pretty hard to convince Marcel about adding something
>> that is not enabled by default anymore
> This must be a misunderstanding. I'm don't want to enable STP by default
> or whatever. My intention is (see me 'foreword' from above) to make the
> bluetoothd as user-friendly as even possible. And this includes for me
> to check for an existing bridge (this means honoring the EEXIST return
> code in bridge_create) first off. Then secondly leaving the bridge's
> settings untouched or setting it to the user-defined values from
> network.conf.

My argument is basically against Delay Forward as part of
network.conf, period. Honoring bridge_create return and leave the
bridge setting untouched are good ideas, as for the user defined
values that is not BlueZ business, actually I would expect that coming
for network specific daemons such as NetworkManager and connman
options for tethering.

>> And again STP is _obsolete_ according to 802.1D-2004 (we are in 2010),
>> I don't know why you are spending time trying to use STP in the first
>> place, If I were you I would spend my time implementing RSTP or MSTP
>> that would be great not only for bluetooth but also for networking in
>> general.
> Again, I don't want to enable STP by default, I want people to be able
> to use bluetooth in a fashion they want (which includes honoring that an
> existing bridge keeps using its existing settings/set user defined
> values for a newly created bridge).
> I know RSTP would be great, but it's not in the kernel. Support would be
> available via user-space lib, but all the project to find in this
> direction seem to be dead.
> So as a compromise, do you think it would be possible to 'convince'
> Marcel to leave a bridge's settings untouched as long as it exists and
> use the (reasonable) default values like a forward delay of zero seconds
> for newly created bridges?

After this discussion I should say we should not mess with forward
delay at all, if BlueZ is to create a bridge itself than it should not
bother enabling STP, but if a bridge configured is already using it
than we simple don't touch it.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação