[PATCH] Update SDP storage records when a record is deleted.

[PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

When a SDP record is deleted at the remote end, we update
the storage only if we have a device driver registered for that UUID.
Update the cache in all cases.
---
 src/device.c |   48 +++++++++++++++++++++++++++---------------------
 1 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/src/device.c b/src/device.c
index 63f35de..9386681 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1110,19 +1110,37 @@ void device_probe_drivers(struct btd_device *device, GSList *profiles)
 	}
 }
 
-static void device_remove_drivers(struct btd_device *device, GSList *uuids)
-{
+static void update_service_records(struct btd_device *device) {
 	struct btd_adapter *adapter = device_get_adapter(device);
-	GSList *list, *next;
-	char srcaddr[18], dstaddr[18];
 	bdaddr_t src;
-	sdp_list_t *records;
+	sdp_list_t *records, *seq;
+	sdp_record_t *rec;
+	char srcaddr[18], dstaddr[18];
+	GSList *l;
+	const char *uuid;
 
 	adapter_get_address(adapter, &src);
 	ba2str(&src, srcaddr);
 	ba2str(&device->bdaddr, dstaddr);
 
 	records = read_records(&src, &device->bdaddr);
+	for (l = device->uuids; l; l = l->next) {
+		uuid = l->data;
+		rec = find_record_in_list(records, uuid);
+		if (rec)
+			records = sdp_list_remove(records, rec);
+	}
+	for (seq = records; seq; seq = seq->next) {
+		rec = (sdp_record_t *) seq->data;
+		delete_record(srcaddr, dstaddr, rec->handle);
+	}
+	if (records)
+		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+}
+
+static void device_remove_drivers(struct btd_device *device, GSList *uuids)
+{
+	GSList *list, *next;
 
 	debug("Remove drivers for %s", device->path);
 
@@ -1134,36 +1152,22 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 		next = list->next;
 
 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
 
-			debug("UUID %s was removed from device %s",
-							*uuid, dstaddr);
+			debug("UUID %s was removed from device path %s",
+							*uuid, device->path);
 
 			driver->remove(device);
 			device->drivers = g_slist_remove(device->drivers,
 								driver_data);
 			g_free(driver_data);
 
-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
-
-			delete_record(srcaddr, dstaddr, rec->handle);
-
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
-
 			break;
 		}
 	}
 
-	if (records)
-		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
-
 	for (list = uuids; list; list = list->next)
 		device->uuids = g_slist_remove(device->uuids, list->data);
 }
@@ -1333,6 +1337,8 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
 	if (req->profiles_removed)
 		device_remove_drivers(device, req->profiles_removed);
 
+	update_service_records(device);
+
 	/* Propagate services changes */
 	services_changed(req->device);
 
-- 
1.6.2.3

Re: [PATCH] Update SDP storage records when a record is deleted.

[Johan Hedberg]

Hi Jaikumar,

A few quick coments on this:

On Thu, Oct 22, 2009, Jaikumar Ganesh wrote:
> +static void update_service_records(struct btd_device *device) {

Coding style: opening bracket of a function on its own line.

>  	records = read_records(&src, &device->bdaddr);
> +	for (l = device->uuids; l; l = l->next) {
> +		uuid = l->data;
> +		rec = find_record_in_list(records, uuid);
> +		if (rec)
> +			records = sdp_list_remove(records, rec);
> +	}

It looks like you're leaking memory here. I guess rec should be freed
after the sdp_list_remove.

> +	for (seq = records; seq; seq = seq->next) {
> +		rec = (sdp_record_t *) seq->data;

Since seq->data is void* the cast is unnecessary here.

> +		delete_record(srcaddr, dstaddr, rec->handle);
> +	}
> +	if (records)
> +		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
> +}

Coding style: I'd add empty lines between the for-loops as well as before
the if-statement here.

Other than those issues one thing that bothers me a little is that it
seems the req->profiles_removed list isn't used anymore at this point but
a list of removed records is built from scratch a second time. Are you
sure that this existing list of removed UUIDs couldn't be used somehow to
make the code more efficient? It might well be that this I'm missing
something and this list can't be used but I thought it'd be good to check
that you've considered this possibility nevertheless.

Johan

Re: [PATCH] Update SDP storage records when a record is deleted.

[Luiz Augusto von Dentz]

Hi,

On Thu, Oct 22, 2009 at 8:19 PM, Jaikumar Ganesh <jaikumar@google.com> wrote:
>        for (list = uuids; list; list = list->next)
>                device->uuids = g_slist_remove(device->uuids, list->data);

It looks like to me this would be the proper place to remove the
records from the storage, since we are updating the uuids list there
is no big deal to redo that in another function.

So it would look like this:

