On Tue, 2014-02-04 at 19:03 -0300, jprvita at gmail.com wrote:
> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>
> ---
> src/modules/bluetooth/hfaudioagent-ofono.c | 129 ++++++++++++++++++++++++++++-
> 1 file changed, 127 insertions(+), 2 deletions(-)
>
> diff --git a/src/modules/bluetooth/hfaudioagent-ofono.c b/src/modules/bluetooth/hfaudioagent-ofono.c
> index c939988..d381a29 100644
> --- a/src/modules/bluetooth/hfaudioagent-ofono.c
> +++ b/src/modules/bluetooth/hfaudioagent-ofono.c
> @@ -25,6 +25,9 @@
>
> #include <pulsecore/core-util.h>
> #include <pulsecore/dbus-shared.h>
> +#include <pulsecore/shared.h>
> +
> +#include "bluez5-util.h"
>
> #include "hfaudioagent.h"
>
> @@ -56,9 +59,21 @@
> " </interface>" \
> "</node>"
>
> +typedef struct hf_audio_card {
> + 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;
> +
> struct hf_audio_agent_data {
> pa_core *core;
> pa_dbus_connection *connection;
> + pa_bluetooth_discovery *discovery;
>
> bool filter_added;
> char *ofono_bus_id;
> @@ -84,6 +99,111 @@ static pa_dbus_pending* pa_bluetooth_dbus_send_and_add_to_pending(hf_audio_agent
> return p;
> }
>
> +static hf_audio_card *hf_audio_card_new(hf_audio_agent_data *hfdata, const char *path) {
> + hf_audio_card *hfac = pa_xnew0(hf_audio_card, 1);
> +
> + hfac->path = pa_xstrdup(path);
> + hfac->fd = -1;
> +
> + return hfac;
> +}
> +
> +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(), it goes against the convention.
> + hf_audio_card *hfac = data;
> +
> + pa_assert(hfac);
> +
> + pa_bluetooth_transport_free(hfac->transport);
> + pa_xfree(hfac->path);
> + pa_xfree(hfac->remote);
> + pa_xfree(hfac->local);
> + pa_xfree(hfac);
> +}
> +
> +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(hf_audio_agent_data *hfdata, const char *path, DBusMessageIter *props_i) {
> + DBusMessageIter i, value_i;
> + const char *key, *value;
> + hf_audio_card *hfac;
> + pa_bluetooth_device *d;
> +
> + pa_assert(hfdata);
> + pa_assert(path);
> + pa_assert(props_i);
> +
> + pa_log_debug("New HF card found: %s", path);
> +
> + hfac = hf_audio_card_new(hfdata, path);
> +
> + while (dbus_message_iter_get_arg_type(props_i) != DBUS_TYPE_INVALID) {
> + char c;
> +
> + if ((c = dbus_message_iter_get_arg_type(props_i)) != DBUS_TYPE_DICT_ENTRY) {
> + pa_log_error("Invalid properties for %s: expected \'e\', received \'%c\'", path, c);
> + goto fail;
> + }
These arg type checks can be avoided if you check the message signature.
(Variant contents of course still need to be validated.)
> +
> + dbus_message_iter_recurse(props_i, &i);
> +
> + if ((c = dbus_message_iter_get_arg_type(&i)) != DBUS_TYPE_STRING) {
> + pa_log_error("Invalid properties for %s: expected \'s\', received \'%c\'", path, c);
> + goto fail;
> + }
> +
> + dbus_message_iter_get_basic(&i, &key);
> + dbus_message_iter_next(&i);
> +
> + if ((c = dbus_message_iter_get_arg_type(&i)) != DBUS_TYPE_VARIANT) {
> + pa_log_error("Invalid properties for %s: expected \'v\', received \'%c\'", path, c);
> + goto fail;
> + }
> +
> + 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);
> + goto fail;
> + }
> +
> + dbus_message_iter_get_basic(&value_i, &value);
> +
> + if (pa_streq(key, "RemoteAddress"))
> + hfac->remote = pa_xstrdup(value);
> + else if (pa_streq(key, "LocalAddress"))
> + hfac->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(hfdata->hf_audio_cards, hfac->path, hfac);
> +
> + d = pa_bluetooth_discovery_get_device_by_address(hfdata->discovery, hfac->remote, hfac->local);
> + if (d) {
> + hfac->transport = pa_bluetooth_transport_new(d, hfdata->ofono_bus_id, path, PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY, NULL, 0);
> + hfac->transport->acquire = hf_audio_agent_transport_acquire;
> + hfac->transport->release = hf_audio_agent_transport_release;
> + hfac->transport->userdata = hfdata;
> +
> + d->transports[PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY] = hfac->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(hfac->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(hfac);
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;
> @@ -132,7 +252,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(hfdata, path, &props_i);
>
> dbus_message_iter_next(&array_i);
> }
> @@ -312,7 +432,9 @@ hf_audio_agent_data *hf_audio_agent_init(pa_core *c) {
>
> hfdata = pa_xnew0(hf_audio_agent_data, 1);
> hfdata->core = c;
> - hfdata->hf_audio_cards = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> + hfdata->hf_audio_cards = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, hf_audio_card_free,
> + NULL);
The third argument of pa_hashmap_new_full() is the function for freeing
the key and the fourth argument is the function for freeing the value.
The key freeing function should be NULL and hf_audio_card_free() should
be used for freeing the value. Doesn't this cause crashing on your
system every time you shut down pulseaudio?
> + hfdata->discovery = pa_shared_get(c, "bluetooth-discovery");
I think it would be better to pass the discovery object as an argument
to hf_audio_agent_init().
>
> dbus_error_init(&err);
>
> @@ -381,5 +503,8 @@ void hf_audio_agent_done(hf_audio_agent_data *data) {
> pa_dbus_connection_unref(hfdata->connection);
> }
>
> + if (hfdata->discovery)
> + hfdata->discovery = NULL;
This is pointless, since hfdata gets freed anyway. Or maybe the idea is
to increase the chances of a quick crash due to null pointer
dereferencing if there's buggy code that tries to access
hfdata->discovery after hfdata is freed? Even in that case having the if
is unnecessary, you can do the assignment unconditionally.
--
Tanu
