[PATCH] Fix device_match_pattern function

[PATCH] Fix device_match_pattern function

[Santiago Carot-Nemesio]

This patch fixes a problem adding uuids to list provided when a
driver is probed. Without this patch the same uuids were added
more than once to list and if two or more uuids were in the
same class id list only the first one was included repeatedly
---
 src/device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/device.c b/src/device.c
index 6ba1612..d0768ce 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
 			continue;
 
 		if (record_has_uuid(rec, match_uuid))
-			uuids = g_slist_append(uuids, profile_uuid);
+			uuids = g_slist_append(uuids, match_uuid);
 	}
 
 	return uuids;
-- 
1.6.3.3

Re: [PATCH] Fix device_match_pattern function

[Luiz Augusto von Dentz]

Hi Santiago,

On Thu, May 20, 2010 at 12:09 PM, Santiago Carot-Nemesio
<sancane@gmail.com> wrote:
> This patch fixes a problem adding uuids to list provided when a
> driver is probed. Without this patch the same uuids were added
> more than once to list and if two or more uuids were in the
> same class id list only the first one was included repeatedly
> ---
>  src/device.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 6ba1612..d0768ce 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
>                        continue;
>
>                if (record_has_uuid(rec, match_uuid))
> -                       uuids = g_slist_append(uuids, profile_uuid);
> +                       uuids = g_slist_append(uuids, match_uuid);
>        }
>
>        return uuids;


It doesn't look right, if we do that the device will be probed by the
matched uuid which would not happen to have a record in the storage.
So in other words the list of uuids you get in the probe may not match
with the one present in the drivers .uuids. Also this would probably
break serial driver, did you tried this before submitting this to the
list?

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix device_match_pattern function

[Luiz Augusto von Dentz]

Hi,

On Thu, May 20, 2010 at 2:52 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Santiago,
>
> On Thu, May 20, 2010 at 12:09 PM, Santiago Carot-Nemesio
> <sancane@gmail.com> wrote:
>> This patch fixes a problem adding uuids to list provided when a
>> driver is probed. Without this patch the same uuids were added
>> more than once to list and if two or more uuids were in the
>> same class id list only the first one was included repeatedly
>> ---
>>  src/device.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 6ba1612..d0768ce 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
>>                        continue;
>>
>>                if (record_has_uuid(rec, match_uuid))
>> -                       uuids = g_slist_append(uuids, profile_uuid);
>> +                       uuids = g_slist_append(uuids, match_uuid);
>>        }
>>
>>        return uuids;
>
>
> It doesn't look right, if we do that the device will be probed by the
> matched uuid which would not happen to have a record in the storage.
> So in other words the list of uuids you get in the probe may not match
> with the one present in the drivers .uuids. Also this would probably
> break serial driver, did you tried this before submitting this to the
> list?

Ok, I did apply you change to see what it happens, first it didn't
compile, but anyway I add cast just to make sure what would gonna
happen:


bluetoothd[23078]: Probe drivers for /org/bluez/23078/hci0/dev_00_26_CC_77_E3_36
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: serial_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36:
00000003-0000-1000-8000-00805F9B34FB
bluetoothd[23078]: headset_probe: path
/org/bluez/23078/hci0/dev_00_26_CC_77_E3_36

See, instead of the real profile uuid all I got was a multiple rfcomm
uuid which doesn't have any meaning alone.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix device_match_pattern function

[Santiago Carot-Nemesio]

Hello Luiz,

El jue, 20-05-2010 a las 15:00 +0200, Luiz Augusto von Dentz escribió:
> Hi,
> 
> On Thu, May 20, 2010 at 2:52 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > Hi Santiago,
> >
> > On Thu, May 20, 2010 at 12:09 PM, Santiago Carot-Nemesio
> > <sancane@gmail.com> wrote:
> >> This patch fixes a problem adding uuids to list provided when a
> >> driver is probed. Without this patch the same uuids were added
> >> more than once to list and if two or more uuids were in the
> >> same class id list only the first one was included repeatedly
> >> ---
> >>  src/device.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/src/device.c b/src/device.c
> >> index 6ba1612..d0768ce 100644
> >> --- a/src/device.c
> >> +++ b/src/device.c
> >> @@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
> >>                        continue;
> >>
> >>                if (record_has_uuid(rec, match_uuid))
> >> -                       uuids = g_slist_append(uuids, profile_uuid);
> >> +                       uuids = g_slist_append(uuids, match_uuid);
> >>        }
> >>
> >>        return uuids;
> >
> >
> > It doesn't look right, if we do that the device will be probed by the
> > matched uuid which would not happen to have a record in the storage.
> > So in other words the list of uuids you get in the probe may not match
> > with the one present in the drivers .uuids. Also this would probably
> > break serial driver, did you tried this before submitting this to the
> > list?
> 
> Ok, I did apply you change to see what it happens, first it didn't
> compile, but anyway I add cast just to make sure what would gonna
> happen:
> 

