[PATCH v1 0/3] Prefer PAN over DUN

[PATCH v1 0/3] Prefer PAN over DUN

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Hi,

as discussed on #bluez I have added a configuration switch for
this. 

cheers,
daniel

Daniel Wagner (3):
  main: Add IngoreDUN configuration switch
  serial: Add DUN_GW_UUID
  device: Ignore DUN if PAN is present

 serial/common.h |   24 ++++++++++++++++++++++++
 src/device.c    |   20 ++++++++++++++++++++
 src/hcid.h      |    1 +
 src/main.c      |    9 +++++++++
 src/main.conf   |    5 +++++
 5 files changed, 59 insertions(+)
 create mode 100644 serial/common.h

-- 
1.7.10.130.g36e6c

[PATCH v1 1/3] main: Add IngoreDUN configuration switch

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

---
 src/hcid.h    |    1 +
 src/main.c    |    9 +++++++++
 src/main.conf |    5 +++++
 3 files changed, 15 insertions(+)

diff --git a/src/hcid.h b/src/hcid.h
index 1e5e15a..2564467 100644
--- a/src/hcid.h
+++ b/src/hcid.h
@@ -39,6 +39,7 @@ struct main_opts {
 	gboolean	name_resolv;
 	gboolean	debug_keys;
 	gboolean	gatt_enabled;
+	gboolean	ignore_dun;
 
 	uint8_t		mode;
 
diff --git a/src/main.c b/src/main.c
index b062b4a..240326d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -245,6 +245,14 @@ static void parse_config(GKeyFile *config)
 	else
 		main_opts.gatt_enabled = boolean;
 
+	boolean = g_key_file_get_boolean(config, "General",
+						"IgnoreDUN", &err);
+	if (err) {
+		DBG("%s", err->message);
+		g_clear_error(&err);
+	} else
+		main_opts.ignore_dun = boolean;
+
 	main_opts.link_mode = HCI_LM_ACCEPT;
 
 	main_opts.link_policy = HCI_LP_RSWITCH | HCI_LP_SNIFF |
@@ -262,6 +270,7 @@ static void init_defaults(void)
 	main_opts.remember_powered = TRUE;
 	main_opts.reverse_sdp = TRUE;
 	main_opts.name_resolv = TRUE;
+	main_opts.ignore_dun = TRUE;
 
 	if (gethostname(main_opts.host_name, sizeof(main_opts.host_name) - 1) < 0)
 		strcpy(main_opts.host_name, "noname");
diff --git a/src/main.conf b/src/main.conf
index 787ef4f..676999d 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -62,3 +62,8 @@ DebugKeys = false
 
 # Enable the GATT functionality. Default is false
 EnableGatt = false
+
+# If a device supports both DUN and PAN at the same time, ignore the
+# DUN profile. Only PAN will be exposed through the D-Bus API in this
+# case. The default is true
+IgnoreDUN = true
-- 
1.7.10.130.g36e6c

Re: [PATCH v1 1/3] main: Add IngoreDUN configuration switch

[Johan Hedberg]

Hi Daniel,

On Thu, May 24, 2012, Daniel Wagner wrote:
> +	main_opts.ignore_dun = TRUE;

I'd prefer if this defaulted to the old behavior, i.e. FALSE.

Johan

Re: [PATCH v1 1/3] main: Add IngoreDUN configuration switch

[Vinicius Costa Gomes]

Hi Daniel,

On 12:00 Thu 24 May, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> ---
>  src/hcid.h    |    1 +
>  src/main.c    |    9 +++++++++
>  src/main.conf |    5 +++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/src/hcid.h b/src/hcid.h
> index 1e5e15a..2564467 100644
> --- a/src/hcid.h
> +++ b/src/hcid.h
> @@ -39,6 +39,7 @@ struct main_opts {
>  	gboolean	name_resolv;
>  	gboolean	debug_keys;
>  	gboolean	gatt_enabled;
> +	gboolean	ignore_dun;
>  
>  	uint8_t		mode;
>  
> diff --git a/src/main.c b/src/main.c
> index b062b4a..240326d 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -245,6 +245,14 @@ static void parse_config(GKeyFile *config)
>  	else
>  		main_opts.gatt_enabled = boolean;
>  
> +	boolean = g_key_file_get_boolean(config, "General",
> +						"IgnoreDUN", &err);
> +	if (err) {
> +		DBG("%s", err->message);
> +		g_clear_error(&err);
> +	} else
> +		main_opts.ignore_dun = boolean;
> +
>  	main_opts.link_mode = HCI_LM_ACCEPT;
>  
>  	main_opts.link_policy = HCI_LP_RSWITCH | HCI_LP_SNIFF |
> @@ -262,6 +270,7 @@ static void init_defaults(void)
>  	main_opts.remember_powered = TRUE;
>  	main_opts.reverse_sdp = TRUE;
>  	main_opts.name_resolv = TRUE;
> +	main_opts.ignore_dun = TRUE;
>  
>  	if (gethostname(main_opts.host_name, sizeof(main_opts.host_name) - 1) < 0)
>  		strcpy(main_opts.host_name, "noname");
> diff --git a/src/main.conf b/src/main.conf
> index 787ef4f..676999d 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -62,3 +62,8 @@ DebugKeys = false
>  
>  # Enable the GATT functionality. Default is false
>  EnableGatt = false
> +
> +# If a device supports both DUN and PAN at the same time, ignore the
> +# DUN profile. Only PAN will be exposed through the D-Bus API in this
> +# case. The default is true
> +IgnoreDUN = true

I would prefer if this would be called something like "PreferPAN" (or
even "PreferPANoverDUN", but that sounds weird). I, at least, find it
easier to understand.

And I am still thinking if it would be worth considering something more
complex, that would deal with cases like this in general. But as I don't
have any concrete proposal, I will stop here.

> -- 
> 1.7.10.130.g36e6c
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: [PATCH v1 1/3] main: Add IngoreDUN configuration switch

[Daniel Wagner]

Hi Vinicius,

Sorry for the late response. Was away :)

On 24.05.2012 19:41, Vinicius Costa Gomes wrote:
>> +
>> +# If a device supports both DUN and PAN at the same time, ignore the
>> +# DUN profile. Only PAN will be exposed through the D-Bus API in this
>> +# case. The default is true
>> +IgnoreDUN = true
> 
> I would prefer if this would be called something like "PreferPAN" (or
> even "PreferPANoverDUN", but that sounds weird). I, at least, find it
> easier to understand.

Funny, the first version the flag was named "PreferPANoverDUN".

All other variables are kept short, that is why I used IgnoreDUN. If
"PreferPANoverDUN" is acceptable, I rather named this way.

"PreferPAN" is not really correct in my opinion, because PAN will be
always there just DUN will be filtered out.

> And I am still thinking if it would be worth considering something more
> complex, that would deal with cases like this in general. But as I don't
> have any concrete proposal, I will stop here.

Yep, I was also thinking on something more general for policing. But
without real use cases I would rather not try to solve something we
don't have a problem yet.

thanks,
daniel

[PATCH v1 2/3] serial: Add DUN_GW_UUID

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

---
 serial/common.h |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 serial/common.h

diff --git a/serial/common.h b/serial/common.h
new file mode 100644
index 0000000..429bf65
--- /dev/null
+++ b/serial/common.h
@@ -0,0 +1,24 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012  BMW Car IT GmbH. All rights reserved.
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#define DUN_GW_UUID	"00001103-0000-1000-8000-00805f9b34fb"
-- 
1.7.10.130.g36e6c

[PATCH v1 3/3] device: Ignore DUN if PAN is present

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Filtering DUN out at BlueZ level reduces the number of CPU cycles and
memory spend just to figure out that PAN is always preferred over DUN.

Doing this could on a the network manager level (e.g. ConnMan) or
at an intermediate daemon such as dundee which handles AT command
parsing and PPP setup is possible just more expensive.

Thereforem just ignore the DUN completely and do not even announce it.
---
 src/device.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/device.c b/src/device.c
index 2695b16..7bc427b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -63,6 +63,8 @@
 #include "btio.h"
 #include "attrib-server.h"
 #include "attrib/client.h"
+#include "network/common.h"
+#include "serial/common.h"
 
 #define DISCONNECT_TIMER	2
 #define DISCOVERY_TIMER		2
@@ -1521,6 +1523,24 @@ static void update_services(struct browse_req *req, sdp_list_t *recs)
 
 		sdp_list_free(svcclass, free);
 	}
