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