On Thu, Jul 25, 2013 at 1:44 AM, Jo?o Paulo Rechi Vita
<jprvita at gmail.com> wrote:
> 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.
>
Looking into the code, the class of device can change on every new connection.
>>> +
>>> + 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
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
