2012-05-24 10:00:15

by Daniel Wagner

[permalink] [raw]
Subject: [PATCH v1 0/3] Prefer PAN over DUN

From: Daniel Wagner <[email protected]>

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



2012-05-30 09:33:01

by Daniel Wagner

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

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

2012-05-24 17:41:46

by Vinicius Costa Gomes

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

Hi Daniel,

On 12:00 Thu 24 May, Daniel Wagner wrote:
> From: Daniel Wagner <[email protected]>
>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Cheers,
--
Vinicius

2012-05-24 13:29:19

by Daniel Wagner

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

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

2012-05-24 10:21:20

by Johan Hedberg

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

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

2012-05-24 10:20:09

by Johan Hedberg

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

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

2012-05-24 10:00:18

by Daniel Wagner

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

From: Daniel Wagner <[email protected]>

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


2012-05-24 10:00:17

by Daniel Wagner

[permalink] [raw]
Subject: [PATCH v1 2/3] serial: Add DUN_GW_UUID

From: Daniel Wagner <[email protected]>

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


2012-05-24 10:00:16

by Daniel Wagner

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

From: Daniel Wagner <[email protected]>

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