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