[pulseaudio-discuss] [PATCH] bluetooth: Use 'Alias' instead of 'Name'

[João Paulo Rechi Vita]

On Wed, Apr 24, 2013 at 3:29 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
> Hi João Paulo,
>
> On Tue, Apr 23, 2013 at 10:48 PM,  <jprvita at gmail.com> wrote:
>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> According to the BlueZ API documentation the 'Alias' property should be
>> preffered over the 'Name' property. Also, if the device doesn't have a
>> remote name the 'Name' property may not be set, but the 'Alias' property
>> will always be.
>
> Can you point to the documentation you refer to? 'Alias' is a
> user-modifiable property, while 'Name' is what the remote device
> reports. They just have different purposes, so I don't think one of
> them is preferred over the other as a general rule.
>

doc/device-api.txt in BlueZ repository explicitly states that 'Alias'
should be preferred over 'Name'. If you go back in history you'll see
that it was first mentioned before BlueZ 4.0.

>> ---
>>  src/modules/bluetooth/bluetooth-util.c          | 7 +------
>>  src/modules/bluetooth/bluetooth-util.h          | 1 -
>>  src/modules/bluetooth/module-bluetooth-device.c | 4 ++--
>>  3 files changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>> index c15ecd1..b0ee62c 100644
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -179,7 +179,6 @@ static pa_bluetooth_device* device_new(pa_bluetooth_discovery *discovery, const
>>
>>      d->device_info_valid = 0;
>>
>> -    d->name = NULL;
>>      d->path = pa_xstrdup(path);
>>      d->paired = -1;
>>      d->alias = NULL;
>> @@ -228,7 +227,6 @@ static void device_free(pa_bluetooth_device *d) {
>>          uuid_free(u);
>>      }
>>
>> -    pa_xfree(d->name);
>>      pa_xfree(d->path);
>>      pa_xfree(d->alias);
>>      pa_xfree(d->address);
>> @@ -364,10 +362,7 @@ static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, boo
>>              const char *value;
>>              dbus_message_iter_get_basic(&variant_i, &value);
>>
>> -            if (pa_streq(key, "Name")) {
>> -                pa_xfree(d->name);
>> -                d->name = pa_xstrdup(value);
>> -            } else if (pa_streq(key, "Alias")) {
>> +            if (pa_streq(key, "Alias")) {
>>                  pa_xfree(d->alias);
>>                  d->alias = pa_xstrdup(value);
>>              } else if (pa_streq(key, "Address")) {
>> diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h
>> index 3361b0f..1040d5b 100644
>> --- a/src/modules/bluetooth/bluetooth-util.h
>> +++ b/src/modules/bluetooth/bluetooth-util.h
>> @@ -120,7 +120,6 @@ struct pa_bluetooth_device {
>>      int device_info_valid;      /* 0: no results yet; 1: good results; -1: bad results ... */
>>
>>      /* Device information */
>> -    char *name;
>
> I don't see the removal as part of this patch, regardless of what you
> do with pa_proplist_sets() below.
>

Ok, I can split it in a separate patch.

>>      char *path;
>>      pa_bluetooth_transport *transports[PA_BLUETOOTH_PROFILE_COUNT];
>>      int paired;
>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>> index cd0a515..ddf658f 100644
>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> @@ -2237,7 +2237,7 @@ static int add_card(struct userdata *u) {
>>      data.driver = __FILE__;
>>      data.module = u->module;
>>
>> -    n = pa_bluetooth_cleanup_name(device->name);
>> +    n = pa_bluetooth_cleanup_name(device->alias);
>>      pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, n);
>>      pa_xfree(n);
>>      pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, device->address);
>> @@ -2250,7 +2250,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.name", device->alias);
>
> What's the purpose of replacing this existing property instead of
> adding 'bluez.alias'?
>

As mentioned before, we should only use the 'Alias' value and not the
'Name' one, so there is no point in storing it. I'm not sure when
(regarding PulseAudio release versions) we can change the property
from 'bluez.name' to 'bluez.alias', since IIRC these properties are
exposed to PulseAudio clients.

> Btw, I'm not seeing any user of this property so I'm wondering why
> you're interested in such a change. Is it for development and testing
> purposes?
>

As said previously, IIRC these properties are available for PulseAudio
clients as a source of information about the card. But what led me
into this was a misbehavior introduced by the patch 2/16 on your
series, found while doing PTS testing. There on
parse_device_properties() you're treating this property as mandatory,
but if the remote name is not set (which is the case for PTS) the
'Name' property is not present on the device object in BlueZ and the
device information in PulseAudio is assumed invalid. That lead to
problems when trying to associate a transport with a device.

>>      data.name = get_name("card", u->modargs, device->address, &b);
>>      data.namereg_fail = b;
>>
>> --
>
> Cheers,
> Mikel
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss