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

[João Paulo Rechi Vita]

On Sat, Sep 28, 2013 at 6:06 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Thu, 2013-09-26 at 19:56 -0300, João Paulo Rechi Vita wrote:
>> 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.
>
> Ok, got it now.
>
>> >> >> +        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.
>
> No, the "any transport connected" state will not change.
> pa_bluetooth_device_any_transport_connected() returns false as long as
> d->device_info_valid is zero.
>

Yes, you're correct here. I've already answered the patch where you
propose a fix to this problem.

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