[pulseaudio-discuss] [PATCH] bluetooth: Use 'Alias' instead of 'Name'
Mikel Astiz
mikel.astiz.oss at gmail.com
Thu Apr 25 01:11:37 PDT 2013
Hi João Paulo,
On Thu, Apr 25, 2013 at 9:36 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
> Hi João,
>
> On Wed, Apr 24, 2013 at 7:51 PM, João Paulo Rechi Vita
> <jprvita at openbossa.org> wrote:
>> 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.
>
> I'm still not convinced. These are two different properties, both
> exposed by BlueZ for some reason, and it can be misleading that we
> start using a different terminology inside PA, and map alias->name.
>
> I'm fine encouraging the use of 'bluez.alias' over 'bluez.name', but
> renaming it internally should be avoided IMO.
>
>>
>>>> ---
>>>> 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.
>
> I'd first add bluez.alias and we can discuss afterwards whether
> bluez.name should be dropped. Changing the internal mapping seems
> obscure and error-prone to me, not to mention that we'd be breaking
> the API if any client is actually using this property, strictly
> speaking.
>
>>
>>> 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,
>
> BlueZ's documentation guarantees that this property is always present,
> including 4.99 (which we depend on) and any later version.
>
>> 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.
>
> I also checked the implementation in several BlueZ versions. In BlueZ
> 4, the property seems to be added always, so the issue you mention
> should not exist
>
> Also BlueZ 5 should guarantee (according to the documentation) that
> the property exists, but the implementation says otherwise, starting
> with 826023de. One or the other is wrong, so we have to clarify this
> first (patch sent to BlueZ's mailing list).
This has been confirmed to be a bug in BlueZ 5's documentation and it
has now been fixed.
Based on this, I propose the following steps:
1. Add "bluez.alias" property.
2. Use the alias as PA_PROP_DEVICE_DESCRIPTION.
3. Remove the "bluez.name" property.
This is basically a split version of your patch, except that you're
removing the name from the internal data structure (which IMO is not
necessary).
I believe the first two patches should make it to the stable branch,
and perhaps even the third one.
After this, I'd update the Bluez5 patchset according to the new
scenario where 'Name' is an optional property.
Cheers,
Mikel
More information about the pulseaudio-discuss
mailing list