[pulseaudio-discuss] [PATCH 1/3] bluetooth: Add 'bluez.alias' property

[João Paulo Rechi Vita]

On Mon, Apr 29, 2013 at 11:59 AM, Tanu Kaskinen <tanu.kaskinen at intel.com> wrote:
> On Mon, 2013-04-29 at 11:49 -0300, João Paulo Rechi Vita wrote:
>> On Mon, Apr 29, 2013 at 11:38 AM, Tanu Kaskinen <tanu.kaskinen at intel.com> wrote:
>> > On Mon, 2013-04-29 at 16:20 +0200, Mikel Astiz wrote:
>> >> On Mon, Apr 29, 2013 at 4:01 PM, Tanu Kaskinen <tanu.kaskinen at intel.com> wrote:
>> >> > On Fri, 2013-04-26 at 12:30 -0300, jprvita at gmail.com wrote:
>> >> >>  src/modules/bluetooth/module-bluetooth-device.c | 1 +
>> >> >>  1 file changed, 1 insertion(+)
>> >> >>
>> >> >> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>> >> >> index c877df2..8695c80 100644
>> >> >> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> >> >> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> >> >> @@ -2243,6 +2243,7 @@ static int add_card(struct userdata *u) {
>> >> >>      pa_proplist_sets(data.proplist, "bluez.path", device->path);
>> >> >>      pa_proplist_setf(data.proplist, "bluez.class", "0x%06x", (unsigned) device->class);
>> >> >>      pa_proplist_sets(data.proplist, "bluez.name", device->name);
>> >> >> +    pa_proplist_sets(data.proplist, "bluez.alias", device->alias);
>> >> >
>> >> > device->alias is not guaranteed to be non-NULL, so this code can crash.
>> >> > If the Alias property is non-optional in Bluez, bluetooth-util should
>> >> > ensure that it is indeed always set.
>> >>
>> >> BlueZ 4 API guarantees the property will be present, and therefore
>> >> device->alias should always be set. The obvious exception is BlueZ
>> >> misbehaving, but this condition might hold true for device->name as
>> >> well, which we're not checking either.
>> >>
>> >> I don't think there's any other scenario where the alias could be NULL
>> >> and the name be set outside bluetooth-util.
>> >>
>> >> So while your point is valid, I suggest we fix this in a later patch,
>> >> in a similar way that the BlueZ 5 patchset does (set device_info_valid
>> >> to -1 if any non-optional property is missing).
>> >
>> > Ah, so this is fixed at least for BlueZ 5. Will the same property
>> > parsing code be used for BlueZ 4? If not, does somebody promise to write
>> > the fix for BlueZ 4?
>> >
>>
>> Both in BlueZ 5 and BlueZ 4 the 'Alias' property is not optional, if
>> it's not present it's a bug in BlueZ. I don't think we should protect
>> ourselfs more than the usual asserts before dereferencing pointers.
>
> Yes, but our policy is to use assertions only for guarding against bugs
> in our own process. Assertions are not used for handling bugs in other
> processes, such as bluetoothd.
>
>> If
>> the property is not set but everything else is fine the only thing
>> that will be missing is a value for PA_PROP_DEVICE_DESCRIPTION.
>> Depending on how PulseAudio deals with description-less devices we can
>> either not care or set a default description. Setting the device_info
>> to invalid makes the device unusable.
>
> I'm not sure if you're saying that it's fine to call pa_proplist_sets()
> with a NULL value? It's not fine, it will cause a crash (due to an
> assertion in pa_proplist_sets()).
>

No, I'm asking if it's fine not to call
'pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, "Some
string here")' before calling 'pa_card_new(core, &data)', that is,
never setting the value of PA_PROP_DEVICE_DESCRIPTION. If it's not
fine (that is, we cannot leave PA_PROP_DEVICE_DESCRIPTION empty) I
suggest we set it to a default value, or an empty string, instead of
making the device unusable.

> Making the device unusable is an acceptable (and IMHO the best) way of
> handling BlueZ bugs. But if everybody else thinks that doing it for
> BlueZ 4 is waste of time, I won't insist on doing it.
>

I don't think is a waste of time, and this discussion is also valid
for how this case should be handled in BlueZ 5. I agree the bug is in
BlueZ in this case, but if we can recover without any operational loss
from it, I think it's better to do so.

> --
> Tanu
>
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss