Dear Bluez developers,
recently I have experienced a problem with one of my USB bluetooth
dongles: Whenever I plugged it into an USB port, the kernel detected
the device (i.e. reasonable output in dmesg) and the correct kernel
module (i.e. btusb) was loaded, but bluez frontends like
gnome-bluetooth could still not activate the device. The bug that I
reported against this software can be found at:
<https://bugzilla.gnome.org/show_bug.cgi?id=617050>.
It turned out that my USB dongle was the culprit, since it had an
invalid default device name that contained non-UTF-8 characters, as
reported by 'hciconfig -a'. When I changed the device name to
something reasonable via e.g. 'hciconfig hci0 name foobar', I had to
*restart* the bluetoothd daemon afterwards in order to get everything
working as expected. However, when I unplugged the USB dongle and
plugged it back in, the device name was reset to the defective default
again and I had to repeat this procedure.
Since I found it unreasonable to be forced to manually change the
device name and restart the daemon each and every time I plugged my
USB dongle in, I wanted a solution that detected a faulty device name
string during the device initialization phase and instantly fixed it
by writing a converted string back to the device. The attached patch
does exact this and makes my USB dongle work out-of-the-box, despite
its defective default device name.
Please review the patch for integration into Bluez.
Cheers,
Fabian
PS: There is still one open question that I would like to address:
Since I am going to print both the faulty and the fixed device name
strings to stderr via debug(), should I make sure the strings do not
contain any ASCII control characters prior to that? At least this is
what is done in tools/hciconfig.c:442.
Am 06.05.2010 17:03, schrieb Johan Hedberg:
> BlueZ will (or at least should) set the name for the adapter as follows:
>
> 1. If there's a name in /var/lib/bluetooth/... use that
> 2. Else if there's a name in main.conf use that
> 3. If all else fails set the name to "BlueZ"
Hm, IIRC it was set to $(hostname)-$(hci-id) last time I established a
connection.
> So I fail to see why this patch is needed at all. It sounds like there's
> something else wrong in the initialization process which makes the
> initialzation fail if the adapter contains some invalid default name (we
> shouldn't as far as I see be trying to read the name at all from the
> adapter before we've written it ourselves from the host side). I.e. I
> suspect the patch might be just working around the real issue instead of
> fixing it.
Yes, obviously Bluez fails to set a reasonable device name if the
initial device name isn't already valid. However, I believe Bluez
shouldn't necessarily be expected to handle invalid device names, as
they clearly violate the specs. Instead, if Bluez detects an invalid
name, it should convert it into a valid name as soon in the
initialization process as possible. This sounds just naturally to me
and is exactly what my patch does.
Cheers,
Fabian
Hi,
On Thu, May 06, 2010, Fabian Greffrath wrote:
> Am Mittwoch, den 05.05.2010, 17:02 +0200 schrieb Stefan Seyfried:
> > Then it would probably good if you could send the patch against current
> > git (even if it still applies cleanly) and in a format that "git am" can
> > process directly. That makes it very easy for the maintainers to apply the
> > code and in the same run makes sure you get proper attribution for your
> > contribution ;)
>
> I have reapplied my patch against current git, I have replaced the
> obscure 249 in "char name[249];" by MAX_NAME_LENGTH as defined in
> src/adapter.h and I am now posting it inline. Happy reviewing! ;)
BlueZ will (or at least should) set the name for the adapter as follows:
1. If there's a name in /var/lib/bluetooth/... use that
2. Else if there's a name in main.conf use that
3. If all else fails set the name to "BlueZ"
So I fail to see why this patch is needed at all. It sounds like there's
something else wrong in the initialization process which makes the
initialzation fail if the adapter contains some invalid default name (we
shouldn't as far as I see be trying to read the name at all from the
adapter before we've written it ourselves from the host side). I.e. I
suspect the patch might be just working around the real issue instead of
fixing it.
Johan
Hi bluez,
Am Mittwoch, den 05.05.2010, 17:02 +0200 schrieb Stefan Seyfried:
> Then it would probably good if you could send the patch against current
> git (even if it still applies cleanly) and in a format that "git am" can
> process directly. That makes it very easy for the maintainers to apply the
> code and in the same run makes sure you get proper attribution for your
> contribution ;)
I have reapplied my patch against current git, I have replaced the
obscure 249 in "char name[249];" by MAX_NAME_LENGTH as defined in
src/adapter.h and I am now posting it inline. Happy reviewing! ;)
>From 5447e55aeeda8a2307de0b7765abd8883c5f16a9 Mon Sep 17 00:00:00 2001
From: Fabian Greffrath <[email protected]>
Date: Thu, 6 May 2010 16:37:43 +0200
Subject: [PATCH] Instantly fix non-UTF-8 local device names during device configuration phase
---
plugins/hciops.c | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 05c3d4e..836c291 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -81,6 +81,7 @@ static void configure_device(int index)
struct hci_dev_info di;
uint16_t policy;
int dd, err;
+ char name[MAX_NAME_LENGTH + 1];
if (hci_devinfo(index, &di) < 0)
return;
@@ -96,6 +97,30 @@ static void configure_device(int index)
return;
}
+ if (hci_read_local_name(dd, sizeof(name), name, 1000) < 0) {
+ error("Can't read local name on hci%d: %s (%d)\n",
+ index, strerror(errno), errno);
+ return;
+ }
+
+ name[MAX_NAME_LENGTH] = '\0';
+
+ if (!g_utf8_validate(name, -1, NULL)) {
+ char *utf8_name;
+
+ utf8_name = g_convert(name, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL);
+ if (utf8_name) {
+ debug("Fix invalid non-UTF-8 device name \"%s\" on hci%d to \"%s\"",
+ name, index, utf8_name);
+ if (hci_write_local_name(dd, utf8_name, 2000) < 0) {
+ error("Can't change local name on hci%d: %s (%d)\n",
+ index, strerror(errno), errno);
+ return;
+ }
+ g_free(utf8_name);
+ }
+ }
+
/* Set page timeout */
if ((main_opts.flags & (1 << HCID_SET_PAGETO))) {
write_page_timeout_cp cp;
--
1.7.1
Am 05.05.2010 17:02, schrieb Stefan Seyfried:
> I have one questions about the code from a cursory look (hint:
> sending the patch inline would help commenting on it)
Alright, I'll know for the next time.
> * where does the 249 in "char name[249];" come from? Is it from the BT
> spec? Or from somewhere else? A comment in the code might help. (If this
> is a number from the spec that is used all over the same code file and
> explained elsewhere, this question is obviously moot)
I took this part of the patch over from tools/hciconfig.c:433 and
indeed I think the fact that a device name may be up to 248 characters
long is part of the BT spec:
<http://www.palowireless.com/infotooth/tutorial/k1_gap.asp#Bluetooth%20Parameter%20Representation>
> Then it would probably good if you could send the patch against current
> git (even if it still applies cleanly) and in a format that "git am" can
> process directly. That makes it very easy for the maintainers to apply the
> code and in the same run makes sure you get proper attribution for your
> contribution ;)
Thanks again. I'd like to get some more feedback for the current patch
and will then repost it in the desired format.
BTW, I'll be on vacation from May 7th to 14th, so please excuse if I
reply with some days delay.
On Wed, 05 May 2010 12:27:37 +0200
Fabian Greffrath <[email protected]> wrote:
> Dear Bluez list,
>
> I am sorry, I know this is the worst way to start getting in touch
> with a software project, but I accidently attached the wrong patch to
> my previous email. It's configure_device() that must be patched.
> Please find a corrected patch attached (it has been written against
> bluez 4.63 but applies perfectly against 4.64).
I have one questions about the code from a cursory look (hint:
sending the patch inline would help commenting on it)
* where does the 249 in "char name[249];" come from? Is it from the BT
spec? Or from somewhere else? A comment in the code might help. (If this
is a number from the spec that is used all over the same code file and
explained elsewhere, this question is obviously moot)
Then it would probably good if you could send the patch against current
git (even if it still applies cleanly) and in a format that "git am" can
process directly. That makes it very easy for the maintainers to apply the
code and in the same run makes sure you get proper attribution for your
contribution ;)
Have fun,
Stefan
--
Stefan Seyfried
"Any ideas, John?"
"Well, surrounding them's out."
Dear Bluez list,
I am sorry, I know this is the worst way to start getting in touch
with a software project, but I accidently attached the wrong patch to
my previous email. It's configure_device() that must be patched.
Please find a corrected patch attached (it has been written against
bluez 4.63 but applies perfectly against 4.64).
Cheers,
Fabian