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