Sorry, you are right I sent an erroneous patch, please excuse me that
was quite a slip-up. I'll revise it more in deep for next time. In any
case, it seems that there is a problem retrieving the service class ID
List when there are more than one entry. At current moment only the
first one is being provided when driver is probed. Please, note that I'm
speaking about profiles like HDP that can registry one or two entries
(Sink, Source or both if it is playing a dual role) although other else
profiles could want to do that too.

I'll revise that issue more in deep if you don't have any objection.

Sorry for the inconvenience and thank for your time and your attention.

Best regards.

Re: [PATCH] Fix device_match_pattern function

[Santiago Carot-Nemesio]

Hi Luiz,

I would like to take up again this matter to discuss some estrange
behaviour that we observed when a driver is probed.

> At current moment only the
> first one is being provided when driver is probed. 

Well, the comment in update_service function is clear when Service Class
Id list is checked:

/* Extract the first element and skip the remainning */

I guess that only the first entry is valid to you to get the profile and
add it to the profile list. We don't know very well why this decision
was taken. It will very helpul for us if you can provide a brief
explanation about that. Wouldn't a driver want to know if there are more
entries in that Service Class ID List when it is probed?. For example a
driver registering for two uuids and those uuids are present in the same
Service Class ID List. Please notice that it is may happen in HDP. In
this scenario only the first entry will appear in the profile list and
then one uuid is passed to the driver when it is probed, well this is
not true at all, see below for some abnormal behaviour with repeated
uuids in the list that is provided to the driver.

Let's suppose an SDP record like this:
*Service Class ID List
	MDPSink
	MDPSource
....aditional info not relevant for this example....

In that scenario, a driver is registered for next uuids:
00001400-0000-1000-8000-00805F9B34FB -> MDP (Health device profile)
00001401-0000-1000-8000-00805F9B34FB -> MDPSource
00001402-0000-1000-8000-00805F9B34FB -> MDPSink

which is perfectly valid for a HDP driver due that no extra logic is
required to manage both roles.

In current Bluez implementation only profile MDPSink is taken in count
due that it appear first in the service class ID list. In addition it
will have affect to the uuids list provided when driver is probed wich
will have repeated uuids due to an issue matching the uuid with the
profiles list: device_match_driver function.

First time that device_match_pattern is executed with uuid
00001400-0000-1000-8000-00805F9B34FB it will provide a list with the
uuid 00001402-0000-1000-8000-00805F9B34FB wich is in the profile list
(MDPSink) and that uuid will be inserted to the uuid list.

Next iteration it will check next uuid provided in the driver:
00001401-0000-1000-8000-00805F9B34FB and device_match_pattern will
return another time a list with 00001402-0000-1000-8000-00805F9B34FB
uuid wich will be inserted another time in the for loop. Please, notice
that now we have the same uuid inserted two times in the uuid list.

Next it will check last uuid provided in the driver:
00001402-0000-1000-8000-00805F9B34FB and skip it because it is already
inserted from before iteration.

You can review the code to check it.

Of course we always can get the necessary entries from the remote SDP
record, but at least the repeated uuids can be avoided by setting a
check before to insert anything in the uuid list that it will provided
when the driver is probed:

/* match pattern driver */
match = device_match_pattern(device, *uuid, profiles);
for (; match; match = match->next) {
+	if (!g_slist_find_custom(uuids, match->data,
+				(GCompareFunc) strcasecmp)) {
		uuids = g_slist_append(uuids, match->data);
+	}
}

If you want we can prepare a patch.

Comments from you are welcome.

Thanks in advance.

Best regards.

Re: [PATCH] Fix device_match_pattern function

[Luiz Augusto von Dentz]

Hi,

