[pulseaudio-discuss] [PATCH 37/56] bluetooth: Parse BlueZ 5 device properties

[Tanu Kaskinen]

On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
> +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 = pa_bluetooth_dbus_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);
> +            } 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->address) {
> +                    pa_log_error("Device %s: Received a duplicate Address property.", d->path);
> +                    return -1;
> +                }
> +
> +                d->address = pa_xstrdup(value);
> +            }
> +
> +            pa_log_debug("%s: %s", key, value);

Please use proper sentences in the log. Or is this a temporary log
message that you just forgot to remove from the final patch?

> +            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;

Is the device allowed to change its class at runtime? If not, then there
should be a check for is_property_change like with Address.

> +
> +            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")) {
> +                while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
> +                    pa_bluetooth_uuid *node;
> +                    const char *value;
> +
> +                    dbus_message_iter_get_basic(&ai, &value);
> +
> +                    if (pa_bluetooth_uuid_has(d->uuids, value)) {
> +                        dbus_message_iter_next(&ai);
> +                        continue;
> +                    }
> +
> +                    node = pa_bluetooth_uuid_new(value);
> +                    PA_LLIST_PREPEND(pa_bluetooth_uuid, d->uuids, node);
> +
> +                    pa_log_debug("%s: %s", key, value);
> +                    dbus_message_iter_next(&ai);

This code only adds UUIDs to pa_bluetooth_device, never removes them. Is
this intentional? If so, I think a comment explaining the rationale
would be good.

There aren't any hooks that would be fired when the properties (Alias,
Class, UUIDs) change. Perhaps you implement them in some later patches?

> +                }
> +            }
> +
> +            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)
> +            ret = -1;
> +
> +        dbus_message_iter_next(&element_i);
> +    }
> +
> +    if (!d->address || !d->alias/* || d->paired < 0 || d->trusted < 0*/) {

If you don't plan to support the paired and trusted properties (which
would be a good plan IMO), please remove the commented out code.

> +        pa_log_error("Non-optional information missing for device %s", d->path);
> +        d->device_info_valid = -1;
> +        return -1;
> +    }
> +
> +    d->device_info_valid = 1;
> +    return ret;
> +}
> +
>  static void register_endpoint_reply(DBusPendingCall *pending, void *userdata) {
>      DBusMessage *r;
>      pa_dbus_pending *p;
> @@ -423,6 +595,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>              register_endpoint(y, path, A2DP_SOURCE_ENDPOINT, A2DP_SOURCE_UUID);
>              register_endpoint(y, path, A2DP_SINK_ENDPOINT, A2DP_SINK_UUID);
>          } else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
> +            pa_bluetooth_device *d;
> +
>              if (pa_bluetooth_discovery_get_device_by_path(y, path)) {
>                  pa_log_error("Found duplicated D-Bus path for device %s", path);
>                  return;
> @@ -430,8 +604,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>  
>              pa_log_debug("Device %s found", path);
>  
> -            pa_bluetooth_discovery_create_device(y, path);
> -            /* TODO: parse device properties */
> +            d = pa_bluetooth_discovery_create_device(y, path);
> +            parse_device_properties(d, &iface_i, false);

If parsing the device properties fails, we shouldn't create the device.

>          } else
>              pa_log_debug("Unknown interface %s found, skipping", interface);
>  
> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
> index 68c2dc0..b1a112f 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -27,6 +27,7 @@
>  #define A2DP_SOURCE_UUID        "0000110a-0000-1000-8000-00805f9b34fb"
>  #define A2DP_SINK_UUID          "0000110b-0000-1000-8000-00805f9b34fb"
>  
> +typedef struct pa_bluetooth_uuid pa_bluetooth_uuid;

I don't see the point of a separate struct. The purpose seems to be only
to make it possible to save the UUIDs in a linked list, but using a
hashmap (as a set, i.e. the key and value are the same) would be much
simpler.

-- 
Tanu