[pulseaudio-discuss] [PATCH v4 18/41] bluetooth: Parse BlueZ 5 device properties

[João Paulo Rechi Vita]

On Wed, Sep 25, 2013 at 7:04 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Tue, 2013-09-24 at 12:43 -0300, João Paulo Rechi Vita wrote:
>> On Sat, Sep 21, 2013 at 8:37 AM, Tanu Kaskinen
>> <tanu.kaskinen at linux.intel.com> wrote:
>> > On Wed, 2013-09-18 at 16:17 -0500, jprvita at gmail.com wrote:
>> >> +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.
>> >
>>
>> Ok, I'm making the adapter_path field persistent instead of temporary,
>> so I'll check for it here instead of adapter.
>
> I don't see why that is necessary.
>

This way we check only for d->adapter_path, instead of d->adapter and
d->adapter_path, to decide whether the device info is valid or not,
which is really the property that is part of the device object.

>> >> +        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.
>> >
>>
>> Ok. FWIW, I think this is a little too paranoid. BlueZ never contructs
>> the message in a way that this branch will be taken, we don't care
>> about this scenario not even in the BlueZ tools.
>
> If the BlueZ tools are shipped along with bluetoothd, then they can
> depend on bluetoothd's implementation details, because updates will
> happen synchronously. We don't have that privilege in PulseAudio. Please
> program against the documented interface, not the implementation.
>

I used the BlueZ tools just as an example of how little likely this is
to change due to how much in BlueZ we rely on this not changing. Maybe
what is really missing is documenting this behavior on the BlueZ side.
But no point in taking this discussion any further, we're already
protecting ourselves from this problem.

>> > 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.
>> >
>>
>> The set_configuration handler should take care of that.
>
> When the device doesn't yet exist when endpoint_set_configuration() is
> called, then endpoint_set_configuration() will create the device, and at
> that time the properties haven't yet been received, so
> DEVICE_CONNECTION_CHANGED will not be fired. The device will become
> available once the properties have been received, so the hook must be
> fired at the time the properties are received.
>

DEVICE_CONNECTION_CHANGED is  fired by transport_state_changed() which
will be called by pa_bluetooth_transport_put() in this case. Nothing
in this code path depends on the device properties being received, or
the device_info being 1. The created transport will be the first one
on the device (since the device would already exist otherwise) so the
"any transport connected" state will change for it and
DEVICE_CONNECTION_CHANGED will be fired.

-- 
João Paulo Rechi Vita
http://about.me/jprvita