On Fri, May 21, 2010 at 2:17 PM, Santiago Carot-Nemesio
<scarot@libresoft.es> wrote:
> Hi Luiz,
>
> I would like to take up again this matter to discuss some estrange
> behaviour that we observed when a driver is probed.
>
>> At current moment only the
>> first one is being provided when driver is probed.
>
> Well, the comment in update_service function is clear when Service Class
> Id list is checked:
>
> /* Extract the first element and skip the remainning */
>
> I guess that only the first entry is valid to you to get the profile and
> add it to the profile list. We don't know very well why this decision
> was taken. It will very helpul for us if you can provide a brief
> explanation about that. Wouldn't a driver want to know if there are more
> entries in that Service Class ID List when it is probed?. For example a
> driver registering for two uuids and those uuids are present in the same
> Service Class ID List. Please notice that it is may happen in HDP. In
> this scenario only the first entry will appear in the profile list and
> then one uuid is passed to the driver when it is probed, well this is
> not true at all, see below for some abnormal behaviour with repeated
> uuids in the list that is provided to the driver.
>
> Let's suppose an SDP record like this:
> *Service Class ID List
>        MDPSink
>        MDPSource
> ....aditional info not relevant for this example....
>
> In that scenario, a driver is registered for next uuids:
> 00001400-0000-1000-8000-00805F9B34FB -> MDP (Health device profile)
> 00001401-0000-1000-8000-00805F9B34FB -> MDPSource
> 00001402-0000-1000-8000-00805F9B34FB -> MDPSink
>
> which is perfectly valid for a HDP driver due that no extra logic is
> required to manage both roles.
>
> In current Bluez implementation only profile MDPSink is taken in count
> due that it appear first in the service class ID list. In addition it
> will have affect to the uuids list provided when driver is probed wich
> will have repeated uuids due to an issue matching the uuid with the
> profiles list: device_match_driver function.
>
> First time that device_match_pattern is executed with uuid
> 00001400-0000-1000-8000-00805F9B34FB it will provide a list with the
> uuid 00001402-0000-1000-8000-00805F9B34FB wich is in the profile list
> (MDPSink) and that uuid will be inserted to the uuid list.
>
> Next iteration it will check next uuid provided in the driver:
> 00001401-0000-1000-8000-00805F9B34FB and device_match_pattern will
> return another time a list with 00001402-0000-1000-8000-00805F9B34FB
> uuid wich will be inserted another time in the for loop. Please, notice
> that now we have the same uuid inserted two times in the uuid list.
>
> Next it will check last uuid provided in the driver:
> 00001402-0000-1000-8000-00805F9B34FB and skip it because it is already
> inserted from before iteration.
>
> You can review the code to check it.
>
> Of course we always can get the necessary entries from the remote SDP
> record, but at least the repeated uuids can be avoided by setting a
> check before to insert anything in the uuid list that it will provided
> when the driver is probed:
>
> /* match pattern driver */
> match = device_match_pattern(device, *uuid, profiles);
> for (; match; match = match->next) {
> +       if (!g_slist_find_custom(uuids, match->data,
> +                               (GCompareFunc) strcasecmp)) {
>                uuids = g_slist_append(uuids, match->data);
> +       }
> }

Yep, this is the correct fix, but also remember that we don't use
brackets in single line if statements.

Another thing you might want to do is to only list only MDP in the
divers .uuid or only MDPSource/MDPSink if you want them to be handle
in separated drivers, I would suggest doing the former since that is
much easier to do the matching in the core and in the latter you might
run in more problems while trying to read the record from the cache
since it is not the very first service class id listen in the record.

-- 
Luiz Augusto von Dentz
Computer Engineer

[PATCH] Fix device_match_pattern function

[Santiago Carot-Nemesio]

This patch fixes a problem adding uuids to list provided when a
driver is probed. Without this patch the same uuids were added
more than once to list and if two or more uuids were in the
same class id list only the first one was included repeatedly
---
 src/device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/device.c b/src/device.c
index 6ba1612..d0768ce 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
 			continue;
 
 		if (record_has_uuid(rec, match_uuid))
-			uuids = g_slist_append(uuids, profile_uuid);
+			uuids = g_slist_append(uuids, match_uuid);
 	}
 
 	return uuids;
-- 
1.6.3.3

Re: [PATCH] Fix device_match_pattern function

[Santiago Carot]

Sorry,
I was wrong about the mailing list. Please, discard this patch here.

2010/5/20 Santiago Carot-Nemesio <sancane@gmail.com>:
> This patch fixes a problem adding uuids to list provided when a
> driver is probed. Without this patch the same uuids were added
> more than once to list and if two or more uuids were in the
> same class id list only the first one was included repeatedly
> ---
>  src/device.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 6ba1612..d0768ce 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1053,7 +1053,7 @@ static GSList *device_match_pattern(struct btd_device *device,
>                        continue;
>
>                if (record_has_uuid(rec, match_uuid))
> -                       uuids = g_slist_append(uuids, profile_uuid);
> +                       uuids = g_slist_append(uuids, match_uuid);
>        }
>
>        return uuids;
> --
> 1.6.3.3
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html