On Thu, 2013-08-08 at 20:02 -0300, Jo?o Paulo Rechi Vita wrote:
> On Wed, Aug 7, 2013 at 3:08 AM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Tue, 2013-08-06 at 12:05 -0300, Jo?o Paulo Rechi Vita wrote:
> >> On Mon, Jul 29, 2013 at 8:40 AM, Tanu Kaskinen
> >> <tanu.kaskinen at linux.intel.com> wrote:
> >> > On Thu, 2013-07-25 at 01:44 -0300, Jo?o Paulo Rechi Vita wrote:
> >> >> On Mon, Jul 22, 2013 at 7:33 AM, Tanu Kaskinen
> >> >> <tanu.kaskinen at linux.intel.com> wrote:
> >> >> > On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
> >> >> >> +
> >> >> >> + pa_log_debug("%s: %d", key, value);
> >> >> >> + break;
> >> >> >> + }
> >> >> >> +
> >> >> >> + case DBUS_TYPE_ARRAY: {
> >> >> >> + DBusMessageIter ai;
> >> >> >> + dbus_message_iter_recurse(&variant_i, &ai);
> >> >> >> +
> >> >> >> + if (dbus_message_iter_get_arg_type(&ai) == DBUS_TYPE_STRING && pa_streq(key, "UUIDs")) {
> >> >> >> + while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
> >> >> >> + pa_bluetooth_uuid *node;
> >> >> >> + const char *value;
> >> >> >> +
> >> >> >> + dbus_message_iter_get_basic(&ai, &value);
> >> >> >> +
> >> >> >> + if (pa_bluetooth_uuid_has(d->uuids, value)) {
> >> >> >> + dbus_message_iter_next(&ai);
> >> >> >> + continue;
> >> >> >> + }
> >> >> >> +
> >> >> >> + node = pa_bluetooth_uuid_new(value);
> >> >> >> + PA_LLIST_PREPEND(pa_bluetooth_uuid, d->uuids, node);
> >> >> >> +
> >> >> >> + pa_log_debug("%s: %s", key, value);
> >> >> >> + dbus_message_iter_next(&ai);
> >> >> >
> >> >> > This code only adds UUIDs to pa_bluetooth_device, never removes them. Is
> >> >> > this intentional? If so, I think a comment explaining the rationale
> >> >> > would be good.
> >> >> >
> >> >> > There aren't any hooks that would be fired when the properties (Alias,
> >> >> > Class, UUIDs) change. Perhaps you implement them in some later patches?
> >> >> >
> >> >>
> >> >> This is used to create card profiles on load of module-bluez5-device.
> >> >> It is possible (although not likely) that a device change the
> >> >> implemented UUIDs during the connection lifetime. Alias can change,
> >> >> but I think this should be handled in a separate patch (I've not
> >> >> written that yet).
> >> >
> >> > It's not clear what changes you plan to do (as separate patches or
> >> > otherwise). If it's possible that UUIDs get removed, we should handle
> >> > that.
> >> >
> >>
> >> Sorry, I wasn't clear enough. It's possible that a remote device
> >> removes a UUID from its list of supported services. But in BlueZ we'll
> >> never detect this situation, so the UUIDs property will not change in
> >> the device object. What will happen is that if BlueZ tries to connect
> >> to that service the connection will fail. I agree it's an
> >> implementation detail, but if we write code to handle UUIDs removal it
> >> will never be exercised with the current BlueZ implementation.
> >
> > What's the problem with having untested code in this case? There is a
> > risk that the code doesn't work if BlueZ some day starts to support
> > removing UUIDs? If we don't implement support for it, then we know for
> > sure that the code won't work.
> >
>
> The problem is carrying/maintaining dead code, making the codebase
> bigger (thus more complex) for no benefit, and the high chance of
> wasting energy (in case BlueZ never supports it or the interface
> changes before that). Right now PulseAudio is the only way to use
> audio devices that is supported by BlueZ, so if something changes in
> BlueZ that affects PulseAudio, it's responsibility of the BlueZ
> developers to update pulse accordingly. So I prefer to wait until that
> happens instead of trying to predict future changes in BlueZ.
Ok then, let's not support removing UUIDs. Just add a comment about
this.
--
Tanu
