2012-10-01 17:53:32

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 01/10] gdbus: Fix not freeing list node by using g_slist_delete_link

From: Luiz Augusto von Dentz <[email protected]>

g_slist_remove_link does not free the node which can cause leaks so
replace that with g_slist_delete_link which does free memory properly.
---
gdbus/watch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index a402ca9..07feb61 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -574,7 +574,7 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
continue;

remove_match(data);
- listeners = g_slist_remove_link(listeners, l);
+ listeners = g_slist_delete_link(listeners, l);

filter_data_free(data);
}
--
1.7.11.4



2012-10-04 08:42:58

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

Hi Marcel,

On Tue, Oct 2, 2012 at 4:57 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Lucas,
>
> On Tue, Oct 2, 2012 at 1:49 PM, Lucas De Marchi
> <[email protected]> wrote:
>> On Tue, Oct 2, 2012 at 5:05 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Lucas,
>>>
>>> On Tue, Oct 2, 2012 at 7:23 AM, Lucas De Marchi
>>> <[email protected]> wrote:
>>>> On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
>>>> <[email protected]> wrote:
>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>
>>>>> The connection is not really needed since the list of listeners is
>>>>> global not per connection, besides it is more convenient this way as
>>>>> only the id is needed.
>>>>> ---
>>>>> gdbus/gdbus.h | 2 +-
>>>>> gdbus/watch.c | 4 ++--
>>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>>>>> index 0a8a27c..3bd8986 100644
>>>>> --- a/gdbus/gdbus.h
>>>>> +++ b/gdbus/gdbus.h
>>>>> @@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>>>> const char *interface, const char *member,
>>>>> GDBusSignalFunction function, void *user_data,
>>>>> GDBusDestroyFunction destroy);
>>>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>>>>> +gboolean g_dbus_remove_watch(guint id);
>>>>> void g_dbus_remove_all_watches(DBusConnection *connection);
>>>>>
>>>>> #ifdef __cplusplus
>>>>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>>>>> index 07feb61..00cedae 100644
>>>>> --- a/gdbus/watch.c
>>>>> +++ b/gdbus/watch.c
>>>>> @@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
>>>>> g_free(l->data);
>>>>>
>>>>> g_slist_free(data->callbacks);
>>>>> - g_dbus_remove_watch(data->connection, data->name_watch);
>>>>> + g_dbus_remove_watch(data->name_watch);
>>>>> g_free(data->name);
>>>>> g_free(data->owner);
>>>>> g_free(data->path);
>>>>> @@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>>>> return cb->id;
>>>>> }
>>>>>
>>>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
>>>>> +gboolean g_dbus_remove_watch(guint id)
>>>>> {
>>>>> struct filter_data *data;
>>>>> struct filter_callback *cb;
>>>>> --
>>>>
>>>> This will lead to a broken build which is not nice to bisect. Any
>>>> other option besides adding with another name, converting everybody
>>>> and then renaming?
>>>>
>>>> The only other option I can think of is adding an ifdef and an option
>>>> to build-sys... this would avoid the final renaming.
>>>
>>> I guess this time it worth having the break of bisect in favor of a
>>> more convenient API, if you look at what the code is doing the
>>> connection is useless and normally we end up having to call the core
>>> to figure out a connection that is not used for anything.
>>
>> Ahn? did you read what I proposed as an option? I proposed splitting
>> the patch in 4 as opposed to 2 as you did.
>>
>> 0001) WARNING, whitespace error below
>>
>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>> index 0a8a27c..d254562 100644
>> --- a/gdbus/gdbus.h
>> +++ b/gdbus/gdbus.h
>> @@ -217,7 +217,12 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>> const char *interface, const char *member,
>> GDBusSignalFunction function, void *user_data,
>> GDBusDestroyFunction destroy);
>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>> +#ifdef _GDBUS_WITH_NEW_REMOVE_WATCH
>> +gboolean g_dbus_remove_watch(guint tag);
>> +#else
>> +gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>> +#endif
>> +
>> void g_dbus_remove_all_watches(DBusConnection *connection);
>>
>> #ifdef __cplusplus
>>
>>
>> 0002) Convert everybody adding the definition on Makefile.am
>>
>> 0003) Remove the define from Makefile.am
>>
>> 0004) Remove the old code from gdbus/
>
> That sounds overkill for so simple change, I will send updates to
> other projects using gdbus to see their reception, if they are
> receptive to the changes I think we can ignore the bisect problem.

Apparently you raised some concerns about this changes, but note that
this should not affect watches to different/private connections since
the watch itself carries a reference to the connection, in fact if you
look at the changes the connection given as parameter is completely
ignored, you could even pass NULL and it would still work.

--
Luiz Augusto von Dentz

2012-10-02 13:57:57

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

Hi Lucas,

On Tue, Oct 2, 2012 at 1:49 PM, Lucas De Marchi
<[email protected]> wrote:
> On Tue, Oct 2, 2012 at 5:05 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Lucas,
>>
>> On Tue, Oct 2, 2012 at 7:23 AM, Lucas De Marchi
>> <[email protected]> wrote:
>>> On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> The connection is not really needed since the list of listeners is
>>>> global not per connection, besides it is more convenient this way as
>>>> only the id is needed.
>>>> ---
>>>> gdbus/gdbus.h | 2 +-
>>>> gdbus/watch.c | 4 ++--
>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>>>> index 0a8a27c..3bd8986 100644
>>>> --- a/gdbus/gdbus.h
>>>> +++ b/gdbus/gdbus.h
>>>> @@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>>> const char *interface, const char *member,
>>>> GDBusSignalFunction function, void *user_data,
>>>> GDBusDestroyFunction destroy);
>>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>>>> +gboolean g_dbus_remove_watch(guint id);
>>>> void g_dbus_remove_all_watches(DBusConnection *connection);
>>>>
>>>> #ifdef __cplusplus
>>>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>>>> index 07feb61..00cedae 100644
>>>> --- a/gdbus/watch.c
>>>> +++ b/gdbus/watch.c
>>>> @@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
>>>> g_free(l->data);
>>>>
>>>> g_slist_free(data->callbacks);
>>>> - g_dbus_remove_watch(data->connection, data->name_watch);
>>>> + g_dbus_remove_watch(data->name_watch);
>>>> g_free(data->name);
>>>> g_free(data->owner);
>>>> g_free(data->path);
>>>> @@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>>> return cb->id;
>>>> }
>>>>
>>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
>>>> +gboolean g_dbus_remove_watch(guint id)
>>>> {
>>>> struct filter_data *data;
>>>> struct filter_callback *cb;
>>>> --
>>>
>>> This will lead to a broken build which is not nice to bisect. Any
>>> other option besides adding with another name, converting everybody
>>> and then renaming?
>>>
>>> The only other option I can think of is adding an ifdef and an option
>>> to build-sys... this would avoid the final renaming.
>>
>> I guess this time it worth having the break of bisect in favor of a
>> more convenient API, if you look at what the code is doing the
>> connection is useless and normally we end up having to call the core
>> to figure out a connection that is not used for anything.
>
> Ahn? did you read what I proposed as an option? I proposed splitting
> the patch in 4 as opposed to 2 as you did.
>
> 0001) WARNING, whitespace error below
>
> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
> index 0a8a27c..d254562 100644
> --- a/gdbus/gdbus.h
> +++ b/gdbus/gdbus.h
> @@ -217,7 +217,12 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
> const char *interface, const char *member,
> GDBusSignalFunction function, void *user_data,
> GDBusDestroyFunction destroy);
> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
> +#ifdef _GDBUS_WITH_NEW_REMOVE_WATCH
> +gboolean g_dbus_remove_watch(guint tag);
> +#else
> +gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
> +#endif
> +
> void g_dbus_remove_all_watches(DBusConnection *connection);
>
> #ifdef __cplusplus
>
>
> 0002) Convert everybody adding the definition on Makefile.am
>
> 0003) Remove the define from Makefile.am
>
> 0004) Remove the old code from gdbus/

