[pulseaudio-discuss] [RFC v1 04/14] bluetooth: Support Properties.PropertiesChanged signal

[Mikel Astiz]

Hi Tanu,

On Mon, Jan 14, 2013 at 9:18 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Mon, 2013-01-14 at 16:41 +0100, Mikel Astiz wrote:
>> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>>
>> Install matches for signal Properties.PropertiesChanged and process the
>> properties corresponding to the tracked devices.
>> ---
>>  src/modules/bluetooth/bluetooth-util.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>> index 9abd264..2073418 100644
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -1239,6 +1239,33 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
>>          }
>>
>>          return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>> +    } else if (dbus_message_is_signal(m, "org.freedesktop.DBus.Properties", "PropertiesChanged")) {
>> +        DBusMessageIter arg_i;
>> +        const char *interface;
>> +
>> +        if (y->version != BLUEZ_VERSION_5)
>> +            return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; /* No reply received yet from GetManagedObjects */
>> +
>> +        if (!dbus_message_iter_init(m, &arg_i) || !pa_streq(dbus_message_get_signature(m), "sa{sv}as")) {
>> +            pa_log("Invalid signature found in PropertiesChanged");
>> +            goto fail;
>> +        }
>> +
>> +        dbus_message_iter_get_basic(&arg_i, &interface);
>> +
>> +        pa_assert_se(dbus_message_iter_next(&arg_i));
>> +        pa_assert(dbus_message_iter_get_arg_type(&arg_i) == DBUS_TYPE_ARRAY);
>> +
>> +        if (pa_streq(interface, "org.bluez.Device1")) {
>> +            pa_bluetooth_device *d;
>> +
>> +            if (!(d = pa_hashmap_get(y->devices, dbus_message_get_path(m))))
>> +                return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; /* Device not being tracked */
>> +
>> +            parse_device_properties(d, &arg_i);
>
> parse_device_properties() will stop immediately if any parse error
> occurs, so we may miss valid property changes. I suggest that

parse_device_property() should only fail if BlueZ is buggy and not
following the D-Bus interface it's supposed to (note for example that
unknown properties are just ignored). In this case, I see little
interest in parsing other properties, since the behavior would be
undefined. Instead, I suggest the module would have to be unloaded,
but that might be admittedly too strict in the long term.

> parse_foo_properties() would take a parameter indicating whether we are
> parsing the initial properties or changed properties, and if we are
> parsing changed properties, parse errors would be ignored. That
> parameter would be also useful for handling properties that are supposed
> to stay constant.

A simpler approach would be to ignore the result of
parse_device_property() inside parse_device_properties(). After all,
the non-optional parameters will be checked anyway (which only has
some relevance during the initial parsing).

As a side note, this reminds me that device_info_is_valid should be
checked while processing property changes, ignoring the signal if
device_info_is_valid != 1.

>
> (I'm sure I already wrote that suggestion in relation to the transport
> property parsing discussion, but it seems that I never sent a mail that
> would have contained the suggestion I wrote.)
>

Cheers,
Mikel