diff --git a/src/device.c b/src/device.c
index 6cb9641..708e069 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1134,8 +1134,6 @@ static void device_remove_drivers(struct
btd_device *device, GSList *uuids)
 		next = list->next;

 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
@@ -1148,15 +1146,6 @@ static void device_remove_drivers(struct
btd_device *device, GSList *uuids)
 								driver_data);
 			g_free(driver_data);

-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
-
-			delete_record(srcaddr, dstaddr, rec->handle);
-
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
-
 			break;
 		}
 	}
@@ -1164,8 +1153,19 @@ static void device_remove_drivers(struct
btd_device *device, GSList *uuids)
 	if (records)
 		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);

-	for (list = uuids; list; list = list->next)
+	for (list = uuids; list; list = list->next) {
+		sdp_record_t *rec;
+
 		device->uuids = g_slist_remove(device->uuids, list->data);
+		rec = find_record_in_list(records, list->data);
+		if (!rec)
+			break;
+
+		delete_record(srcaddr, dstaddr, rec->handle);
+
+		records = sdp_list_remove(records, rec);
+		sdp_record_free(rec);
+	}
 }

 static void services_changed(struct btd_device *device)


-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

[-- Attachment #1: Type: text/plain, Size: 3445 bytes --]

Hi Luiz:

On Fri, Oct 23, 2009 at 6:49 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
>
> On Thu, Oct 22, 2009 at 8:19 PM, Jaikumar Ganesh <jaikumar@google.com> wrote:
> >        for (list = uuids; list; list = list->next)
> >                device->uuids = g_slist_remove(device->uuids, list->data);
>
> It looks like to me this would be the proper place to remove the
> records from the storage, since we are updating the uuids list there
> is no big deal to redo that in another function.
>
> So it would look like this:
>
> diff --git a/src/device.c b/src/device.c
> index 6cb9641..708e069 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1134,8 +1134,6 @@ static void device_remove_drivers(struct
> btd_device *device, GSList *uuids)
>                next = list->next;
>
>                for (uuid = driver->uuids; *uuid; uuid++) {
> -                       sdp_record_t *rec;
> -
>                        if (!g_slist_find_custom(uuids, *uuid,
>                                        (GCompareFunc) strcasecmp))
>                                continue;
> @@ -1148,15 +1146,6 @@ static void device_remove_drivers(struct
> btd_device *device, GSList *uuids)
>                                                                driver_data);
>                        g_free(driver_data);
>
> -                       rec = find_record_in_list(records, *uuid);
> -                       if (!rec)
> -                               break;
> -
> -                       delete_record(srcaddr, dstaddr, rec->handle);
> -
> -                       records = sdp_list_remove(records, rec);
> -                       sdp_record_free(rec);
> -
>                        break;
>                }
>        }
> @@ -1164,8 +1153,19 @@ static void device_remove_drivers(struct
> btd_device *device, GSList *uuids)
>        if (records)
>                sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
>
> -       for (list = uuids; list; list = list->next)
> +       for (list = uuids; list; list = list->next) {
> +               sdp_record_t *rec;
> +
>                device->uuids = g_slist_remove(device->uuids, list->data);
> +               rec = find_record_in_list(records, list->data);
> +               if (!rec)
> +                       break;
> +
> +               delete_record(srcaddr, dstaddr, rec->handle);
> +
> +               records = sdp_list_remove(records, rec);
> +               sdp_record_free(rec);
> +       }
>  }
>
>  static void services_changed(struct btd_device *device)
>

My mail to linux bluetooth bounced yesterday.  Sending it again.

    The req->profiles_removed gets updated from device->uuids.
However, in the case, where we are paired with the remote device and
we unpair and then repair, since we will free device->uuids on the
unpair, the req->profiles_removed won't get updated after the SDP
query on repair. And so we would need to update the storage from
device->uuids.

Since req->profiles_removed would be empty, device_remove_drivers
won't get called. Also I believe it makes sense to have 2 functions,
even though there is slight overhead, since the deletion of SDP
records from storage, and removing device_probe_drivers are 2 distinct
operations.

   Coding style, cast issue, memory leak fixed, patch attached.

>
> --
> Luiz Augusto von Dentz
> Engenheiro de Computação

[-- Attachment #2: 0002-Update-SDP-storage-records-when-a-record-is-deleted.patch --]
[-- Type: application/octet-stream, Size: 3262 bytes --]

From 45fc714460b82c0b3d82d73d00582d9a5618d267 Mon Sep 17 00:00:00 2001
From: Jaikumar Ganesh <jaikumar@google.com>
Date: Thu, 22 Oct 2009 16:17:17 -0700
Subject: [PATCH] Update SDP storage records when a record is deleted.

When a SDP record is deleted at the remote end, we update
the storage only if we have a device driver registered for that UUID.
Update the cache in all cases.
---
 src/device.c |   52 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/src/device.c b/src/device.c
index 63f35de..2b0c80a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1110,13 +1110,15 @@ void device_probe_drivers(struct btd_device *device, GSList *profiles)
 	}
 }
 
