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

[Tanu Kaskinen]

On Tue, 2013-01-15 at 09:29 +0100, Mikel Astiz wrote:
> 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.

I'm fine with either approach: give up all trust on bluez, or just log
an error and pretend that nothing happened. What I don't like is to
cause some collateral damage (such as ignoring valid property changes if
one property is invalid) while otherwise pretending that bluetooth is
still working.

If we'd go with the "give up all trust on bluez" alternative, then it
would be good to change the error handling policy everywhere in the
bluetooth code. That is, whenever unexpected behavior from bluez is
encountered, unload everything. (I wouldn't unload
module-bluetooth-discovery, though. Instead, just remove all devices
from pa_bluetooth_discovery and wait for bluetoothd restart, hoping that
the new bluetoothd process is a new version with the bug fixed.)

> > 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).

I was talking about the situation where a property changes its value,
when it's supposed to stay constant, while you seem to be talking about
a different problem. For non-optional constant properties your proposal
works well enough, because parse_foo_property() can figure out whether
we're parsing the initial properties or changed properties by checking
whether the property has already been set. But for optional constant
properties that check doesn't work, if the property wasn't set during
the object initialization.

This is not really a practical issue with the current code base: the
device class might be the only optional property that pulseaudio treats
as a constant, and if that property ever changes, the only bad effect is
that the "device.form_factor" property of the bluetooth card gets out of
date. But as a measure against future bugs, I'd like to have a way for
parse_foo_property() to know whether it's parsing the initial properties
or changed properties. (I'm not demanding this, just saying that I'd
prefer things to be this way.)

-- 
Tanu