That sounds overkill for so simple change, I will send updates to
other projects using gdbus to see their reception, if they are
receptive to the changes I think we can ignore the bisect problem.

--
Luiz Augusto von Dentz

2012-10-02 10:49:00

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

On Tue, Oct 2, 2012 at 5:05 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Lucas,
>
> On Tue, Oct 2, 2012 at 7:23 AM, Lucas De Marchi
> <[email protected]> wrote:
>> On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> The connection is not really needed since the list of listeners is
>>> global not per connection, besides it is more convenient this way as
>>> only the id is needed.
>>> ---
>>> gdbus/gdbus.h | 2 +-
>>> gdbus/watch.c | 4 ++--
>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>>> index 0a8a27c..3bd8986 100644
>>> --- a/gdbus/gdbus.h
>>> +++ b/gdbus/gdbus.h
>>> @@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>> const char *interface, const char *member,
>>> GDBusSignalFunction function, void *user_data,
>>> GDBusDestroyFunction destroy);
>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>>> +gboolean g_dbus_remove_watch(guint id);
>>> void g_dbus_remove_all_watches(DBusConnection *connection);
>>>
>>> #ifdef __cplusplus
>>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>>> index 07feb61..00cedae 100644
>>> --- a/gdbus/watch.c
>>> +++ b/gdbus/watch.c
>>> @@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
>>> g_free(l->data);
>>>
>>> g_slist_free(data->callbacks);
>>> - g_dbus_remove_watch(data->connection, data->name_watch);
>>> + g_dbus_remove_watch(data->name_watch);
>>> g_free(data->name);
>>> g_free(data->owner);
>>> g_free(data->path);
>>> @@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>>> return cb->id;
>>> }
>>>
>>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
>>> +gboolean g_dbus_remove_watch(guint id)
>>> {
>>> struct filter_data *data;
>>> struct filter_callback *cb;
>>> --
>>
>> This will lead to a broken build which is not nice to bisect. Any
>> other option besides adding with another name, converting everybody
>> and then renaming?
>>
>> The only other option I can think of is adding an ifdef and an option
>> to build-sys... this would avoid the final renaming.
>
> I guess this time it worth having the break of bisect in favor of a
> more convenient API, if you look at what the code is doing the
> connection is useless and normally we end up having to call the core
> to figure out a connection that is not used for anything.

Ahn? did you read what I proposed as an option? I proposed splitting
the patch in 4 as opposed to 2 as you did.

0001) WARNING, whitespace error below

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 0a8a27c..d254562 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -217,7 +217,12 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
const char *interface, const char *member,
GDBusSignalFunction function, void *user_data,
GDBusDestroyFunction destroy);
-gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
+#ifdef _GDBUS_WITH_NEW_REMOVE_WATCH
+gboolean g_dbus_remove_watch(guint tag);
+#else
+gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
+#endif
+
void g_dbus_remove_all_watches(DBusConnection *connection);

#ifdef __cplusplus


0002) Convert everybody adding the definition on Makefile.am

0003) Remove the define from Makefile.am

0004) Remove the old code from gdbus/


Lucas De Marchi

2012-10-02 08:05:32

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

Hi Lucas,

On Tue, Oct 2, 2012 at 7:23 AM, Lucas De Marchi
<[email protected]> wrote:
> On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> The connection is not really needed since the list of listeners is
>> global not per connection, besides it is more convenient this way as
>> only the id is needed.
>> ---
>> gdbus/gdbus.h | 2 +-
>> gdbus/watch.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
>> index 0a8a27c..3bd8986 100644
>> --- a/gdbus/gdbus.h
>> +++ b/gdbus/gdbus.h
>> @@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>> const char *interface, const char *member,
>> GDBusSignalFunction function, void *user_data,
>> GDBusDestroyFunction destroy);
>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
>> +gboolean g_dbus_remove_watch(guint id);
>> void g_dbus_remove_all_watches(DBusConnection *connection);
>>
>> #ifdef __cplusplus
>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>> index 07feb61..00cedae 100644
>> --- a/gdbus/watch.c
>> +++ b/gdbus/watch.c
>> @@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
>> g_free(l->data);
>>
>> g_slist_free(data->callbacks);
>> - g_dbus_remove_watch(data->connection, data->name_watch);
>> + g_dbus_remove_watch(data->name_watch);
>> g_free(data->name);
>> g_free(data->owner);
>> g_free(data->path);
>> @@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
>> return cb->id;
>> }
>>
>> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
>> +gboolean g_dbus_remove_watch(guint id)
>> {
>> struct filter_data *data;
>> struct filter_callback *cb;
>> --
>
> This will lead to a broken build which is not nice to bisect. Any
> other option besides adding with another name, converting everybody
> and then renaming?
>
> The only other option I can think of is adding an ifdef and an option
> to build-sys... this would avoid the final renaming.

I guess this time it worth having the break of bisect in favor of a
more convenient API, if you look at what the code is doing the
connection is useless and normally we end up having to call the core
to figure out a connection that is not used for anything.

--
Luiz Augusto von Dentz

2012-10-02 04:23:48

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> The connection is not really needed since the list of listeners is
> global not per connection, besides it is more convenient this way as
> only the id is needed.
> ---
> gdbus/gdbus.h | 2 +-
> gdbus/watch.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
> index 0a8a27c..3bd8986 100644
> --- a/gdbus/gdbus.h
> +++ b/gdbus/gdbus.h
> @@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
> const char *interface, const char *member,
> GDBusSignalFunction function, void *user_data,
> GDBusDestroyFunction destroy);
> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
> +gboolean g_dbus_remove_watch(guint id);
> void g_dbus_remove_all_watches(DBusConnection *connection);
>
> #ifdef __cplusplus
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index 07feb61..00cedae 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
> g_free(l->data);
>
> g_slist_free(data->callbacks);
> - g_dbus_remove_watch(data->connection, data->name_watch);
> + g_dbus_remove_watch(data->name_watch);
> g_free(data->name);
> g_free(data->owner);
> g_free(data->path);
> @@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
> return cb->id;
> }
>
> -gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
> +gboolean g_dbus_remove_watch(guint id)
> {
> struct filter_data *data;
> struct filter_callback *cb;
> --

This will lead to a broken build which is not nice to bisect. Any
other option besides adding with another name, converting everybody
and then renaming?

The only other option I can think of is adding an ifdef and an option
to build-sys... this would avoid the final renaming.


Lucas De Marchi

2012-10-02 04:10:28

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/10] gdbus: Fix not freeing list node by using g_slist_delete_link

