On Mon, Jul 22, 2013 at 7:33 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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?
>
This is usually received when the device is created, together with a
lot of other properties, so I think is better to minimize verbosity
here. What I'll change to improve this scenario is add a log in the
beginning of the function with the device path.
>> + 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.
>
I'm not sure, and BlueZ' documentation does't mention anything about
that. Maybe Luiz have more information about that.
>> +
>> + 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?
>
This is used to create card profiles on load of module-bluez5-device.
It is possible (although not likely) that a device change the
implemented UUIDs during the connection lifetime. Alias can change,
but I think this should be handled in a separate patch (I've not
written that yet).
>> + }
>> + }
>> +
>> + 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.
>
Yes, I've forgot to remove that. There is no use for the Paired and
Trusted properties in PulseAudio.
>> + 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.
>
parse_device_properties() will fail even if the parsing of an optional
property fails, but the device is still fully usable, so we should
still create the device in this case. To differentiate between these
two situations there is the device_info_valid field. Maybe we can
review the necessity of the device_info_valid field, but since this
affects the modules working logic and may introduce bugs I would
prefer to change that in a latter moment to minimize the impact.
>> } 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.
>
AFAIK that's the only purpose, so I think it makes sense to change to
a hashmap as you suggested.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
