On Mon, Apr 29, 2013 at 1:15 PM, Tanu Kaskinen <tanu.kaskinen at intel.com> wrote:
> On Mon, 2013-04-29 at 12:51 -0300, Jo?o Paulo Rechi Vita wrote:
>> 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.
>
> It is probably a bad idea to leave the device description empty. Clients
> (and modules) that use it are expected to do something sensible even if
> it's missing, but it's better to set a good default already at the
> server side (but in this case there's no need to care, see below).
>
>> > 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.
>
> The handling of non-optional BlueZ properties has been discussed already
> before. As a result, Mikel's BlueZ 5 patches make the device unusable if
> mandatory properties are missing, if I've understood correctly.
>
> The reason why I prefer to make the device unusable even in case of this
> sort of recoverable bugs is that, as a general rule, bugs should be
> fixed, not worked around. If bugs are hidden, they won't get fixed.
>
We're not hiding it, it's going to be exposed through the bogus device
description. It seems a matter of personal opinion on how should we
expose this, so we're probably not going to reach an agreement here.
That's fine to me, let's move on.
