Hi Tanu,
On Mon, Apr 29, 2013 at 7:51 PM, Jo?o Paulo Rechi Vita
<jprvita at openbossa.org> wrote:
> 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.
I actually have a different patch for this:
http://gitorious.org/~vudentz/pulseaudio/vudentzs-mainline/commit/876ec26ab283d50d019e2d0232b60c675caa81ac
Basically the Name property is not useful to PA, Name just represent
the name set by the remote stack and it can be not present until it is
resolved, Alias in the other hand is always set, it defaults to BT
address, and the user can overwrite with whatever it wants which
should reflect in the card's name.
--
Luiz Augusto von Dentz