-static void device_remove_drivers(struct btd_device *device, GSList *uuids)
+static void update_service_records(struct btd_device *device)
 {
 	struct btd_adapter *adapter = device_get_adapter(device);
-	GSList *list, *next;
-	char srcaddr[18], dstaddr[18];
 	bdaddr_t src;
-	sdp_list_t *records;
+	sdp_list_t *records, *seq;
+	sdp_record_t *rec;
+	char srcaddr[18], dstaddr[18];
+	GSList *l;
+	const char *uuid;
 
 	adapter_get_address(adapter, &src);
 	ba2str(&src, srcaddr);
@@ -1124,6 +1126,28 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 
 	records = read_records(&src, &device->bdaddr);
 
+	for (l = device->uuids; l; l = l->next) {
+		uuid = l->data;
+		rec = find_record_in_list(records, uuid);
+		if (rec) {
+			records = sdp_list_remove(records, rec);
+			sdp_record_free(rec);
+		}
+	}
+
+	for (seq = records; seq; seq = seq->next) {
+		rec = seq->data;
+		delete_record(srcaddr, dstaddr, rec->handle);
+	}
+
+	if (records)
+		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+}
+
+static void device_remove_drivers(struct btd_device *device, GSList *uuids)
+{
+	GSList *list, *next;
+
 	debug("Remove drivers for %s", device->path);
 
 	for (list = device->drivers; list; list = next) {
@@ -1134,36 +1158,22 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 		next = list->next;
 
 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
 
-			debug("UUID %s was removed from device %s",
-							*uuid, dstaddr);
+			debug("UUID %s was removed from device path %s",
+							*uuid, device->path);
 
 			driver->remove(device);
 			device->drivers = g_slist_remove(device->drivers,
 								driver_data);
 			g_free(driver_data);
 
-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
-
-			delete_record(srcaddr, dstaddr, rec->handle);
-
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
-
 			break;
 		}
 	}
 
-	if (records)
-		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
-
 	for (list = uuids; list; list = list->next)
 		device->uuids = g_slist_remove(device->uuids, list->data);
 }
@@ -1333,6 +1343,8 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
 	if (req->profiles_removed)
 		device_remove_drivers(device, req->profiles_removed);
 
+	update_service_records(device);
+
 	/* Propagate services changes */
 	services_changed(req->device);
 
-- 
1.6.2.3

Re: [PATCH] Update SDP storage records when a record is deleted.

[Luiz Augusto von Dentz]

Hi,

On Fri, Oct 23, 2009 at 12:17 PM, Jaikumar Ganesh <jaikumar@google.com> wrote:
>
> My mail to linux bluetooth bounced yesterday.  Sending it again.
>
>    The req->profiles_removed gets updated from device->uuids.
> However, in the case, where we are paired with the remote device and
> we unpair and then repair, since we will free device->uuids on the
> unpair, the req->profiles_removed won't get updated after the SDP
> query on repair. And so we would need to update the storage from
> device->uuids.

Im not sure if I get you right, but if you are saying that when device
object is gone profiles_removed is empty that is the a new device and
we shouldn't have any profile at that point for consistence, so either
we remove the sdp cache on device_remove_stored or we never remove
records from the cache. Perhaps the record cache is really useless and
we should only cache the uuids information and query the record on
every connect attempt (we are going to connect anyway so the cost is
about the same.)

> Since req->profiles_removed would be empty, device_remove_drivers
> won't get called. Also I believe it makes sense to have 2 functions,
> even though there is slight overhead, since the deletion of SDP
> records from storage, and removing device_probe_drivers are 2 distinct
> operations.

Maybe if you called it on device_remove_stored too, but if we are
going to validate that information on every connection attempt I doubt
this is useful in the end.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

Hi Luiz:

On Fri, Oct 23, 2009 at 10:44 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
>
> On Fri, Oct 23, 2009 at 12:17 PM, Jaikumar Ganesh <jaikumar@google.com> wrote:
> >
> > My mail to linux bluetooth bounced yesterday.  Sending it again.
> >
> >    The req->profiles_removed gets updated from device->uuids.
> > However, in the case, where we are paired with the remote device and
> > we unpair and then repair, since we will free device->uuids on the
> > unpair, the req->profiles_removed won't get updated after the SDP
> > query on repair. And so we would need to update the storage from
> > device->uuids.
>
> Im not sure if I get you right, but if you are saying that when device
> object is gone profiles_removed is empty that is the a new device and
> we shouldn't have any profile at that point for consistence, so either
> we remove the sdp cache on device_remove_stored or we never remove
> records from the cache. Perhaps the record cache is really useless and
> we should only cache the uuids information and query the record on
> every connect attempt (we are going to connect anyway so the cost is
> about the same.)

