[PATCH 0/1] Remove driver only if remote has no matching UUID

[PATCH 0/1] Remove driver only if remote has no matching UUID

[Dmitriy Paliy]

Hi,

This patch of Daniel Orstadius seems to be was left without attention,
or lost. However, it fixes right thing. Currently, Headset interface
is removed for a device if one of HFP or HSP SDP records was not
provided. It doestn make sence, since the other profile is still valid.

What is done here is search of suported UUIDs of the given interface
on a remote device, and the interface is removed if none of supported
ones is found.

BR,
Dmitriy

[PATCH] Remove driver only if remote has no matching UUID

[Dmitriy Paliy]

From: Daniel Orstadius <daniel.orstadius@nokia.com>

Service discovery may generate a list of removed UUIDs. Previously
BlueZ promptly removed the btd_device_driver mapped to each
missing UUID.

With this patch it first checks if the remote has any remaining
UUID mapped to the driver, since each driver is used for several
UUIDs.

In case the driver is kept, the interface corresponding to the UUID
will not be removed (the patch deletes too little instead of too
much).
---
 src/device.c |   63 ++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/src/device.c b/src/device.c
index d624b46..f7bff75 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1187,29 +1187,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 
 	records = read_records(&src, &device->bdaddr);
 
-	DBG("Removing drivers for %s", dstaddr);
-
-	for (list = device->drivers; list; list = next) {
-		struct btd_device_driver *driver = list->data;
-		const char **uuid;
-
-		next = list->next;
-
-		for (uuid = driver->uuids; *uuid; uuid++) {
-			if (!g_slist_find_custom(uuids, *uuid,
-						(GCompareFunc) strcasecmp))
-				continue;
-
-			DBG("UUID %s was removed from device %s",
-							*uuid, dstaddr);
-
-			driver->remove(device);
-			device->drivers = g_slist_remove(device->drivers,
-								driver);
-			break;
-		}
-	}
-
 	for (list = uuids; list; list = list->next) {
 		sdp_record_t *rec;
 
@@ -1228,6 +1205,46 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 
 	if (records)
 		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+
+	DBG("Checking if drivers need to be removed for %s", dstaddr);
+
+	for (list = device->drivers; list; list = next) {
+		struct btd_device_driver *driver = list->data;
+		const char **uuid;
+
+		next = list->next;
+
+		for (uuid = driver->uuids; *uuid; uuid++) {
+			const char **uuid2;
+			gboolean found = FALSE;
+
+			if (!g_slist_find_custom(uuids, *uuid,
+						(GCompareFunc) strcasecmp))
+				continue;
+
+			DBG("UUID %s was removed from device %s",
+							*uuid, dstaddr);
+
+			/* check if there is any uuid for the driver
+			on the remote */
+			for (uuid2 = driver->uuids; *uuid2; uuid2++) {
+				if (g_slist_find_custom(device->uuids, *uuid2,
+					       (GCompareFunc) strcasecmp)) {
+					found = TRUE;
+					break;
+				}
+			}
+
+			if (!found) {
+				error("Removing driver for %s", *uuid);
+				driver->remove(device);
+				device->drivers = g_slist_remove(
+						device->drivers, driver);
+			}
+
+			break;
+		}
+	}
 }
 
 static void services_changed(struct btd_device *device)
-- 
1.6.0.4

Re: [PATCH] Remove driver only if remote has no matching UUID

[Johan Hedberg]

Hi Dmitriy,

