[pulseaudio-discuss] [RFC v0 03/15] bluetooth: Parse the tree returned by ObjectManager

[Tanu Kaskinen]

On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -366,7 +366,7 @@ static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i) {
>  
>      dbus_message_iter_recurse(i, &variant_i);
>  
> -/*     pa_log_debug("Parsing property org.bluez.Device.%s", key); */
> +/*    pa_log_debug("Parsing device property '%s'", key); */

Not that this bothers me much, but I'd personally remove any commented
out code instead of trying to keep it up to date.

> +static int parse_device_properties(pa_bluetooth_device *d, 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 (parse_device_property(d, &dict_i) < 0)
> +            return -1;
> +
> +        dbus_message_iter_next(&element_i);
> +    }
> +
> +    d->device_info_valid = d->name && d->address;
> +
> +    return 0;

Shouldn't the return value be -1 if device_info_valid is 0? And why do
you only check that the name and address are set, why not other
properties?

> +}
> +
> +static int parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessageIter *dict_i) {
> +    DBusMessageIter element_i;
> +    const char *path;
> +
> +    if (dbus_message_iter_get_arg_type(dict_i) != DBUS_TYPE_OBJECT_PATH) {
> +        pa_log("Expected D-Bus object path in GetManagedObjects() dictionary.");
> +        return -1;
> +    }
> +
> +    dbus_message_iter_get_basic(dict_i, &path);
> +
> +    if (!dbus_message_iter_next(dict_i) || dbus_message_iter_get_arg_type(dict_i) != DBUS_TYPE_ARRAY) {
> +        pa_log("D-Bus interfaces missing for object %s", path);
> +        return -1;
> +    }
> +
> +    dbus_message_iter_recurse(dict_i, &element_i);
> +
> +    while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
> +        DBusMessageIter iface_i;
> +        const char *interface;
> +
> +        dbus_message_iter_recurse(&element_i, &iface_i);
> +
> +        if (dbus_message_iter_get_arg_type(&iface_i) != DBUS_TYPE_STRING) {
> +            pa_log("Expected interface name in ObjectManager dictionary.");
> +            return -1;
> +        }
> +
> +        dbus_message_iter_get_basic(&iface_i, &interface);
> +
> +        if (!dbus_message_iter_next(&iface_i) || dbus_message_iter_get_arg_type(&iface_i) != DBUS_TYPE_ARRAY) {
> +            pa_log("Interface properties missing for %s interface %s", path, interface);
> +            return -1;
> +        }
> +
> +        if (pa_streq(interface, "org.bluez.Adapter1")) {
> +            pa_log_debug("Adapter %s found", path);
> +            register_adapter_endpoints(y, path);
> +        } else if (pa_streq(interface, "org.bluez.Device1")) {
> +            pa_bluetooth_device *d;
> +
> +            if (pa_hashmap_get(y->devices, path)) {
> +                pa_log("Found duplicated D-Bus address for device %s", path);

Nitpick: s/address/object path/

> +                return -1;
> +            }
> +
> +            pa_log_debug("Device %s found", path);
> +
> +            d = device_new(y, path);
> +            pa_hashmap_put(y->devices, d->path, d);
> +
> +            /* FIXME: BlueZ 5 doesn't support the old Audio interface, and thus it's not possible to know if any audio
> +               profile is about to be connected. This can introduce regressions with modules such as module-card-restore */

Please wrap comments, at least multi-line comments, at 80 chars. (This
rule didn't seem to be documented in the coding style wiki page... now
it is.)

Btw, is it in your plans to fix module-card-restore?

> +            d->audio_state = PA_BT_AUDIO_STATE_DISCONNECTED;
> +
> +            if (parse_device_properties(d, &iface_i) < 0)
> +                return -1;

The broken device is now left in y->devices. I think it would be better
to only add the device to y->devices if parsing the properties succeeds,
and otherwise free the device here.

> @@ -896,6 +993,23 @@ static void get_managed_objects_reply(DBusPendingCall *pending, void *userdata)
>      pa_log_info("D-Bus ObjectManager detected so assuming BlueZ version 5.");
>      y->version = BLUEZ_VERSION_5;
>  
> +    if (!dbus_message_iter_init(r, &arg_i) || dbus_message_iter_get_arg_type(&arg_i) != DBUS_TYPE_ARRAY) {
> +        pa_log("GetManagedObjects() result is not a dictionary.");
> +        goto finish;
> +    }

libdbus guarantees that the message contents match the message
signature, so rather than checking the message argument types separately
for each argument, you can just check that the message signature is what
you expect ("a{oa{sa{sv}}}" in this case). Only when you start parsing
variants it's necessary to check the types again.

> +
> +    dbus_message_iter_recurse(&arg_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 (parse_interfaces_and_properties(y, &dict_i) < 0)
> +            goto finish;

I think you should either ignore it here if parsing the properties of an
object fails, or fail properly (that is, remove all objects and do
nothing until bluetoothd is restarted). Stopping the discovery process
half-way if one object is broken doesn't seem like a good error handling
approach.

-- 
Tanu