When the device is removed, we remove the cache and remove the device->uuids
value. So when the device is created the next time, we cannot depend on
the profiles_removed value, we need to reconstruct from device->uuids.
device_remove_drivers call is based on the profiles_removed flag and hence that
call will never be made, when a new device is created.

My change removes the dependency of SDP records and the probe_drivers,
Thus, removal of SDP records doesn't depend on the profiles_removed flag
but rather uses device->uuids directly.

profiles_added and profiles_removed flags are useful in the case where the user
calls discover services on an already created device and there are
changes in the SDP
records.

>
> > Since req->profiles_removed would be empty, device_remove_drivers
> > won't get called. Also I believe it makes sense to have 2 functions,
> > even though there is slight overhead, since the deletion of SDP
> > records from storage, and removing device_probe_drivers are 2 distinct
> > operations.
>
> Maybe if you called it on device_remove_stored too, but if we are
> going to validate that information on every connection attempt I doubt
> this is useful in the end.
>
> --
> Luiz Augusto von Dentz
> Engenheiro de Computação

Thanks
Jaikumar

Re: [PATCH] Update SDP storage records when a record is deleted.

[jaikumar Ganesh]

Hey Luiz, Johan:

On Mon, Oct 26, 2009 at 8:20 AM, Jaikumar Ganesh <jaikumar@google.com> wrote:
> Hi Luiz:
>
> On Fri, Oct 23, 2009 at 10:44 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi,
>>
>> On Fri, Oct 23, 2009 at 12:17 PM, Jaikumar Ganesh <jaikumar@google.com> wrote:
>> >
>> > My mail to linux bluetooth bounced yesterday.  Sending it again.
>> >
>> >    The req->profiles_removed gets updated from device->uuids.
>> > However, in the case, where we are paired with the remote device and
>> > we unpair and then repair, since we will free device->uuids on the
>> > unpair, the req->profiles_removed won't get updated after the SDP
>> > query on repair. And so we would need to update the storage from
>> > device->uuids.
>>
>> Im not sure if I get you right, but if you are saying that when device
>> object is gone profiles_removed is empty that is the a new device and
>> we shouldn't have any profile at that point for consistence, so either
>> we remove the sdp cache on device_remove_stored or we never remove
>> records from the cache. Perhaps the record cache is really useless and
>> we should only cache the uuids information and query the record on
>> every connect attempt (we are going to connect anyway so the cost is
>> about the same.)
>
> When the device is removed, we remove the cache and remove the device->uuids
> value. So when the device is created the next time, we cannot depend on
> the profiles_removed value, we need to reconstruct from device->uuids.
> device_remove_drivers call is based on the profiles_removed flag and hence that
> call will never be made, when a new device is created.
>
> My change removes the dependency of SDP records and the probe_drivers,
> Thus, removal of SDP records doesn't depend on the profiles_removed flag
> but rather uses device->uuids directly.
>
> profiles_added and profiles_removed flags are useful in the case where the user
> calls discover services on an already created device and there are
> changes in the SDP
> records.
>
>>
>> > Since req->profiles_removed would be empty, device_remove_drivers
>> > won't get called. Also I believe it makes sense to have 2 functions,
>> > even though there is slight overhead, since the deletion of SDP
>> > records from storage, and removing device_probe_drivers are 2 distinct
>> > operations.
>>
>> Maybe if you called it on device_remove_stored too, but if we are
>> going to validate that information on every connection attempt I doubt
>> this is useful in the end.
>>
>> --
>> Luiz Augusto von Dentz
>> Engenheiro de Computação
>
> Thanks
> Jaikumar
> --
> 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
>

Any update on this ?

Thanks
Jaikumar

Re: [PATCH] Update SDP storage records when a record is deleted.

[Johan Hedberg]

Hi Jaikumar,

On Mon, Oct 26, 2009, Jaikumar Ganesh wrote:
> When the device is removed, we remove the cache and remove the device->uuids
> value. So when the device is created the next time, we cannot depend on
> the profiles_removed value, we need to reconstruct from device->uuids.
> device_remove_drivers call is based on the profiles_removed flag and hence that
> call will never be made, when a new device is created.
> 
> My change removes the dependency of SDP records and the probe_drivers,
> Thus, removal of SDP records doesn't depend on the profiles_removed flag
> but rather uses device->uuids directly.
> 
> profiles_added and profiles_removed flags are useful in the case where
> the user calls discover services on an already created device and there
> are changes in the SDP records.