On Sun, Nov 13, 2011, Dmitriy Paliy wrote:
> Service discovery may generate a list of removed UUIDs. Previously
> BlueZ promptly removed the btd_device_driver mapped to each
> missing UUID.
> 
> With this patch it first checks if the remote has any remaining
> UUID mapped to the driver, since each driver is used for several
> UUIDs.
> 
> In case the driver is kept, the interface corresponding to the UUID
> will not be removed (the patch deletes too little instead of too
> much).
> ---
>  src/device.c |   63 ++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/src/device.c b/src/device.c
> index d624b46..f7bff75 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1187,29 +1187,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
>  
>  	records = read_records(&src, &device->bdaddr);
>  
> -	DBG("Removing drivers for %s", dstaddr);
> -
> -	for (list = device->drivers; list; list = next) {
> -		struct btd_device_driver *driver = list->data;
> -		const char **uuid;
> -
> -		next = list->next;
> -
> -		for (uuid = driver->uuids; *uuid; uuid++) {
> -			if (!g_slist_find_custom(uuids, *uuid,
> -						(GCompareFunc) strcasecmp))
> -				continue;
> -
> -			DBG("UUID %s was removed from device %s",
> -							*uuid, dstaddr);
> -
> -			driver->remove(device);
> -			device->drivers = g_slist_remove(device->drivers,
> -								driver);
> -			break;
> -		}
> -	}
> -
>  	for (list = uuids; list; list = list->next) {
>  		sdp_record_t *rec;
>  
> @@ -1228,6 +1205,46 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
>  
>  	if (records)
>  		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
> +
> +	DBG("Checking if drivers need to be removed for %s", dstaddr);
> +
> +	for (list = device->drivers; list; list = next) {
> +		struct btd_device_driver *driver = list->data;
> +		const char **uuid;
> +
> +		next = list->next;
> +
> +		for (uuid = driver->uuids; *uuid; uuid++) {
> +			const char **uuid2;
> +			gboolean found = FALSE;
> +
> +			if (!g_slist_find_custom(uuids, *uuid,
> +						(GCompareFunc) strcasecmp))
> +				continue;
> +
> +			DBG("UUID %s was removed from device %s",
> +							*uuid, dstaddr);
> +
> +			/* check if there is any uuid for the driver
> +			on the remote */
> +			for (uuid2 = driver->uuids; *uuid2; uuid2++) {
> +				if (g_slist_find_custom(device->uuids, *uuid2,
> +					       (GCompareFunc) strcasecmp)) {

Mixed tabs and spaces in the last line above.

> +					found = TRUE;
> +					break;
> +				}
> +			}
> +
> +			if (!found) {
> +				error("Removing driver for %s", *uuid);
> +				driver->remove(device);
> +				device->drivers = g_slist_remove(
> +						device->drivers, driver);
> +			}
> +
> +			break;
> +		}
> +	}

In general this code is really quite hard to follow. I had to read it
through many times to be sure that it's doing the right thing. Since I
don't want to go through all that work every time I have to come back to
this code in the future I'd like to make it saner. I think one way to
accomplish that is through reference counting instead of constantly
iterating through the various lists and figuring out their
relationships.

So, what I'd propose is to have each successfully probed UUID contribute
a reference to the driver probed. This means that instead of a simple
UUID string you'll need to have a string + list of drivers the UUID has
been successfully probed for.  Also, the driver list for the device
needs to be constructed out of (also new) structs consisting of a
reference count and a pointer to the btd_device_driver. When a UUID gets
removed the reference gets decremented and once it reaches 0 the
driver->remove() callback can be called (which should be much easier to
understand than your multiple nested for-loops and GSList lookups).

Another question that rises here is do we need some callback to the
driver to let it know that a subset of its UUIDs have been removed.
After all, we give the full set in when probing the driver so it will
think that all UUIDs are there until remove() is called. In your case
we're dealing with HSP which doesn't really affect much, but if AVRCP
was removed (but all other profiles left intact) then the
org.bluez.Control interface should be removed. With your patch this
wouldn't happen.

My initial feeling is that we do want to let the driver know of a subset
of UUIDs being removed. I.e. we let the driver do the UUID counting
instead of the core daemon (and then you wont need the reference count
stuff like I proposed above). The device driver API options then become:

	int probe(struct btd_device *device, GSList *uuids);
	void remove(struct btd_device, *device, GSlist *uuids);

or:

	int probe(struct btd_device *device, const char *uuid);
	void remove(struct btd_device, *device, const char *uuid);

>From an implementation standpoint I think the later might end up
producing simpler code. Thinking about this now I have a feeling that we
might have considered both approaches when designing the API and for
some reason opted against it. So I'd like to hear some extra opinions on
this (especially from Marcel) before selecting a specific solution.

Johan

Re: [PATCH] Remove driver only if remote has no matching UUID

[Marcel Holtmann]

Hi Johan,

> > Service discovery may generate a list of removed UUIDs. Previously
> > BlueZ promptly removed the btd_device_driver mapped to each
> > missing UUID.
> > 
> > With this patch it first checks if the remote has any remaining
> > UUID mapped to the driver, since each driver is used for several
> > UUIDs.
> > 
> > In case the driver is kept, the interface corresponding to the UUID
> > will not be removed (the patch deletes too little instead of too
> > much).
> > ---
> >  src/device.c |   63 ++++++++++++++++++++++++++++++++++++---------------------
> >  1 files changed, 40 insertions(+), 23 deletions(-)
> > 
> > diff --git a/src/device.c b/src/device.c
> > index d624b46..f7bff75 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -1187,29 +1187,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
> >  
> >  	records = read_records(&src, &device->bdaddr);
> >  
> > -	DBG("Removing drivers for %s", dstaddr);
> > -
> > -	for (list = device->drivers; list; list = next) {
> > -		struct btd_device_driver *driver = list->data;
> > -		const char **uuid;
> > -
> > -		next = list->next;
> > -
> > -		for (uuid = driver->uuids; *uuid; uuid++) {
> > -			if (!g_slist_find_custom(uuids, *uuid,
> > -						(GCompareFunc) strcasecmp))
> > -				continue;
> > -
> > -			DBG("UUID %s was removed from device %s",
> > -							*uuid, dstaddr);
> > -
> > -			driver->remove(device);
> > -			device->drivers = g_slist_remove(device->drivers,
> > -								driver);
> > -			break;
> > -		}
> > -	}
> > -
> >  	for (list = uuids; list; list = list->next) {
> >  		sdp_record_t *rec;
> >  
> > @@ -1228,6 +1205,46 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
> >  
> >  	if (records)
> >  		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
> > +
> > +	DBG("Checking if drivers need to be removed for %s", dstaddr);
> > +
> > +	for (list = device->drivers; list; list = next) {
> > +		struct btd_device_driver *driver = list->data;
> > +		const char **uuid;
> > +
> > +		next = list->next;
> > +
> > +		for (uuid = driver->uuids; *uuid; uuid++) {
> > +			const char **uuid2;
> > +			gboolean found = FALSE;
> > +
> > +			if (!g_slist_find_custom(uuids, *uuid,
> > +						(GCompareFunc) strcasecmp))
> > +				continue;
> > +
> > +			DBG("UUID %s was removed from device %s",
> > +							*uuid, dstaddr);
> > +
> > +			/* check if there is any uuid for the driver
> > +			on the remote */
> > +			for (uuid2 = driver->uuids; *uuid2; uuid2++) {
> > +				if (g_slist_find_custom(device->uuids, *uuid2,
> > +					       (GCompareFunc) strcasecmp)) {
> 
> Mixed tabs and spaces in the last line above.
> 
> > +					found = TRUE;
> > +					break;
> > +				}
> > +			}
> > +
> > +			if (!found) {
> > +				error("Removing driver for %s", *uuid);
> > +				driver->remove(device);
> > +				device->drivers = g_slist_remove(
> > +						device->drivers, driver);
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> 
> In general this code is really quite hard to follow. I had to read it
> through many times to be sure that it's doing the right thing. Since I
> don't want to go through all that work every time I have to come back to
> this code in the future I'd like to make it saner. I think one way to
> accomplish that is through reference counting instead of constantly
> iterating through the various lists and figuring out their
> relationships.
> 
> So, what I'd propose is to have each successfully probed UUID contribute
> a reference to the driver probed. This means that instead of a simple
> UUID string you'll need to have a string + list of drivers the UUID has
> been successfully probed for.  Also, the driver list for the device
> needs to be constructed out of (also new) structs consisting of a
> reference count and a pointer to the btd_device_driver. When a UUID gets
> removed the reference gets decremented and once it reaches 0 the
> driver->remove() callback can be called (which should be much easier to
> understand than your multiple nested for-loops and GSList lookups).
> 
> Another question that rises here is do we need some callback to the
> driver to let it know that a subset of its UUIDs have been removed.
> After all, we give the full set in when probing the driver so it will
> think that all UUIDs are there until remove() is called. In your case
> we're dealing with HSP which doesn't really affect much, but if AVRCP
> was removed (but all other profiles left intact) then the
> org.bluez.Control interface should be removed. With your patch this
> wouldn't happen.
> 
> My initial feeling is that we do want to let the driver know of a subset
> of UUIDs being removed. I.e. we let the driver do the UUID counting
> instead of the core daemon (and then you wont need the reference count
> stuff like I proposed above). The device driver API options then become:
> 
> 	int probe(struct btd_device *device, GSList *uuids);
> 	void remove(struct btd_device, *device, GSlist *uuids);
> 
> or:
> 
> 	int probe(struct btd_device *device, const char *uuid);
> 	void remove(struct btd_device, *device, const char *uuid);
> 
> From an implementation standpoint I think the later might end up
> producing simpler code. Thinking about this now I have a feeling that we
> might have considered both approaches when designing the API and for
> some reason opted against it. So I'd like to hear some extra opinions on
> this (especially from Marcel) before selecting a specific solution.

so our general approach is against any kind of GLib types for exported
functions to plugins/drivers. So the first approach is off the table.

For the second one, I do not remember. It might have been that we wanted
to keep the probe() functions simple. Maybe this needs to be revisited.

Regards

Marcel