[pulseaudio-discuss] [PATCH v4 18/41] bluetooth: Parse BlueZ 5 device properties
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Sat Sep 21 06:37:29 PDT 2013
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).
> +
> 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.
> + 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.
> + }
> +
> + 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.
> + 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.
> + 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.
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.
> + }
> +
> 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".
--
Tanu
More information about the pulseaudio-discuss
mailing list