If a device was removed then we shouldn't have any records stored records
for it left in the cache and so it shouldn't matter that the
profiles_removed list is empty when the device is recreated again, right?
It seems there's a bug in the device_remove_stored function in device.c
since it doesn't remove the record cache, so that's at least something
that should be fixed.

Johan

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

Hi Johan:

On Wed, Oct 28, 2009 at 4:34 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Jaikumar,
>
> On Mon, Oct 26, 2009, Jaikumar Ganesh wrote:
>> When the device is removed, we remove the cache and remove the device->uuids
>> value. So when the device is created the next time, we cannot depend on
>> the profiles_removed value, we need to reconstruct from device->uuids.
>> device_remove_drivers call is based on the profiles_removed flag and hence that
>> call will never be made, when a new device is created.
>>
>> My change removes the dependency of SDP records and the probe_drivers,
>> Thus, removal of SDP records doesn't depend on the profiles_removed flag
>> but rather uses device->uuids directly.
>>
>> profiles_added and profiles_removed flags are useful in the case where
>> the user calls discover services on an already created device and there
>> are changes in the SDP records.
>
> If a device was removed then we shouldn't have any records stored records
> for it left in the cache and so it shouldn't matter that the
> profiles_removed list is empty when the device is recreated again, right?
> It seems there's a bug in the device_remove_stored function in device.c
> since it doesn't remove the record cache, so that's at least something
> that should be fixed.
>

The SDP cache is  removed when the device is removed. In fact, you
fixed this a few days back.
This is the scenario I am referring to:

a) Device is paired and SDP records fetched.
b) Device is unpaired - device cache records of SDP is freed
c) On Remote Device - some SDP records are deleted.
d) When the device is created again and the SDP records fetched,
profiles_removed is empty and so

        if (req->profiles_removed)
                device_remove_drivers(device, req->profiles_removed);

device_remove_drivers will not be called and hence SDP records will
never get deleted.

e) My change removes the coupling of SDP records with the drivers code
and hence we will remove the SDP records in this scenario.

> Johan
>

Thanks
Jaikumar

Re: [PATCH] Update SDP storage records when a record is deleted.

[Johan Hedberg]

Hi,

On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
> The SDP cache is  removed when the device is removed. In fact, you
> fixed this a few days back.

Which commit are you referring to?

> This is the scenario I am referring to:
> a) Device is paired and SDP records fetched.
> b) Device is unpaired - device cache records of SDP is freed

Ok, so at this point we don't have anything stored about the device,
neither on disk nor in runtime memory.

> c) On Remote Device - some SDP records are deleted.
> d) When the device is created again and the SDP records fetched,
> profiles_removed is empty and so
> 
>         if (req->profiles_removed)
>                 device_remove_drivers(device, req->profiles_removed);
> 
> device_remove_drivers will not be called and hence SDP records will
> never get deleted.

But what is there to delete if in step b) we already deleted everything?

Johan

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

Hi Johan:

On Wed, Oct 28, 2009 at 4:54 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
>> The SDP cache is  removed when the device is removed. In fact, you
>> fixed this a few days back.
>
> Which commit are you referring to?

Commit number:

4bec43039626e853489e72149014868f8c8afedc

http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=4bec43039626e853489e72149014868f8c8afedc


>
>> This is the scenario I am referring to:
>> a) Device is paired and SDP records fetched.
>> b) Device is unpaired - device cache records of SDP is freed
>
> Ok, so at this point we don't have anything stored about the device,
> neither on disk nor in runtime memory.

I should have been more clear - we only remove the runtime memory cache
The on disk SDP records are not removed.

>
>> c) On Remote Device - some SDP records are deleted.
>> d) When the device is created again and the SDP records fetched,
>> profiles_removed is empty and so
>>
>>         if (req->profiles_removed)
>>                 device_remove_drivers(device, req->profiles_removed);
>>
>> device_remove_drivers will not be called and hence SDP records will
>> never get deleted.
>
> But what is there to delete if in step b) we already deleted everything?

We can also fix this by removing the on disk SDP records when the
device is freed.
(I think which is what you suggested - by cache I was interpreting it
as in memory cache)
Will submit a new patch. Let me know if I read you wrong.

>
> Johan
>

Re: [PATCH] Update SDP storage records when a record is deleted.

[Johan Hedberg]

Hi,

On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
> > On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
> >> The SDP cache is  removed when the device is removed. In fact, you
> >> fixed this a few days back.
> >
> > Which commit are you referring to?
> 
> Commit number:
> 
> 4bec43039626e853489e72149014868f8c8afedc
> 
> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=4bec43039626e853489e72149014868f8c8afedc

Ok, so we're talking about different things here. I was talking about
removing the records from /var/lib/bluetooth/...

