[pulseaudio-discuss] [PATCH 05/17] bluetooth: Parse HandsfreeAudioCard properties

[Luiz Augusto von Dentz]

Hi Tanu,

On Mon, Sep 1, 2014 at 11:01 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Fri, 2014-08-22 at 17:04 +0300, Luiz Augusto von Dentz wrote:
>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> ---
>>  src/modules/bluetooth/backend-ofono.c | 110 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
>> index 614df76..79765bb 100644
>> --- a/src/modules/bluetooth/backend-ofono.c
>> +++ b/src/modules/bluetooth/backend-ofono.c
>> @@ -25,6 +25,7 @@
>>
>>  #include <pulsecore/core-util.h>
>>  #include <pulsecore/dbus-shared.h>
>> +#include <pulsecore/shared.h>
>>
>>  #include "bluez5-util.h"
>>
>> @@ -56,9 +57,22 @@
>>      "  </interface>"                                                \
>>      "</node>"
>>
>> +typedef struct hf_audio_card {
>> +    pa_bluetooth_backend *hf;
>> +    char *path;
>> +    char *remote;
>> +    char *local;
>
> I suggest renaming these to remote_address and local_address for
> clarity.
>
>> +
>> +    int fd;
>> +    uint8_t codec;
>> +
>> +    pa_bluetooth_transport *transport;
>> +} hf_audio_card;
>
> The convention is to not use typedef for structs that are declared in .c
> files.

Ok.

