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

[Tanu Kaskinen]

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.

> >> +        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.

> > 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.

-- 
Tanu