> We can also fix this by removing the on disk SDP records when the
> device is freed.
> (I think which is what you suggested - by cache I was interpreting it
> as in memory cache)
> Will submit a new patch. Let me know if I read you wrong.

No, you're right. I was referring to the disk cache. I'm glad we have this
misunderstanding finally sorted out :)

Johan

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Hi Johan:

On Wed, Oct 28, 2009 at 5:15 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
>> > On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
>> >> The SDP cache is  removed when the device is removed. In fact, you
>> >> fixed this a few days back.
>> >
>> > Which commit are you referring to?
>>
>> Commit number:
>>
>> 4bec43039626e853489e72149014868f8c8afedc
>>
>> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=4bec43039626e853489e72149014868f8c8afedc
>
> Ok, so we're talking about different things here. I was talking about
> removing the records from /var/lib/bluetooth/...
>
>> We can also fix this by removing the on disk SDP records when the
>> device is freed.
>> (I think which is what you suggested - by cache I was interpreting it
>> as in memory cache)
>> Will submit a new patch. Let me know if I read you wrong.
>
> No, you're right. I was referring to the disk cache. I'm glad we have this
> misunderstanding finally sorted out :)

   New patch attached.

>
> Johan
>

[-- Attachment #2: 0001-Fix-handling-of-SDP-records.patch --]
[-- Type: text/x-diff, Size: 3889 bytes --]

From 3b973c8a8fcc32ac9fe8ef939932800d70b59e48 Mon Sep 17 00:00:00 2001
From: Jaikumar Ganesh <jaikumar@google.com>
Date: Thu, 29 Oct 2009 11:23:25 -0700
Subject: [PATCH] Fix handling of SDP records.

1. Delete all SDP records when the device is removed.
   We are going to do the SDP connection again anyways next time.
   This fixes the issues of stale SDP records.
2. Handle deletion of SDP records even when there is no driver
   registered for that UUID
---
 src/device.c  |   29 ++++++++++++++++-------------
 src/storage.c |   20 ++++++++++++++++++++
 src/storage.h |    1 +
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/src/device.c b/src/device.c
index 6cb9641..760e661 100644
--- a/src/device.c
+++ b/src/device.c
@@ -937,10 +937,12 @@ static void device_remove_stored(struct btd_device *device)
 
 	adapter_get_address(device->adapter, &src);
 	ba2str(&device->bdaddr, addr);
+
 	if (device->paired)
 		device_remove_bonding(device);
 	delete_entry(&src, "profiles", addr);
 	delete_entry(&src, "trusts", addr);
+	delete_all_records(&src, &device->bdaddr);
 }
 
 void device_remove(struct btd_device *device, gboolean remove_stored)
@@ -1134,8 +1136,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 		next = list->next;
 
 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
@@ -1148,24 +1148,27 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 								driver_data);
 			g_free(driver_data);
 
-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
+			break;
+		}
+	}
+
+	for (list = uuids; list; list = list->next) {
+		device->uuids = g_slist_remove(device->uuids, list->data);
+
+		sdp_record_t *rec;
+		rec = find_record_in_list(records, list->data);
+		if (!rec)
+			continue;
 
-			delete_record(srcaddr, dstaddr, rec->handle);
+		delete_record(srcaddr, dstaddr, rec->handle);
 
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
+		records = sdp_list_remove(records, rec);
+		sdp_record_free(rec);
 
-			break;
-		}
 	}
 
 	if (records)
 		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
-
-	for (list = uuids; list; list = list->next)
-		device->uuids = g_slist_remove(device->uuids, list->data);
 }
 
 static void services_changed(struct btd_device *device)
diff --git a/src/storage.c b/src/storage.c
index 5760656..cd33206 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -913,6 +913,26 @@ static void create_stored_records_from_keys(char *key, char *value,
 	rec_list->recs = sdp_list_append(rec_list->recs, rec);
 }
 
