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

[Mikel Astiz]

Hi Tanu,

On Sun, Dec 30, 2012 at 2:21 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> 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

Makes sense. Furthermore, device_info_valid should be set to -1 in this case.

> you only check that the name and address are set, why not other
> properties?

I chose these two because of being non-optional in both BlueZ 4 and 5
and are at the same time being used by PA. We could also check the
alias, paired and trusted 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.)

OK, I didn't know about this rule.

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

I gave this a quick try and I found it more complicated than I first
thought. It's not my highest priority so it might take a while until I
could address this issue.

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

device_info_valid==-1 seems to serve exactly this purpose, so I'd
avoid changing this behavior in this patch and leave it for later.

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

OK.

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

I'll ignore the error and proceed with the following objects.

Cheers,
Mikel