On Sun, Aug 18, 2013 at 9:41 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Tue, 2013-08-13 at 01:54 -0300, 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 | 150 +++++++++++++++++++++++++++++++++++-
>> src/modules/bluetooth/bluez5-util.h | 1 +
>> 2 files changed, 150 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index 1118bd0..94e3c35 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -282,6 +282,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);
>>
>> @@ -336,6 +337,9 @@ static void device_free(pa_bluetooth_device *d) {
>> pa_bluetooth_transport_free(t);
>> }
>>
>> + if (d->uuids)
>> + pa_hashmap_free(d->uuids, NULL);
>> +
>> d->discovery = NULL;
>> pa_xfree(d->path);
>> pa_xfree(d->alias);
>> @@ -379,6 +383,145 @@ static void adapter_remove_all(pa_bluetooth_discovery *y) {
>> }
>> }
>>
>> +static int 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 -1;
>> + }
>> +
>> + 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_error("Device property 'Address' expected to be constant but changed for %s", d->path);
>> + return -1;
>> + }
>> +
>> + if (d->remote) {
>> + pa_log_error("Device %s: Received a duplicate 'Address' property.", d->path);
>> + return -1;
>> + }
>> +
>> + d->remote = 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")) {
>> + pa_bluetooth_adapter *a;
>> +
>> + if (is_property_change) {
>> + pa_log_error("Device property 'Adapter' expected to be constant but changed for %s", d->path);
>> + return -1;
>> + }
>> +
>> + if (d->local) {
>> + pa_log_error("Device %s: Received a duplicate 'Adapter' property.", d->path);
>> + return -1;
>> + }
>> +
>> + a = pa_hashmap_get(d->discovery->adapters, value);
>> + d->local = pa_xstrdup(a->address);
>
> There's no guarantee that a is non-NULL. bluetoothd may send a bogus
> adapter path, or since the initial information about adapters and
> devices is sent in the same message, it's possible that we haven't yet
> parsed the adapter object information.
>
> This is a bit hairy situation, because we don't know whether we will see
> the referenced adapter later or not, if it's missing at this point. I
> think we will have to save the adapter path here, and attempt to resolve
> it to a pa_bluetooth_adapter pointer after all interfaces have been
> parsed.
>
I think the most reliable way to solve this is, as you said, save the
adapter path if the adapter is still unknown to us and later on, after
all interfaces have been parsed, go through all devices and update the
ones which the adapter pointer is still NULL. I think we should only
keep the adapter path temporarily, and free it at the moment we
resolve the adapter pointer
>> + 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 = (int) value;
>
> Why is class_of_device signed if the Class property is unsigned? I would
> understand it if uninitialized class was signalled with a negative
> value, but that doesn't seem to be done either.
>
This is also something inherited from old code, I agree with you it
should be uint32_t.
>> + 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;
>> +
>> + dbus_message_iter_get_basic(&ai, &value);
>> +
>> + if (pa_hashmap_get(d->uuids, value)) {
>> + dbus_message_iter_next(&ai);
>> + continue;
>> + }
>> +
>> + pa_hashmap_put(d->uuids, value, (void *) 1);
>
> I'd prefer pa_hashmap_put(d->uuids, value, value).
>
> And you need to copy the value before storing it to the hashmap
> (remember then to also free it when freeing the hashmap).
>
Ok. I was trying to avoid this copy because I thought the key was
already copied inside pa_hasmap_put(), but checking the code again I
see it's not.
>> +
>> + pa_log_debug("%s: %s", key, value);
>> + dbus_message_iter_next(&ai);
>> + }
>> + }
>> +
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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);
>> +
>> + if (parse_device_property(d, &dict_i, is_property_change) < 0) {
>> + d->device_info_valid = -1;
>
> If you set device_info_valid = -1 here, then the info can become invalid
> at any time the properties change, rendering the device unusable. I
> don't have anything against that, and in fact I prefer that behaviour,
> but there needs to be a notification mechanism so that e.g.
> module-bluetooth-device knows when the device blows up. That
> notification mechanism is not implemented in this patch nor in the next
> one, which implements the PropertiesChanged handling.
>
Reviewing this code I think we should not set device_info_valid = -1
here. Even more, parse_device_property() should return void and the
appropriate value for device_info_valid be set based on the presence
or absence of the mandatory device properties only. This also fixes
the next two comments.
>> + ret = -1;
>
> You can just return -1 here immediately (that will also fix the problem
> pointed out below).
>
>> + }
>> +
>> + dbus_message_iter_next(&element_i);
>> + }
>> +
>> + if (!d->remote || !d->local || !d->alias) {
>> + pa_log_error("Non-optional information missing for device %s", d->path);
>> + d->device_info_valid = -1;
>> + return -1;
>> + }
>> +
>> + d->device_info_valid = 1;
>
> This will override the earlier d->device_info_valid = -1 assignment.
>
>> + return ret;
>> +}
>> +
>> static int parse_adapter_properties(pa_bluetooth_discovery *y, const char *adapter, DBusMessageIter *i) {
>> DBusMessageIter element_i;
>>
>> @@ -532,6 +675,11 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>>
>> if (d->device_info_valid == -1) {
>> pa_log_notice("Device %s was known before but had invalid information, reseting", path);
>> + pa_hashmap_remove_all(d->uuids, NULL);
>> + pa_xfree(d->alias);
>> + pa_xfree(d->remote);
>> + pa_xfree(d->local);
>> + d->class_of_device = 0;
>
> This is very error prone. Whenever a new field is added to
> pa_bluetooth_device, it will have to be reset here, which is very easy
> to forget. I would prefer us to not have this reset logic at all, since
> it's not necessary (if device_info_valid is -1, then BlueZ is buggy, so
> we have no obligation to try to make things work here).
>
> If resetting is done, then the freed pointers need to be set to NULL.
>
Yes, re-thinking about it I agree we better do not reset it here. If
device_info_valid == -1 we should just return and do not try to parse
any property.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