On Mon, Oct 1, 2012 at 2:53 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> g_slist_remove_link does not free the node which can cause leaks so
> replace that with g_slist_delete_link which does free memory properly.
> ---
> gdbus/watch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index a402ca9..07feb61 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -574,7 +574,7 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
> continue;
>
> remove_match(data);
> - listeners = g_slist_remove_link(listeners, l);
> + listeners = g_slist_delete_link(listeners, l);
>
> filter_data_free(data);
> }
> --

Ack,

Lucas De Marchi

2012-10-01 17:53:41

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 10/10] AVRCP: Don't respond with errors when no player is registered

From: Luiz Augusto von Dentz <[email protected]>

Some devices w.g. Sony MW600 will stop using certain commands if an
error happen, so the code now just fake a player and once a real
player is registered it takes place of the fake one.
---
audio/avrcp.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 63 insertions(+), 14 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 21d105a..6b426f5 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -477,6 +477,17 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
return;
}

+static void *player_get_metadata(struct avrcp_player *player, uint32_t attr)
+{
+ if (player != NULL)
+ return player->cb->get_metadata(attr, player->user_data);
+
+ if (attr == AVRCP_MEDIA_ATTRIBUTE_TITLE)
+ return "";
+
+ return NULL;
+}
+
static uint16_t player_write_media_attribute(struct avrcp_player *player,
uint32_t id, uint8_t *buf,
uint16_t *pos,
@@ -489,7 +500,7 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,

DBG("%u", id);

- value = player->cb->get_metadata(id, player->user_data);
+ value = player_get_metadata(player, id);
if (value == NULL) {
*offset = 0;
return 0;
@@ -588,6 +599,9 @@ static int player_set_attribute(struct avrcp_player *player,
{
DBG("Change attribute: %u %u", attr, val);

+ if (player == NULL)
+ return -ENOENT;
+
return player->cb->set_setting(attr, val, player->user_data);
}

@@ -597,6 +611,9 @@ static int player_get_attribute(struct avrcp_player *player, uint8_t attr)

DBG("attr %u", attr);

+ if (player == NULL)
+ return -ENOENT;
+
value = player->cb->get_setting(attr, player->user_data);
if (value < 0)
DBG("attr %u not supported by player", attr);
@@ -653,7 +670,7 @@ static uint8_t avrcp_handle_list_player_attributes(struct avrcp *session,
uint16_t len = ntohs(pdu->params_len);
unsigned int i;

- if (len != 0 || player == NULL) {
+ if (len != 0) {
pdu->params_len = htons(1);
pdu->params[0] = AVRCP_STATUS_INVALID_PARAM;
return AVC_CTYPE_REJECTED;
@@ -685,7 +702,7 @@ static uint8_t avrcp_handle_list_player_values(struct avrcp *session,
uint16_t len = ntohs(pdu->params_len);
unsigned int i;

- if (len != 1 || player == NULL)
+ if (len != 1)
goto err;

if (player_get_attribute(player, pdu->params[0]) < 0)
@@ -707,6 +724,15 @@ err:
return AVC_CTYPE_REJECTED;
}

+static GList *player_list_metadata(struct avrcp_player *player)
+{
+ if (player != NULL)
+ return player->cb->list_metadata(player->user_data);
+
+ return g_list_prepend(NULL,
+ GUINT_TO_POINTER(AVRCP_MEDIA_ATTRIBUTE_TITLE));
+}
+
static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -719,7 +745,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
GList *attr_ids;
uint16_t offset;

- if (len < 9 || identifier != 0 || player == NULL)
+ if (len < 9 || identifier != 0)
goto err;

nattr = pdu->params[8];
@@ -732,7 +758,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
* Return all available information, at least
* title must be returned if there's a track selected.
*/
- attr_ids = player->cb->list_metadata(player->user_data);
+ attr_ids = player_list_metadata(player);
len = g_list_length(attr_ids);
} else {
unsigned int i;
@@ -788,8 +814,7 @@ static uint8_t avrcp_handle_get_current_player_value(struct avrcp *session,
uint8_t *settings;
unsigned int i;

- if (player == NULL || len <= 1 || pdu->params[0] != len - 1 ||
- player == NULL)
+ if (len <= 1 || pdu->params[0] != len - 1)
goto err;

/*
@@ -922,6 +947,14 @@ err:
return AVC_CTYPE_REJECTED;
}

+static uint32_t player_get_position(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return 0;
+
+ return player->cb->get_position(player->user_data);
+}
+
static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -932,15 +965,15 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
uint32_t duration;
void *pduration;

- if (len != 0 || player == NULL) {
+ if (len != 0) {
pdu->params_len = htons(1);
pdu->params[0] = AVRCP_STATUS_INVALID_PARAM;
return AVC_CTYPE_REJECTED;
}

- position = player->cb->get_position(player->user_data);
- pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
- player->user_data);
+ position = player_get_position(player);
+ pduration = player_get_metadata(player,
+ AVRCP_MEDIA_ATTRIBUTE_DURATION);
if (pduration != NULL)
duration = htonl(GPOINTER_TO_UINT(pduration));
else
@@ -957,6 +990,22 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
return AVC_CTYPE_STABLE;
}

+static uint64_t player_get_uid(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return UINT64_MAX;
+
+ return player->cb->get_status(player->user_data);
+}
+
+static uint8_t player_get_status(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return AVRCP_PLAY_STATUS_STOPPED;
+
+ return player->cb->get_status(player->user_data);
+}
+
static uint8_t avrcp_handle_register_notification(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -970,18 +1019,18 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
* one is applicable only for EVENT_PLAYBACK_POS_CHANGED. See AVRCP
* 1.3 spec, section 5.4.2.
*/
- if (len != 5 || player == NULL)
+ if (len != 5)
goto err;

switch (pdu->params[0]) {
case AVRCP_EVENT_STATUS_CHANGED:
len = 2;
- pdu->params[1] = player->cb->get_status(player->user_data);
+ pdu->params[1] = player_get_status(player);

break;
case AVRCP_EVENT_TRACK_CHANGED:
len = 9;
- uid = player->cb->get_uid(player->user_data);
+ uid = player_get_uid(player);
memcpy(&pdu->params[1], &uid, sizeof(uint64_t));

break;
--
1.7.11.4


2012-10-01 17:53:40

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 09/10] AVRCP: Register to AVCTP state changes without depending on player

From: Luiz Augusto von Dentz <[email protected]>

It is not longer necessary to have a player to be able to register the
extra pdu handlers.
---
audio/avrcp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 2e36fb7..21d105a 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1467,6 +1467,9 @@ int avrcp_register(const bdaddr_t *src, GKeyFile *config)

servers = g_slist_append(servers, server);

+ if (!avctp_id)
+ avctp_id = avctp_add_state_cb(state_changed, NULL);
+
return 0;
}

@@ -1527,9 +1530,6 @@ struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
player->user_data = user_data;
player->destroy = destroy;

- if (!avctp_id)
- avctp_id = avctp_add_state_cb(state_changed, NULL);
-
server->players = g_slist_append(server->players, player);

/* Assign player to session without current player */
--
1.7.11.4


2012-10-01 17:53:39

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 08/10] AVCTP: Allocate memory to hold incoming/outgoing PDUs

From: Luiz Augusto von Dentz <[email protected]>

This makes possible to the handler to respond asyncronous as the memory
remains valid after it returns.

In addition to that it uses the MTU to calculate the buffer size
necessary.
---
audio/avctp.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/audio/avctp.c b/audio/avctp.c
index b77a61b..d715cbf 100644
--- a/audio/avctp.c
+++ b/audio/avctp.c
@@ -134,7 +134,9 @@ struct avctp_rsp_handler {
struct avctp_channel {
GIOChannel *io;
guint watch;
- uint16_t mtu;
+ uint16_t imtu;
+ uint16_t omtu;
+ uint8_t *buffer;
};

struct avctp {
@@ -343,6 +345,7 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
if (chan->watch)
g_source_remove(chan->watch);

+ g_free(chan->buffer);
g_free(chan);
}

@@ -446,7 +449,9 @@ static gboolean session_browsing_cb(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
struct avctp *session = data;
- uint8_t buf[1024], *operands;
+ struct avctp_channel *browsing = session->browsing;
+ uint8_t *buf = browsing->buffer;
+ uint8_t *operands;
struct avctp_header *avctp;
int sock, ret, packet_size, operand_count;

@@ -455,7 +460,7 @@ static gboolean session_browsing_cb(GIOChannel *chan, GIOCondition cond,

sock = g_io_channel_unix_get_fd(chan);

- ret = read(sock, buf, sizeof(buf));
+ ret = read(sock, buf, sizeof(browsing->imtu));
if (ret <= 0)
goto failed;

@@ -496,7 +501,9 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
gpointer data)
{
struct avctp *session = data;
- uint8_t buf[1024], *operands, code, subunit;
+ struct avctp_channel *control = session->control;
+ uint8_t *buf = control->buffer;
+ uint8_t *operands, code, subunit;
struct avctp_header *avctp;
struct avc_header *avc;
int ret, packet_size, operand_count, sock;
@@ -507,7 +514,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,

sock = g_io_channel_unix_get_fd(chan);

- ret = read(sock, buf, sizeof(buf));
+ ret = read(sock, buf, control->imtu);
if (ret <= 0)
goto failed;

@@ -688,7 +695,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
{
struct avctp *session = data;
char address[18];
- uint16_t imtu;
+ uint16_t imtu, omtu;
GError *gerr = NULL;

if (err) {
@@ -699,6 +706,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
bt_io_get(chan, &gerr,
BT_IO_OPT_DEST, &address,
BT_IO_OPT_IMTU, &imtu,
+ BT_IO_OPT_OMTU, &omtu,
BT_IO_OPT_INVALID);
if (gerr) {
error("%s", gerr->message);
@@ -713,7 +721,9 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
if (session->browsing == NULL)
session->browsing = avctp_channel_create(chan);

- session->browsing->mtu = imtu;
+ session->browsing->imtu = imtu;
+ session->browsing->omtu = omtu;
+ session->browsing->buffer = g_malloc0(MAX(imtu, omtu));
session->browsing->watch = g_io_add_watch(session->browsing->io,
G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
(GIOFunc) session_browsing_cb, session);
@@ -731,7 +741,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
{
struct avctp *session = data;
char address[18];
- uint16_t imtu;
+ uint16_t imtu, omtu;
GError *gerr = NULL;

if (err) {
@@ -743,6 +753,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
bt_io_get(chan, &gerr,
BT_IO_OPT_DEST, &address,
BT_IO_OPT_IMTU, &imtu,
+ BT_IO_OPT_IMTU, &omtu,
BT_IO_OPT_INVALID);
if (gerr) {
avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
@@ -756,7 +767,9 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
if (session->control == NULL)
session->control = avctp_channel_create(chan);

- session->control->mtu = imtu;
+ session->control->imtu = imtu;
+ session->control->omtu = omtu;
+ session->control->buffer = g_malloc0(MAX(imtu, omtu));
session->control->watch = g_io_add_watch(session->control->io,
G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
(GIOFunc) session_cb, session);
--
1.7.11.4


2012-10-01 17:53:38

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 07/10] AVCTP: Simplify channel handling

From: Luiz Augusto von Dentz <[email protected]>

Make both control and browsing channels to use the same structure to
store its channel information.
---
audio/avctp.c | 155 +++++++++++++++++++++++++++++++---------------------------
1 file changed, 82 insertions(+), 73 deletions(-)

diff --git a/audio/avctp.c b/audio/avctp.c
index b7b3083..b77a61b 100644
--- a/audio/avctp.c
+++ b/audio/avctp.c
@@ -131,6 +131,12 @@ struct avctp_rsp_handler {
void *user_data;
};

+struct avctp_channel {
+ GIOChannel *io;
+ guint watch;
+ uint16_t mtu;
+};
+
struct avctp {
struct avctp_server *server;
bdaddr_t dst;
@@ -139,14 +145,10 @@ struct avctp {

int uinput;

- GIOChannel *control_io;
- GIOChannel *browsing_io;
- guint control_io_id;
- guint browsing_io_id;
guint auth_id;

- uint16_t control_mtu;
- uint16_t browsing_mtu;
+ struct avctp_channel *control;
+ struct avctp_channel *browsing;

uint8_t key_quirks[256];
GSList *handlers;
@@ -333,6 +335,17 @@ static struct avctp_pdu_handler *find_handler(GSList *list, uint8_t opcode)
return NULL;
}

+static void avctp_channel_destroy(struct avctp_channel *chan)
+{
+ g_io_channel_shutdown(chan->io, TRUE, NULL);
+ g_io_channel_unref(chan->io);
+
+ if (chan->watch)
+ g_source_remove(chan->watch);
+
+ g_free(chan);
+}
+
static void avctp_disconnected(struct avctp *session)
{
struct avctp_server *server;
@@ -340,31 +353,15 @@ static void avctp_disconnected(struct avctp *session)
if (!session)
return;

- if (session->browsing_io) {
- g_io_channel_shutdown(session->browsing_io, TRUE, NULL);
- g_io_channel_unref(session->browsing_io);
- session->browsing_io = NULL;
- }
-
- if (session->browsing_io_id) {
- g_source_remove(session->browsing_io_id);
- session->browsing_io_id = 0;
- }
-
- if (session->control_io) {
- g_io_channel_shutdown(session->control_io, TRUE, NULL);
- g_io_channel_unref(session->control_io);
- session->control_io = NULL;
- }
+ if (session->browsing)
+ avctp_channel_destroy(session->browsing);

- if (session->control_io_id) {
- g_source_remove(session->control_io_id);
- session->control_io_id = 0;
+ if (session->control)
+ avctp_channel_destroy(session->control);

- if (session->auth_id != 0) {
- btd_cancel_authorization(session->auth_id);
- session->auth_id = 0;
- }
+ if (session->auth_id != 0) {
+ btd_cancel_authorization(session->auth_id);
+ session->auth_id = 0;
}

if (session->uinput >= 0) {
@@ -420,7 +417,7 @@ static void avctp_set_state(struct avctp *session, avctp_state_t new_state)
}
}

-static void handle_response(struct avctp *session, struct avctp_header *avctp,
+static void control_response(struct avctp *session, struct avctp_header *avctp,
struct avc_header *avc, uint8_t *operands,
size_t operand_count)
{
@@ -456,7 +453,7 @@ static gboolean session_browsing_cb(GIOChannel *chan, GIOCondition cond,
if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
goto failed;

- sock = g_io_channel_unix_get_fd(session->browsing_io);
+ sock = g_io_channel_unix_get_fd(chan);

ret = read(sock, buf, sizeof(buf));
if (ret <= 0)
@@ -508,7 +505,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
goto failed;

- sock = g_io_channel_unix_get_fd(session->control_io);
+ sock = g_io_channel_unix_get_fd(chan);

ret = read(sock, buf, sizeof(buf));
if (ret <= 0)
@@ -548,7 +545,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
avc->opcode, operand_count);

if (avctp->cr == AVCTP_RESPONSE) {
- handle_response(session, avctp, avc, operands, operand_count);
+ control_response(session, avctp, avc, operands, operand_count);
return TRUE;
}

@@ -676,6 +673,16 @@ static void init_uinput(struct avctp *session)
DBG("AVRCP: uinput initialized for %s", address);
}

+static struct avctp_channel *avctp_channel_create(GIOChannel *io)
+{
+ struct avctp_channel *chan;
+
+ chan = g_new0(struct avctp_channel, 1);
+ chan->io = g_io_channel_ref(io);
+
+ return chan;
+}
+
static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
gpointer data)
{
@@ -686,10 +693,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,

if (err) {
error("Browsing: %s", err->message);
- g_io_channel_shutdown(chan, TRUE, NULL);
- g_io_channel_unref(chan);
- session->browsing_io = NULL;
- return;
+ goto fail;
}

bt_io_get(chan, &gerr,
@@ -700,20 +704,28 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
error("%s", gerr->message);
g_io_channel_shutdown(chan, TRUE, NULL);
g_io_channel_unref(chan);
- session->browsing_io = NULL;
g_error_free(gerr);
- return;
+ goto fail;
}

DBG("AVCTP Browsing: connected to %s", address);

- if (!session->browsing_io)
- session->browsing_io = g_io_channel_ref(chan);
+ if (session->browsing == NULL)
+ session->browsing = avctp_channel_create(chan);

- session->browsing_mtu = imtu;
- session->browsing_io_id = g_io_add_watch(chan,
+ session->browsing->mtu = imtu;
+ session->browsing->watch = g_io_add_watch(session->browsing->io,
G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
- (GIOFunc) session_browsing_cb, session); }
+ (GIOFunc) session_browsing_cb, session);
+
+ return;
+
+fail:
+ if (session->browsing) {
+ avctp_channel_destroy(session->browsing);
+ session->browsing = NULL;
+ }
+}

static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
{
@@ -741,16 +753,17 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)

DBG("AVCTP: connected to %s", address);

- if (!session->control_io)
- session->control_io = g_io_channel_ref(chan);
+ if (session->control == NULL)
+ session->control = avctp_channel_create(chan);
+
+ session->control->mtu = imtu;
+ session->control->watch = g_io_add_watch(session->control->io,
+ G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+ (GIOFunc) session_cb, session);

init_uinput(session);

avctp_set_state(session, AVCTP_STATE_CONNECTED);
- session->control_mtu = imtu;
- session->control_io_id = g_io_add_watch(chan,
- G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
- (GIOFunc) session_cb, session);
}

static void auth_cb(DBusError *derr, void *user_data)
@@ -760,9 +773,9 @@ static void auth_cb(DBusError *derr, void *user_data)

session->auth_id = 0;

- if (session->control_io_id) {
- g_source_remove(session->control_io_id);
- session->control_io_id = 0;
+ if (session->control->watch > 0) {
+ g_source_remove(session->control->watch);
+ session->control->watch = 0;
}

if (derr && dbus_error_is_set(derr)) {
@@ -771,7 +784,7 @@ static void auth_cb(DBusError *derr, void *user_data)
return;
}

- if (!bt_io_accept(session->control_io, avctp_connect_cb, session,
+ if (!bt_io_accept(session->control->io, avctp_connect_cb, session,
NULL, &err)) {
error("bt_io_accept: %s", err->message);
g_error_free(err);
@@ -836,14 +849,14 @@ static struct avctp *avctp_get_internal(const bdaddr_t *src,
static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
struct audio_device *dev)
{
- if (session->control_io) {
+ if (session->control != NULL) {
error("Control: Refusing unexpected connect");
g_io_channel_shutdown(chan, TRUE, NULL);
return;
}

avctp_set_state(session, AVCTP_STATE_CONNECTING);
- session->control_io = g_io_channel_ref(chan);
+ session->control = avctp_channel_create(chan);

session->auth_id = btd_request_authorization(&dev->src, &dev->dst,
AVRCP_TARGET_UUID,
@@ -851,7 +864,7 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
if (session->auth_id == 0)
goto drop;

- session->control_io_id = g_io_add_watch(chan, G_IO_ERR | G_IO_HUP |
+ session->control->watch = g_io_add_watch(chan, G_IO_ERR | G_IO_HUP |
G_IO_NVAL, session_cb, session);
return;

@@ -864,7 +877,7 @@ static void avctp_browsing_confirm(struct avctp *session, GIOChannel *chan,
{
GError *err = NULL;

- if (session->control_io == NULL || session->browsing_io) {
+ if (session->control == NULL || session->browsing != NULL) {
error("Browsing: Refusing unexpected connect");
g_io_channel_shutdown(chan, TRUE, NULL);
return;
@@ -911,8 +924,8 @@ static void avctp_confirm_cb(GIOChannel *chan, gpointer data)
DBG("AVCTP: incoming connect from %s", address);

session = avctp_get_internal(&src, &dst);
- if (!session)
- goto drop;
+ if (session == NULL)
+ return;

dev = manager_get_device(&src, &dst, FALSE);
if (!dev) {
@@ -942,13 +955,7 @@ static void avctp_confirm_cb(GIOChannel *chan, gpointer data)
return;

drop:
- if (session && session->browsing_io)
- g_io_channel_unref(session->browsing_io);
-
- if (session && session->control_io)
- g_io_channel_unref(session->control_io);
-
- if (session && psm == AVCTP_CONTROL_PSM)
+ if (psm == AVCTP_CONTROL_PSM)
avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
}

@@ -1086,7 +1093,7 @@ int avctp_send_passthrough(struct avctp *session, uint8_t op)
operands[0] = op & 0x7f;
operands[1] = 0;

- sk = g_io_channel_unix_get_fd(session->control_io);
+ sk = g_io_channel_unix_get_fd(session->control->io);

if (write(sk, buf, sizeof(buf)) < 0)
return -errno;
@@ -1115,7 +1122,7 @@ static int avctp_send(struct avctp *session, uint8_t transaction, uint8_t cr,
if (session->state != AVCTP_STATE_CONNECTED)
return -ENOTCONN;

- sk = g_io_channel_unix_get_fd(session->control_io);
+ sk = g_io_channel_unix_get_fd(session->control->io);

memset(buf, 0, sizeof(buf));

@@ -1299,7 +1306,8 @@ struct avctp *avctp_connect(const bdaddr_t *src, const bdaddr_t *dst)
return NULL;
}

- session->control_io = io;
+ session->control = avctp_channel_create(io);
+ g_io_channel_unref(io);

return session;
}
@@ -1312,7 +1320,7 @@ int avctp_connect_browsing(struct avctp *session)
if (session->state != AVCTP_STATE_CONNECTED)
return -ENOTCONN;

- if (session->browsing_io != NULL)
+ if (session->browsing != NULL)
return 0;

io = bt_io_connect(avctp_connect_browsing_cb, session, NULL, &err,
@@ -1327,14 +1335,15 @@ int avctp_connect_browsing(struct avctp *session)
return -EIO;
}

- session->browsing_io = io;
+ session->browsing = avctp_channel_create(io);
+ g_io_channel_unref(io);

return 0;
}

void avctp_disconnect(struct avctp *session)
{
- if (!session->control_io)
+ if (session->state == AVCTP_STATE_DISCONNECTED)
return;

avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
--
1.7.11.4


2012-10-01 17:53:37

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 06/10] AVRCP: Fix not removing session from player upon disconnect

From: Luiz Augusto von Dentz <[email protected]>

Invalid read of size 8
at 0x1310E3: avrcp_unregister_player (avrcp.c:1604)
by 0x13EB57: path_free (media.c:1834)
by 0x123208: remove_interface.isra.1 (object.c:558)
by 0x1238DD: g_dbus_unregister_interface (object.c:705)
by 0x124BB8: media_server_remove (manager.c:1077)
by 0x4E91C5C: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x17B349: adapter_remove (adapter.c:2309)
by 0x176F39: manager_cleanup (manager.c:290)
by 0x120E65: main (main.c:555)
Address 0x6685058 is 24 bytes inside a block of size 80 free'd
at 0x4C279AE: free (vg_replace_malloc.c:427)
by 0x4E7C50E: g_free (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x12FC97: state_changed (avrcp.c:1380)
by 0x12D351: avctp_set_state (avctp.c:396)
by 0x12D7B4: session_cb (avctp.c:601)
by 0x4E76824: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4E76B57: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4E76F51: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x120E51: main (main.c:551)
---
audio/avrcp.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 49f1550..2e36fb7 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1328,6 +1328,11 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
avctp_unregister_browsing_pdu_handler(
session->browsing_handler);

+ if (session->player != NULL)
+ session->player->sessions = g_slist_remove(
+ session->player->sessions,
+ session);
+
g_free(session);
break;
case AVCTP_STATE_CONNECTING:
--
1.7.11.4


2012-10-01 17:53:36

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 05/10] AVRCP: Fix not freeing player session list on exit

From: Luiz Augusto von Dentz <[email protected]>

16 bytes in 1 blocks are definitely lost in loss record 111 of 359
at 0x4A0884D: malloc (vg_replace_malloc.c:263)
by 0x4C8026E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C94671: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C959B2: g_slist_append (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x130FCC: avrcp_register_player (avrcp.c:1584)
by 0x13FA1F: register_player (media.c:1689)
by 0x123100: process_message.isra.0 (object.c:197)
by 0x4F70684: ??? (in /usr/lib64/libdbus-1.so.3.5.6)
by 0x4F6290C: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.5.6)
by 0x121747: message_dispatch (mainloop.c:76)
by 0x4C7B22A: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C7A694: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3200.4)
---
audio/avrcp.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 376f4a1..49f1550 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1472,6 +1472,7 @@ static void player_destroy(gpointer data)
if (player->destroy)
player->destroy(player->user_data);

+ g_slist_free(player->sessions);
g_free(player);
}

--
1.7.11.4


2012-10-01 17:53:35

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 04/10] audio: Fix not freeing gateway agent data on exit

From: Luiz Augusto von Dentz <[email protected]>

434 (168 direct, 266 indirect) bytes in 7 blocks are definitely lost in loss record 322 of 338
at 0x4A06F18: calloc (vg_replace_malloc.c:566)
by 0x4C802C6: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x126C0C: register_agent (gateway.c:673)
by 0x122960: process_message.isra.0 (object.c:197)
by 0x4F70684: ??? (in /usr/lib64/libdbus-1.so.3.5.6)
by 0x4F6290C: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.5.6)
by 0x120FA7: message_dispatch (mainloop.c:76)
by 0x4C7B22A: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C7A694: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C7A9C7: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x4C7ADC1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3200.4)
by 0x120671: main (main.c:551)
---
audio/gateway.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index fe17284..cab32a1 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -110,6 +110,8 @@ static void agent_free(struct hf_agent *agent)
if (!agent)
return;

+ g_dbus_remove_watch(agent->watch);
+
g_free(agent->name);
g_free(agent->path);
g_free(agent);
@@ -704,8 +706,6 @@ static DBusMessage *unregister_agent(DBusConnection *conn,
if (strcmp(gw->agent->path, path) != 0)
return btd_error_does_not_exist(msg);

- g_dbus_remove_watch(gw->agent->watch);
-
agent_free(gw->agent);
gw->agent = NULL;

@@ -741,6 +741,9 @@ static void path_unregister(void *data)

gateway_close(dev);

+ if (dev->gateway->agent)
+ agent_free(dev->gateway->agent);
+
g_free(dev->gateway);
dev->gateway = NULL;
}
--
1.7.11.4


2012-10-01 17:53:34

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 03/10] Fix passing D-Bus connection to g_dbus_remove_watch

From: Luiz Augusto von Dentz <[email protected]>

All it needs now is the id
---
attrib/client.c | 4 ++--
audio/gateway.c | 2 +-
audio/media.c | 9 ++++-----
audio/telephony-maemo6.c | 2 +-
audio/telephony-ofono.c | 4 ++--
audio/transport.c | 2 +-
plugins/service.c | 4 ++--
profiles/health/hdp_util.c | 3 +--
profiles/heartrate/heartrate.c | 6 +++---
profiles/network/connection.c | 4 ++--
profiles/network/server.c | 2 +-
profiles/thermometer/thermometer.c | 6 +++---
src/adapter.c | 2 +-
src/agent.c | 3 +--
src/device.c | 6 ++----
src/profile.c | 4 ++--
16 files changed, 29 insertions(+), 34 deletions(-)

diff --git a/attrib/client.c b/attrib/client.c
index ece16bf..c5862fa 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -294,7 +294,7 @@ static void watcher_exit(DBusConnection *conn, void *user_data)
DBG("%s watcher %s exited", gatt->path, watcher->name);

gatt->watchers = g_slist_remove(gatt->watchers, watcher);
- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);
remove_attio(gatt);
}

@@ -500,7 +500,7 @@ static DBusMessage *unregister_watcher(DBusConnection *conn,

watcher = l->data;
gatt->watchers = g_slist_remove(gatt->watchers, watcher);
- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);
remove_attio(gatt);

return dbus_message_new_method_return(msg);
diff --git a/audio/gateway.c b/audio/gateway.c
index 45b25a1..fe17284 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -704,7 +704,7 @@ static DBusMessage *unregister_agent(DBusConnection *conn,
if (strcmp(gw->agent->path, path) != 0)
return btd_error_does_not_exist(msg);

- g_dbus_remove_watch(conn, gw->agent->watch);
+ g_dbus_remove_watch(gw->agent->watch);

agent_free(gw->agent);
gw->agent = NULL;
diff --git a/audio/media.c b/audio/media.c
index c88afc0..5c26035 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -159,7 +159,7 @@ static void media_endpoint_destroy(struct media_endpoint *endpoint)
g_slist_free_full(endpoint->transports,
(GDestroyNotify) media_transport_destroy);

- g_dbus_remove_watch(btd_get_dbus_connection(), endpoint->watch);
+ g_dbus_remove_watch(endpoint->watch);
g_free(endpoint->capabilities);
g_free(endpoint->sender);
g_free(endpoint->path);
@@ -966,7 +966,6 @@ static void release_player(struct media_player *mp)

static void media_player_free(gpointer data)
{
- DBusConnection *conn = btd_get_dbus_connection();
struct media_player *mp = data;
struct media_adapter *adapter = mp->adapter;

@@ -975,9 +974,9 @@ static void media_player_free(gpointer data)
release_player(mp);
}

- g_dbus_remove_watch(conn, mp->watch);
- g_dbus_remove_watch(conn, mp->property_watch);
- g_dbus_remove_watch(conn, mp->track_watch);
+ g_dbus_remove_watch(mp->watch);
+ g_dbus_remove_watch(mp->property_watch);
+ g_dbus_remove_watch(mp->track_watch);

if (mp->track)
g_hash_table_unref(mp->track);
diff --git a/audio/telephony-maemo6.c b/audio/telephony-maemo6.c
index d000a2a..ef997ca 100644
--- a/audio/telephony-maemo6.c
+++ b/audio/telephony-maemo6.c
@@ -2167,7 +2167,7 @@ int telephony_init(void)

static void remove_watch(gpointer data)
{
- g_dbus_remove_watch(btd_get_dbus_connection(), GPOINTER_TO_UINT(data));
+ g_dbus_remove_watch(GPOINTER_TO_UINT(data));
}

void telephony_exit(void)
diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
index f962c7e..f87b652 100644
--- a/audio/telephony-ofono.c
+++ b/audio/telephony-ofono.c
@@ -657,7 +657,7 @@ static void call_free(void *data)
if (vc->status == CALL_STATUS_INCOMING)
telephony_calling_stopped_ind();

- g_dbus_remove_watch(btd_get_dbus_connection(), vc->watch);
+ g_dbus_remove_watch(vc->watch);
g_free(vc->obj_path);
g_free(vc->number);
g_free(vc);
@@ -1603,7 +1603,7 @@ int telephony_init(void)

static void remove_watch(gpointer data)
{
- g_dbus_remove_watch(btd_get_dbus_connection(), GPOINTER_TO_UINT(data));
+ g_dbus_remove_watch(GPOINTER_TO_UINT(data));
}

static void pending_free(void *data)
diff --git a/audio/transport.c b/audio/transport.c
index daafff8..4615280 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -320,7 +320,7 @@ static void media_transport_remove(struct media_transport *transport,
transport->owners = g_slist_remove(transport->owners, owner);

if (owner->watch)
- g_dbus_remove_watch(btd_get_dbus_connection(), owner->watch);
+ g_dbus_remove_watch(owner->watch);

media_owner_free(owner);

diff --git a/plugins/service.c b/plugins/service.c
index 45886ac..f77dd47 100644
--- a/plugins/service.c
+++ b/plugins/service.c
@@ -457,7 +457,7 @@ static int remove_record(const char *sender,

DBG("listner_id %d", user_record->listener_id);

- g_dbus_remove_watch(conn, user_record->listener_id);
+ g_dbus_remove_watch(user_record->listener_id);

exit_callback(conn, user_record);

@@ -724,7 +724,7 @@ static void path_unregister(void *data)

next = l->next;

- g_dbus_remove_watch(conn, user_record->listener_id);
+ g_dbus_remove_watch(user_record->listener_id);
exit_callback(conn, user_record);
}

diff --git a/profiles/health/hdp_util.c b/profiles/health/hdp_util.c
index 3afd715..bbc2ed9 100644
--- a/profiles/health/hdp_util.c
+++ b/profiles/health/hdp_util.c
@@ -1184,8 +1184,7 @@ gboolean hdp_get_dcpsm(struct hdp_device *device, hdp_continue_dcpsm_f func,
static void hdp_free_application(struct hdp_application *app)
{
if (app->dbus_watcher > 0)
- g_dbus_remove_watch(btd_get_dbus_connection(),
- app->dbus_watcher);
+ g_dbus_remove_watch(app->dbus_watcher);

g_free(app->oname);
g_free(app->description);
diff --git a/profiles/heartrate/heartrate.c b/profiles/heartrate/heartrate.c
index 94d4b8d..8db20dc 100644
--- a/profiles/heartrate/heartrate.c
+++ b/profiles/heartrate/heartrate.c
@@ -209,7 +209,7 @@ static void remove_watcher(gpointer user_data)
{
struct watcher *watcher = user_data;

- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);
}

static void destroy_heartrate_adapter(gpointer user_data)
@@ -573,7 +573,7 @@ static void watcher_exit_cb(DBusConnection *conn, void *user_data)
DBG("heartrate watcher [%s] disconnected", watcher->path);

hradapter->watchers = g_slist_remove(hradapter->watchers, watcher);
- g_dbus_remove_watch(conn, watcher->id);
+ g_dbus_remove_watch(watcher->id);

if (g_slist_length(hradapter->watchers) == 0)
g_slist_foreach(hradapter->devices, disable_measurement, 0);
@@ -629,7 +629,7 @@ static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
return btd_error_does_not_exist(msg);

hradapter->watchers = g_slist_remove(hradapter->watchers, watcher);
- g_dbus_remove_watch(conn, watcher->id);
+ g_dbus_remove_watch(watcher->id);

if (g_slist_length(hradapter->watchers) == 0)
g_slist_foreach(hradapter->devices, disable_measurement, 0);
diff --git a/profiles/network/connection.c b/profiles/network/connection.c
index 7688beb..c19de49 100644
--- a/profiles/network/connection.c
+++ b/profiles/network/connection.c
@@ -129,7 +129,7 @@ static gboolean bnep_watchdog_cb(GIOChannel *chan, GIOCondition cond,
device_remove_disconnect_watch(nc->peer->device, nc->dc_id);
nc->dc_id = 0;
if (nc->watch) {
- g_dbus_remove_watch(btd_get_dbus_connection(), nc->watch);
+ g_dbus_remove_watch(nc->watch);
nc->watch = 0;
}

@@ -154,7 +154,7 @@ static void cancel_connection(struct network_conn *nc, const char *err_msg)
}

if (nc->watch) {
- g_dbus_remove_watch(conn, nc->watch);
+ g_dbus_remove_watch(nc->watch);
nc->watch = 0;
}

diff --git a/profiles/network/server.c b/profiles/network/server.c
index 6ee4770..06fc456 100644
--- a/profiles/network/server.c
+++ b/profiles/network/server.c
@@ -674,7 +674,7 @@ static DBusMessage *unregister_server(DBusConnection *conn,
if (!reply)
return NULL;

- g_dbus_remove_watch(conn, ns->watch_id);
+ g_dbus_remove_watch(ns->watch_id);

server_disconnect(conn, ns);

diff --git a/profiles/thermometer/thermometer.c b/profiles/thermometer/thermometer.c
index 3506ba7..f0988a4 100644
--- a/profiles/thermometer/thermometer.c
+++ b/profiles/thermometer/thermometer.c
@@ -145,7 +145,7 @@ static void remove_watcher(gpointer user_data)
{
struct watcher *watcher = user_data;

- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);
}

static void destroy_char(gpointer user_data)
@@ -820,7 +820,7 @@ static void watcher_exit(DBusConnection *conn, void *user_data)
remove_int_watcher(t, watcher);

t->fwatchers = g_slist_remove(t->fwatchers, watcher);
- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);

if (g_slist_length(t->fwatchers) == 0)
disable_final_measurement(t);
@@ -899,7 +899,7 @@ static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
remove_int_watcher(t, watcher);

t->fwatchers = g_slist_remove(t->fwatchers, watcher);
- g_dbus_remove_watch(btd_get_dbus_connection(), watcher->id);
+ g_dbus_remove_watch(watcher->id);

if (g_slist_length(t->fwatchers) == 0)
disable_final_measurement(t);
diff --git a/src/adapter.c b/src/adapter.c
index f11be70..3e62271 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -556,7 +556,7 @@ static void session_free(void *data)
struct session_req *req = data;

if (req->id)
- g_dbus_remove_watch(btd_get_dbus_connection(), req->id);
+ g_dbus_remove_watch(req->id);

if (req->msg) {
dbus_message_unref(req->msg);
diff --git a/src/agent.c b/src/agent.c
index 8cf37b1..694c641 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -179,8 +179,7 @@ void agent_free(struct agent *agent)
}

if (!agent->exited) {
- g_dbus_remove_watch(btd_get_dbus_connection(),
- agent->listener_id);
+ g_dbus_remove_watch(agent->listener_id);
agent_release(agent);
}

diff --git a/src/device.c b/src/device.c
index c41b0c8..35d965e 100644
--- a/src/device.c
+++ b/src/device.c
@@ -179,8 +179,7 @@ static uint16_t uuid_list[] = {
static void browse_request_free(struct browse_req *req)
{
if (req->listener_id)
- g_dbus_remove_watch(btd_get_dbus_connection(),
- req->listener_id);
+ g_dbus_remove_watch(req->listener_id);
if (req->msg)
dbus_message_unref(req->msg);
if (req->device)
@@ -2426,8 +2425,7 @@ static void bonding_request_free(struct bonding_req *bonding)
return;

if (bonding->listener_id)
- g_dbus_remove_watch(btd_get_dbus_connection(),
- bonding->listener_id);
+ g_dbus_remove_watch(bonding->listener_id);

if (bonding->msg)
dbus_message_unref(bonding->msg);
diff --git a/src/profile.c b/src/profile.c
index cb0e311..7c71e85 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -847,7 +847,7 @@ DBusMessage *btd_profile_unreg_ext(DBusConnection *conn, DBusMessage *msg,
if (!ext)
return btd_error_does_not_exist(msg);

- g_dbus_remove_watch(conn, ext->id);
+ g_dbus_remove_watch(ext->id);
remove_ext(ext);

return dbus_message_new_method_return(msg);
@@ -871,7 +871,7 @@ void btd_profile_cleanup(void)
if (msg)
g_dbus_send_message(conn, msg);

- g_dbus_remove_watch(conn, ext->id);
+ g_dbus_remove_watch(ext->id);
remove_ext(ext);

}
--
1.7.11.4


2012-10-01 17:53:33

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 02/10] gdbus: Remove connection from g_dbus_remove_watch

From: Luiz Augusto von Dentz <[email protected]>

The connection is not really needed since the list of listeners is
global not per connection, besides it is more convenient this way as
only the id is needed.
---
gdbus/gdbus.h | 2 +-
gdbus/watch.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 0a8a27c..3bd8986 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -217,7 +217,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
const char *interface, const char *member,
GDBusSignalFunction function, void *user_data,
GDBusDestroyFunction destroy);
-gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
+gboolean g_dbus_remove_watch(guint id);
void g_dbus_remove_all_watches(DBusConnection *connection);

#ifdef __cplusplus
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 07feb61..00cedae 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -285,7 +285,7 @@ static void filter_data_free(struct filter_data *data)
g_free(l->data);

g_slist_free(data->callbacks);
- g_dbus_remove_watch(data->connection, data->name_watch);
+ g_dbus_remove_watch(data->name_watch);
g_free(data->name);
g_free(data->owner);
g_free(data->path);
@@ -752,7 +752,7 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
return cb->id;
}

-gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
+gboolean g_dbus_remove_watch(guint id)
{
struct filter_data *data;
struct filter_callback *cb;
--
1.7.11.4