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

[Tanu Kaskinen]

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.

> +
>  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.

> +    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.

> +            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.

> +
> +        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().)

> +
> +        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.

> +
> +    return;
> +
> +fail:
> +    pa_xfree(card);

Use hf_audio_card_free() to make sure you don't leak any of the struct
members.

> +}
> +
>  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.

> +    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().

-- 
Tanu