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