+void delete_all_records(bdaddr_t *src, bdaddr_t *dst)
+{
+	sdp_list_t *records, *seq;
+	sdp_record_t *rec;
+	char srcaddr[18], dstaddr[18];
+
+	ba2str(src, srcaddr);
+	ba2str(dst, dstaddr);
+
+	records = read_records(src, dst);
+
+	for (seq = records; seq; seq = seq->next) {
+		rec = seq->data;
+		delete_record(srcaddr, dstaddr, rec->handle);
+	}
+
+	if (records)
+		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+}
+
 sdp_list_t *read_records(bdaddr_t *src, bdaddr_t *dst)
 {
 	char filename[PATH_MAX + 1];
diff --git a/src/storage.h b/src/storage.h
index 3159f2a..6e40151 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -63,6 +63,7 @@ int store_record(const gchar *src, const gchar *dst, sdp_record_t *rec);
 sdp_record_t *record_from_string(const gchar *str);
 sdp_record_t *fetch_record(const gchar *src, const gchar *dst, const uint32_t handle);
 int delete_record(const gchar *src, const gchar *dst, const uint32_t handle);
+void delete_all_records(bdaddr_t *src, bdaddr_t *dst);
 sdp_list_t *read_records(bdaddr_t *src, bdaddr_t *dst);
 sdp_record_t *find_record_in_list(sdp_list_t *recs, const char *uuid);
 int store_device_id(const gchar *src, const gchar *dst,
-- 
1.6.2.3

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

Hi Johan:

On Thu, Oct 29, 2009 at 11:42 AM, Jaikumar Ganesh <jaikumar@google.com> wrote:
> Hi Johan:
>
> On Wed, Oct 28, 2009 at 5:15 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Hi,
>>
>> On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
>>> > On Wed, Oct 28, 2009, Jaikumar Ganesh wrote:
>>> >> The SDP cache is  removed when the device is removed. In fact, you
>>> >> fixed this a few days back.
>>> >
>>> > Which commit are you referring to?
>>>
>>> Commit number:
>>>
>>> 4bec43039626e853489e72149014868f8c8afedc
>>>
>>> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=4bec43039626e853489e72149014868f8c8afedc
>>
>> Ok, so we're talking about different things here. I was talking about
>> removing the records from /var/lib/bluetooth/...
>>
>>> We can also fix this by removing the on disk SDP records when the
>>> device is freed.
>>> (I think which is what you suggested - by cache I was interpreting it
>>> as in memory cache)
>>> Will submit a new patch. Let me know if I read you wrong.
>>
>> No, you're right. I was referring to the disk cache. I'm glad we have this
>> misunderstanding finally sorted out :)
>
>   New patch attached.

The patch broken into 2 patches are attached.

>
>>
>> Johan
>>
>

[-- Attachment #2: 0001-When-a-device-is-removed-delete-the-SDP-records.patch --]
[-- Type: text/x-diff, Size: 2600 bytes --]

From edfa5d2aac3173964e914f9feea511e62025e051 Mon Sep 17 00:00:00 2001
From: Jaikumar Ganesh <jaikumar@google.com>
Date: Thu, 29 Oct 2009 13:19:51 -0700
Subject: [PATCH 1/2] When a device is removed delete the SDP records.

This fixes the issues of stale SDP records, when the
SDP records are modified on the remote end, before the next
connection. Since we are going to perform SDP on the next connection
anyway, this doesn't introduce any new overheads.
---
 src/device.c  |    2 ++
 src/storage.c |   20 ++++++++++++++++++++
 src/storage.h |    1 +
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 6cb9641..5585271 100644
--- a/src/device.c
+++ b/src/device.c
@@ -937,10 +937,12 @@ static void device_remove_stored(struct btd_device *device)
 
 	adapter_get_address(device->adapter, &src);
 	ba2str(&device->bdaddr, addr);
+
 	if (device->paired)
 		device_remove_bonding(device);
 	delete_entry(&src, "profiles", addr);
 	delete_entry(&src, "trusts", addr);
+	delete_all_records(&src, &device->bdaddr);
 }
 
 void device_remove(struct btd_device *device, gboolean remove_stored)
diff --git a/src/storage.c b/src/storage.c
index 5760656..cd33206 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -913,6 +913,26 @@ static void create_stored_records_from_keys(char *key, char *value,
 	rec_list->recs = sdp_list_append(rec_list->recs, rec);
 }
 
+void delete_all_records(bdaddr_t *src, bdaddr_t *dst)
+{
+	sdp_list_t *records, *seq;
+	sdp_record_t *rec;
+	char srcaddr[18], dstaddr[18];
+
+	ba2str(src, srcaddr);
+	ba2str(dst, dstaddr);
+
+	records = read_records(src, dst);
+
+	for (seq = records; seq; seq = seq->next) {
+		rec = seq->data;
+		delete_record(srcaddr, dstaddr, rec->handle);
+	}
+
+	if (records)
+		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+}
+
 sdp_list_t *read_records(bdaddr_t *src, bdaddr_t *dst)
 {
 	char filename[PATH_MAX + 1];
diff --git a/src/storage.h b/src/storage.h
index 3159f2a..6e40151 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -63,6 +63,7 @@ int store_record(const gchar *src, const gchar *dst, sdp_record_t *rec);
 sdp_record_t *record_from_string(const gchar *str);
 sdp_record_t *fetch_record(const gchar *src, const gchar *dst, const uint32_t handle);
 int delete_record(const gchar *src, const gchar *dst, const uint32_t handle);
+void delete_all_records(bdaddr_t *src, bdaddr_t *dst);
 sdp_list_t *read_records(bdaddr_t *src, bdaddr_t *dst);
 sdp_record_t *find_record_in_list(sdp_list_t *recs, const char *uuid);
 int store_device_id(const gchar *src, const gchar *dst,
-- 
1.6.2.3


[-- Attachment #3: 0002-Fix-handling-of-SDP-records.patch --]
[-- Type: text/x-diff, Size: 1858 bytes --]

From 1b901dabde304237dc1ca05eb17a42137a650e16 Mon Sep 17 00:00:00 2001
From: Jaikumar Ganesh <jaikumar@google.com>
Date: Thu, 29 Oct 2009 13:24:05 -0700
Subject: [PATCH 2/2] Fix handling of SDP records.

We were deleting the SDP records only if a device
was registered for those UUIDs. With this change,
we delete the SDP records even if there are no
drivers registered.
---
 src/device.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/device.c b/src/device.c
index 5585271..760e661 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1136,8 +1136,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 		next = list->next;
 
 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
@@ -1150,24 +1148,27 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 								driver_data);
 			g_free(driver_data);
 
-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
+			break;
+		}
+	}
 
-			delete_record(srcaddr, dstaddr, rec->handle);
+	for (list = uuids; list; list = list->next) {
+		device->uuids = g_slist_remove(device->uuids, list->data);
 
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
+		sdp_record_t *rec;
+		rec = find_record_in_list(records, list->data);
+		if (!rec)
+			continue;
+
+		delete_record(srcaddr, dstaddr, rec->handle);
+
+		records = sdp_list_remove(records, rec);
+		sdp_record_free(rec);
 
-			break;
-		}
 	}
 
 	if (records)
 		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
