[pulseaudio-discuss] [RFC v0 10/15] bluetooth: Parse media transport's properties

[Tanu Kaskinen]

On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
> 
> Consider the media transport when a PropertiesChanged signal is
> received.
> 
> Note that the transport might have an owner other than BlueZ, and thus
> the property changes would be emitted from arbitrary senders. For
> performance reasons, the installed match considers the interface name
> where the property has changed.
> 
> It could be possible to install and remove the D-Bus matches dynamically
> when a new owner is registered/unregistered, but filtering based on the
> interface name seems good enough already.
> ---
>  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 3c0fcf7..f8ecb15 100644
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -1087,6 +1087,25 @@ static int transport_parse_property(pa_bluetooth_transport *t, DBusMessageIter *
>      return 0;
>  }
>  
> +static int parse_transport_properties(pa_bluetooth_transport *t, DBusMessageIter *i) {
> +    DBusMessageIter element_i;
> +
> +    dbus_message_iter_recurse(i, &element_i);
> +
> +    while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
> +        DBusMessageIter dict_i;
> +
> +        dbus_message_iter_recurse(&element_i, &dict_i);
> +
> +        if (transport_parse_property(t, &dict_i) < 0)
> +            return -1;

It would be better to not return here. Just store the fact that parsing
some property failed in some boolean and continue parsing the next
property, so that any valid property changes aren't missed (the same
applies to parse_device_properties()).

I think parse_transport_properties() could also be used in
endpoint_set_configuration().

A suggestion for a general policy for property parse error handling:

 * Whenever parsing a property fails, print an error message to the log
(already done, I think).

 * When parsing the initial properties of a new object, fail hard (i.e.
don't create the object) if all non-optional properties (that we're
interested in) are not received, or if parsing any such property fails.

 * When parsing a PropertiesChanged signal, never fail if there are
parse errors, just pretend that the problematic properties didn't
change.

 * When parsing any optional property, never fail on parse errors, just
pretend that the property isn't set (when parsing the initial
properties) or that the property value didn't change (when parsing a
PropertiesChanged signal).

> +
> +        dbus_message_iter_next(&element_i);
> +    }
> +
> +    return 0;
> +}
> +
>  static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *userdata) {
>      DBusError err;
>      pa_bluetooth_discovery *y;
> @@ -1303,6 +1322,14 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
>  
>              if (parse_device_properties(d, &arg_i) < 0)
>                  return -1;
> +        } else if (pa_streq(interface, "org.bluez.MediaTransport1")) {
> +            pa_bluetooth_transport *t;
> +
> +            if (!(t = pa_hashmap_get(y->transports, dbus_message_get_path(m))))
> +                return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +
> +            if (parse_transport_properties(t, &arg_i) < 0)
> +                return -1;

-1 is not a valid return value in this context, and you can ignore parse
failures anyway here.

-- 
Tanu