On Sat, Sep 21, 2013 at 8:37 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Wed, 2013-09-18 at 16:17 -0500, jprvita at gmail.com wrote:
>> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> This code is based on previous work by Mikel Astiz.
>> ---
>> src/modules/bluetooth/bluez5-util.c | 164 ++++++++++++++++++++++++++++++++++--
>> src/modules/bluetooth/bluez5-util.h | 6 ++
>> 2 files changed, 161 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index 7fc7d50..49d5c38 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -297,6 +297,7 @@ static pa_bluetooth_device* device_create(pa_bluetooth_discovery *y, const char
>> d = pa_xnew0(pa_bluetooth_device, 1);
>> d->discovery = y;
>> d->path = pa_xstrdup(path);
>> + d->uuids = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>>
>> pa_hashmap_put(y->devices, d->path, d);
>>
>> @@ -348,6 +349,9 @@ static void device_free(pa_bluetooth_device *d) {
>> pa_bluetooth_transport_free(t);
>> }
>>
>> + if (d->uuids)
>> + pa_hashmap_free(d->uuids, NULL);
>
> The hashmap is not necessarily empty, so this leaks the contained uuid
> strings.
>
> The hashmap interface changed recently, and now the correct way to deal
> with this is to call pa_hashmap_new_full(), and provide a free callback
> for the value (since the key is the same object as the value, don't
> provide free callbacks for both the value and the key, otherwise there
> will be double freeing).
>
Ok.
>> +
>> d->discovery = NULL;
>> d->adapter = NULL;
>> pa_xfree(d->path);
>> @@ -408,6 +412,144 @@ static void adapter_remove_all(pa_bluetooth_discovery *y) {
>> }
>> }
>>
>> +static void parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
>> + const char *key;
>> + DBusMessageIter variant_i;
>> +
>> + pa_assert(d);
>> +
>> + key = check_variant_property(i);
>> + if (key == NULL) {
>> + pa_log_error("Received invalid property for device %s", d->path);
>> + return;
>> + }
>> +
>> + dbus_message_iter_recurse(i, &variant_i);
>> +
>> + switch (dbus_message_iter_get_arg_type(&variant_i)) {
>> +
>> + case DBUS_TYPE_STRING: {
>> + const char *value;
>> + dbus_message_iter_get_basic(&variant_i, &value);
>> +
>> + if (pa_streq(key, "Alias")) {
>> + pa_xfree(d->alias);
>> + d->alias = pa_xstrdup(value);
>> + pa_log_debug("%s: %s", key, value);
>> + } else if (pa_streq(key, "Address")) {
>> + if (is_property_change) {
>> + pa_log_warn("Device property 'Address' expected to be constant but changed for %s, ignoring", d->path);
>> + return;
>> + }
>> +
>> + if (d->address) {
>> + pa_log_warn("Device %s: Received a duplicate 'Address' property, ignoring", d->path);
>> + return;
>> + }
>> +
>> + d->address = pa_xstrdup(value);
>> + pa_log_debug("%s: %s", key, value);
>> + }
>> +
>> + break;
>> + }
>> +
>> + case DBUS_TYPE_OBJECT_PATH: {
>> + const char *value;
>> + dbus_message_iter_get_basic(&variant_i, &value);
>> +
>> + if (pa_streq(key, "Adapter")) {
>> +
>> + if (is_property_change) {
>> + pa_log_warn("Device property 'Adapter' expected to be constant but changed for %s, ignoring", d->path);
>> + return;
>> + }
>> +
>> + if (d->adapter) {
>> + pa_log_warn("Device %s: Received a duplicate 'Adapter' property, ignoring", d->path);
>> + return;
>> + }
>> +
>> + d->adapter = pa_hashmap_get(d->discovery->adapters, value);
>> + if (!d->adapter) {
>> + pa_log_warn("Device %s: 'Adapter' property references an unknown adapter %s.", d->path, value);
>
> This is normal behaviour, no need to log anything, especially not at the
> warning level.
>
I'm reducing it to info then, since I think it's usefull to log it and
that is of more relevance than debug. Normal behavior would be if the
adapter was already know at this point, since bluetoothd appends the
message arguments depth-first from the object tree root.
>> + d->_adapter_path = pa_xstrdup(value);
>> + break;
>
> Does it really make sense to break here? The only thing this skips is
> the line that logs the property key and value, and I don't see any
> reason to skip that.
>
I was planning to log it when it is finally resolved to an address,
but maybe it is better to keep this log here.
>> + }
>> +
>> + pa_log_debug("%s: %s", key, value);
>> + }
>> +
>> + break;
>> + }
>> +
>> + case DBUS_TYPE_UINT32: {
>> + uint32_t value;
>> + dbus_message_iter_get_basic(&variant_i, &value);
>> +
>> + if (pa_streq(key, "Class")) {
>> + d->class_of_device = value;
>> + pa_log_debug("%s: %d", key, value);
>> + }
>> +
>> + break;
>> + }
>> +
>> + case DBUS_TYPE_ARRAY: {
>> + DBusMessageIter ai;
>> + dbus_message_iter_recurse(&variant_i, &ai);
>> +
>> + if (dbus_message_iter_get_arg_type(&ai) == DBUS_TYPE_STRING && pa_streq(key, "UUIDs")) {
>> + /* bluetoothd never removes UUIDs from a device object so there
>> + * is no need to handle it here. */
>> + while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
>> + const char *value;
>> + char *uuid;
>> +
>> + dbus_message_iter_get_basic(&ai, &value);
>> +
>> + if (pa_hashmap_get(d->uuids, value)) {
>> + dbus_message_iter_next(&ai);
>> + continue;
>> + }
>> +
>> + uuid = pa_xstrdup(value);
>> + pa_hashmap_put(d->uuids, uuid, uuid);
>> +
>> + pa_log_debug("%s: %s", key, value);
>> + dbus_message_iter_next(&ai);
>> + }
>> + }
>> +
>> + break;
>> + }
>> + }
>> +}
>> +
>> +static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
>> + DBusMessageIter element_i;
>> + int ret = 0;
>> +
>> + dbus_message_iter_recurse(i, &element_i);
>> +
>> + while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>> + DBusMessageIter dict_i;
>> +
>> + dbus_message_iter_recurse(&element_i, &dict_i);
>> + parse_device_property(d, &dict_i, is_property_change);
>> + dbus_message_iter_next(&element_i);
>> + }
>> +
>> + if (!d->address || !d->adapter || !d->alias) {
>> + pa_log_error("Non-optional information missing for device %s", d->path);
>
> This error shouldn't be printed if d->_adapter_path is set.
>
Ok, I'm making the adapter_path field persistent instead of temporary,
so I'll check for it here instead of adapter.
>> + d->device_info_valid = -1;
>> + return -1;
>> + }
>> +
>> + d->device_info_valid = 1;
>> + return ret;
>> +}
>> +
>> static int parse_adapter_properties(pa_bluetooth_adapter *a, DBusMessageIter *i, bool is_property_change) {
>> DBusMessageIter element_i;
>>
>> @@ -520,6 +662,8 @@ static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const
>> static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessageIter *dict_i) {
>> DBusMessageIter element_i;
>> const char *path;
>> + void *state = NULL;
>> + pa_bluetooth_device *d;
>>
>> pa_assert(dbus_message_iter_get_arg_type(dict_i) == DBUS_TYPE_OBJECT_PATH);
>> dbus_message_iter_get_basic(dict_i, &path);
>> @@ -558,25 +702,18 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>> register_endpoint(y, path, A2DP_SINK_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK);
>>
>> } else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
>> - pa_bluetooth_device *d;
>>
>> if ((d = pa_hashmap_get(y->devices, path))) {
>> - if (d->device_info_valid == 1) {
>> + if (d->device_info_valid != 0) {
>> pa_log_error("Found duplicated D-Bus path for device %s", path);
>> return;
>> }
>> -
>> - if (d->device_info_valid == -1) {
>> - pa_log_notice("Device %s was known before but had invalid information, reseting", path);
>> - d->device_info_valid = 0;
>> - }
>> -
>> } else
>> d = device_create(y, path);
>>
>> pa_log_debug("Device %s found", d->path);
>>
>> - /* TODO: parse device properties */
>> + parse_device_properties(d, &iface_i, false);
>>
>> } else
>> pa_log_debug("Unknown interface %s found, skipping", interface);
>> @@ -584,6 +721,15 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>> dbus_message_iter_next(&element_i);
>> }
>>
>> + while ((d = pa_hashmap_iterate(y->devices, &state, NULL)))
>
> PA_HASHMAP_FOREACH would be nicer.
>
Ok.
>> + if (!d->adapter && d->_adapter_path) {
>> + d->adapter = pa_hashmap_get(d->discovery->adapters, d->_adapter_path);
>> + pa_xfree(d->_adapter_path);
>> + d->_adapter_path = NULL;
>> + if (d->adapter && d->address && d->alias)
>> + d->device_info_valid = 1;
>
> If d->adapter is not set at this point, then please log an error.
>
Ok. FWIW, I think this is a little too paranoid. BlueZ never contructs
the message in a way that this branch will be taken, we don't care
about this scenario not even in the BlueZ tools.
> After the device properties have been successfully parsed for a new
> device, the DEVICE_CONNECTION_CHANGED hook needs to be fired if there is
> already some transport connected for the device.
>
The set_configuration handler should take care of that.
>> + }
>> +
>> return;
>> }
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
>> index 4e0b567..c0adfb3 100644
>> --- a/src/modules/bluetooth/bluez5-util.h
>> +++ b/src/modules/bluetooth/bluez5-util.h
>> @@ -76,6 +76,11 @@ struct pa_bluetooth_device {
>> pa_bluetooth_discovery *discovery;
>> pa_bluetooth_adapter *adapter;
>>
>> + /* Do no use this field: _adapter_path is only used temporarily when the
>
> Typo: "no" -> "not".
>
This comment has been removed.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
