On Fri, Aug 9, 2013 at 1:06 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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.
>
Ok
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
