Hi Luiz,
On Tue, Apr 30, 2013 at 6:27 AM, Luiz Augusto von Dentz
<luiz.dentz at gmail.com> wrote:
> 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.
>
The whole series has already been integrated to next and, unless I've
missed something, it does the same thing as your patch.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