-
-	for (list = uuids; list; list = list->next)
-		device->uuids = g_slist_remove(device->uuids, list->data);
 }
 
 static void services_changed(struct btd_device *device)
-- 
1.6.2.3

Re: [PATCH] Update SDP storage records when a record is deleted.

[Luiz Augusto von Dentz]

Hi,

They look so much better now :D

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] Update SDP storage records when a record is deleted.

[Jaikumar Ganesh]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Hi Johan:

On Thu, Oct 29, 2009 at 1:34 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> They look so much better now :D

Attached.


>
> --
> Luiz Augusto von Dentz
> Engenheiro de Computação
>

[-- Attachment #2: 0001-Fix-handling-of-SDP-records.patch --]
[-- Type: text/x-diff, Size: 1857 bytes --]

From ab5b4da387bdd1d3461a8a45d05a4c6a13157e6a Mon Sep 17 00:00:00 2001
From: Jaikumar Ganesh <jaikumar@google.com>
Date: Thu, 29 Oct 2009 14:01:36 -0700
Subject: [PATCH] Fix handling of SDP records.

We were deleting the SDP records only if a device was registered for
those UUIDs. With this change, we delete the SDP records even if there
are no drivers registered.
---
 src/device.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/device.c b/src/device.c
index 5585271..69a68db 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1136,8 +1136,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 		next = list->next;
 
 		for (uuid = driver->uuids; *uuid; uuid++) {
-			sdp_record_t *rec;
-
 			if (!g_slist_find_custom(uuids, *uuid,
 					(GCompareFunc) strcasecmp))
 				continue;
@@ -1150,24 +1148,28 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
 								driver_data);
 			g_free(driver_data);
 
-			rec = find_record_in_list(records, *uuid);
-			if (!rec)
-				break;
+			break;
+		}
+	}
 
-			delete_record(srcaddr, dstaddr, rec->handle);
+	for (list = uuids; list; list = list->next) {
+		sdp_record_t *rec;
 
-			records = sdp_list_remove(records, rec);
-			sdp_record_free(rec);
+		device->uuids = g_slist_remove(device->uuids, list->data);
+
+		rec = find_record_in_list(records, list->data);
+		if (!rec)
+			continue;
+
+		delete_record(srcaddr, dstaddr, rec->handle);
+
+		records = sdp_list_remove(records, rec);
+		sdp_record_free(rec);
 
-			break;
-		}
 	}
 
 	if (records)
 		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
-
-	for (list = uuids; list; list = list->next)
-		device->uuids = g_slist_remove(device->uuids, list->data);
 }
 
 static void services_changed(struct btd_device *device)
-- 
1.6.2.3

Re: [PATCH] Update SDP storage records when a record is deleted.

[Johan Hedberg]

Hi Jaikumar,

On Thu, Oct 29, 2009, Jaikumar Ganesh wrote:
> On Thu, Oct 29, 2009 at 1:34 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > Hi,
> >
> > They look so much better now :D
> 
> Attached.

Thanks. Both patches have now been pushed upstream.

Johan