>> +
>>  struct pa_bluetooth_backend {
>>      pa_core *core;
>>      pa_dbus_connection *connection;
>> +    pa_bluetooth_discovery *discovery;
>>      pa_hashmap *cards;
>>      char *ofono_bus_id;
>>
>> @@ -82,6 +96,96 @@ static pa_dbus_pending* hf_dbus_send_and_add_to_pending(pa_bluetooth_backend *ba
>>      return p;
>>  }
>>
>> +static hf_audio_card *hf_audio_card_new(pa_bluetooth_backend *hf, const char *path) {
>> +    hf_audio_card *card = pa_xnew0(hf_audio_card, 1);
>> +
>> +    card->path = pa_xstrdup(path);
>> +    card->hf = hf;
>> +    card->fd = -1;
>> +
>> +    return card;
>> +}
>> +
>> +static void hf_audio_card_free(void *data) {
>
> Don't use void pointer here just to avoid a cast in the
> pa_hashmap_new_full() call, it goes against the convention.

So we cast when using pa_hashmap_new_full?

>> +    hf_audio_card *card = data;
>> +
>> +    pa_assert(card);
>> +
>> +    pa_bluetooth_transport_free(card->transport);
>> +    pa_xfree(card->path);
>> +    pa_xfree(card->remote);
>> +    pa_xfree(card->local);
>> +    pa_xfree(card);
>> +}
>> +
>> +static int hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
>> +    return -1;
>> +}
>> +
>> +static void hf_audio_agent_transport_release(pa_bluetooth_transport *t) {
>> +}
>> +
>> +static void hf_audio_agent_card_found(pa_bluetooth_backend *backend, const char *path, DBusMessageIter *props_i) {
>> +    DBusMessageIter i, value_i;
>> +    const char *key, *value;
>> +    hf_audio_card *card;
>> +    pa_bluetooth_device *d;
>> +
>> +    pa_assert(backend);
>> +    pa_assert(path);
>> +    pa_assert(props_i);
>> +
>> +    pa_log_debug("New HF card found: %s", path);
>> +
>> +    card = hf_audio_card_new(backend, path);
>> +
>> +    while (dbus_message_iter_get_arg_type(props_i) != DBUS_TYPE_INVALID) {
>> +        char c;
>> +
>> +        dbus_message_iter_recurse(props_i, &i);
>> +
>> +        dbus_message_iter_get_basic(&i, &key);
>> +        dbus_message_iter_next(&i);
>> +        dbus_message_iter_recurse(&i, &value_i);
>> +
>> +        if ((c = dbus_message_iter_get_arg_type(&value_i)) != DBUS_TYPE_STRING) {
>> +            pa_log_error("Invalid properties for %s: expected \'s\', received \'%c\'", path, c);
>
> Escaping the apostrophes is unnecessary.

Will fix this.

>> +            goto fail;
>> +        }
>> +
>> +        dbus_message_iter_get_basic(&value_i, &value);
>> +
>> +        if (pa_streq(key, "RemoteAddress"))
>> +            card->remote = pa_xstrdup(value);
>> +        else if (pa_streq(key, "LocalAddress"))
>> +            card->local = pa_xstrdup(value);
>
> You should prepare for duplicate properties. The current code leaks
> memory in case of duplicates.

I guess you meant to be safe it is better to free before assigning
anything, perhaps if we handle this together with PropertiesChanged it
would make these checks common.

>> +
>> +        pa_log_debug("%s: %s", key, value);
>> +
>> +        dbus_message_iter_next(props_i);
>> +    }
>> +
>> +    pa_hashmap_put(backend->cards, card->path, card);
>> +
>> +    d = pa_bluetooth_discovery_get_device_by_address(backend->discovery, card->remote, card->local);
>> +    if (d) {
>> +        card->transport = pa_bluetooth_transport_new(d, backend->ofono_bus_id, path, PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY, NULL, 0);
>> +        card->transport->acquire = hf_audio_agent_transport_acquire;
>> +        card->transport->release = hf_audio_agent_transport_release;
>> +        card->transport->userdata = backend;
>> +
>> +        d->transports[PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY] = card->transport;
>
> I don't see matching assignment that sets the device transport to NULL
> when the transport is removed.
>
> I think this assignment doesn't belong here anyway, it should be done in
> pa_bluetooth_transport_put(). (Setting the device transport to NULL
> would then ideally be done in pa_bluetooth_transport_unlink(), because
> unlink() should generally mirror put() so that unlink() undoes
> everything that is done in put().)

Sounds like a good idea, I will look into to that.

>> +
>> +        pa_bluetooth_transport_put(card->transport);
>> +    } else
>> +        pa_log_error("Device doesnt exist for %s", path);
>
> It seems possible at least in theory that we get HandsfreeAudioCard
> properties from oFono before we get Device properties from BlueZ. Is
> there something (other than implementation details) that guarantees that
> this doesn't happen? If nothing guarantees that, then we should handle
> the case where information from oFono arrives before information from
> BlueZ.

There is a fix for that in a latter in the patch-set but I can fixup
this one as well.

>> +
>> +    return;
>> +
>> +fail:
>> +    pa_xfree(card);
>
> Use hf_audio_card_free() to make sure you don't leak any of the struct
> members.

Ok

>
>> +}
>> +
>>  static void hf_audio_agent_get_cards_reply(DBusPendingCall *pending, void *userdata) {
>>      DBusMessage *r;
>>      pa_dbus_pending *p;
>> @@ -113,7 +217,7 @@ static void hf_audio_agent_get_cards_reply(DBusPendingCall *pending, void *userd
>>
>>          dbus_message_iter_recurse(&struct_i, &props_i);
>>
>> -        /* TODO: Parse HandsfreeAudioCard properties */
>> +        hf_audio_agent_card_found(backend, path, &props_i);
>>
>>          dbus_message_iter_next(&array_i);
>>      }
>> @@ -293,7 +397,9 @@ pa_bluetooth_backend *pa_bluetooth_backend_new(pa_core *c) {
>>
>>      backend = pa_xnew0(pa_bluetooth_backend, 1);
>>      backend->core = c;
>> -    backend->cards = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>> +    backend->cards = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL,
>> +                                                 hf_audio_card_free);
>
> Unaligned indentation.

Will fix it.

>> +    backend->discovery = pa_shared_get(c, "bluetooth-discovery");
>
> I think it would be better to pass the discovery object as an argument
> to pa_bluetooth_backend_new().

So it would be passed along with core? I tried changing to just pass
the discovery object but core is actually used to access the D-Bus
connection object.

-- 
Luiz Augusto von Dentz