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

[Tanu Kaskinen]

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.

-- 
Tanu