+
+	if (!main_opts.ignore_dun)
+		return;
+
+	if (g_slist_find_custom(req->profiles_added, NAP_UUID,
+					(GCompareFunc) strcmp) != NULL) {
+		GSList *l;
+
+		l = g_slist_find_custom(req->profiles_added,
+							DUN_GW_UUID,
+							(GCompareFunc) strcmp);
+		if (l != NULL) {
+			DBG("Skipping DUN entry because PAN was found.");
+			g_free(l->data);
+			req->profiles_added = g_slist_remove_link(
+							req->profiles_added, l);
+		}
+	}
 }
 
 static void store_profiles(struct btd_device *device)
-- 
1.7.10.130.g36e6c

Re: [PATCH v1 3/3] device: Ignore DUN if PAN is present

[Johan Hedberg]

Hi Daniel,

On Thu, May 24, 2012, Daniel Wagner wrote:
> --- a/src/device.c
> +++ b/src/device.c
> @@ -63,6 +63,8 @@
>  #include "btio.h"
>  #include "attrib-server.h"
>  #include "attrib/client.h"
> +#include "network/common.h"
> +#include "serial/common.h"

I don't really like the idea of creating dependencies from the core
daemon (src/*) to plugin or profile directories. Any other (clean) way
we could avoid this?

Johan

Re: [PATCH v1 3/3] device: Ignore DUN if PAN is present

[Daniel Wagner]

Hi Johan,

On 24.05.2012 12:21, Johan Hedberg wrote:
> Hi Daniel,
> 
> On Thu, May 24, 2012, Daniel Wagner wrote:
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -63,6 +63,8 @@
>>  #include "btio.h"
>>  #include "attrib-server.h"
>>  #include "attrib/client.h"
>> +#include "network/common.h"
>> +#include "serial/common.h"
> 
> I don't really like the idea of creating dependencies from the core
> daemon (src/*) to plugin or profile directories. Any other (clean) way
> we could avoid this?

Okay, good point.

I see that update_sevice() already uses PNP_UUID which happens to be
defined in src/storage.h. Maybe we could move all *_UUID into one header
file into the core? Would that be an option?

cheers,
daniel