[pulseaudio-discuss] [PATCH 29/56] bluetooth: Implement org.bluez.MediaEndpoint1.SetConfiguration()

[João Paulo Rechi Vita]

On Fri, Jul 19, 2013 at 7:24 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> (Sorry, I sent the previous mail too early. I hadn't yet finished
> reviewing this patch.)
>
> On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> ---
>>  src/modules/bluetooth/bluez5-util.c | 116 +++++++++++++++++++++++++++++++++++-
>>  src/modules/bluetooth/bluez5-util.h |   3 +
>>  2 files changed, 117 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index 3e6eb1c..a68f8ca 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -350,11 +350,123 @@ fail:
>>  }
>>
>>  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
>> +    pa_bluetooth_discovery *y = userdata;
>> +    pa_bluetooth_device *d;
>> +    pa_bluetooth_transport *t;
>> +    const char *sender, *path, *endpoint_path, *dev_path = NULL, *uuid = NULL;
>> +    uint8_t *config = NULL;
>> +    int size = 0;
>> +    pa_bluetooth_profile_t p = PROFILE_OFF;
>> +    DBusMessageIter args, props;
>>      DBusMessage *r;
>> +    bool old_any_connected;
>>
>> -    pa_assert_se(r = dbus_message_new_error(m, BLUEZ_MEDIA_ENDPOINT_INTERFACE ".Error.NotImplemented",
>> -                                            "Method not implemented"));
>> +    if (!dbus_message_iter_init(m, &args) || !pa_streq(dbus_message_get_signature(m), "oa{sv}")) {
>> +        pa_log_error("Invalid signature for method SetConfiguration()");
>> +        goto fail2;
>> +    }
>> +
>> +    dbus_message_iter_get_basic(&args, &path);
>> +
>> +    if (pa_hashmap_get(y->transports, path)) {
>> +        pa_log_error("Endpoint SetConfiguration(): Transport %s is already configured.", path);
>> +        goto fail2;
>> +    }
>> +
>> +    pa_assert_se(dbus_message_iter_next(&args));
>> +
>> +    dbus_message_iter_recurse(&args, &props);
>> +    if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
>> +        goto fail;
>> +
>> +    /* Read transport properties */
>> +    while (dbus_message_iter_get_arg_type(&props) == DBUS_TYPE_DICT_ENTRY) {
>> +        const char *key;
>> +        DBusMessageIter value, entry;
>> +        int var;
>> +
>> +        dbus_message_iter_recurse(&props, &entry);
>> +        dbus_message_iter_get_basic(&entry, &key);
>> +
>> +        dbus_message_iter_next(&entry);
>> +        dbus_message_iter_recurse(&entry, &value);
>> +
>> +        var = dbus_message_iter_get_arg_type(&value);
>> +
>> +        if (strcasecmp(key, "UUID") == 0) {
>
> What is the reason to use strcasecmp() instead of pa_streq()?
>

This was copied from previous code, I'm changing it to pa_streq.

>> +            if (var != DBUS_TYPE_STRING)
>> +                goto fail;
>> +
>> +            dbus_message_iter_get_basic(&value, &uuid);
>> +        } else if (strcasecmp(key, "Device") == 0) {
>> +            if (var != DBUS_TYPE_OBJECT_PATH)
>> +                goto fail;
>> +
>> +            dbus_message_iter_get_basic(&value, &dev_path);
>> +        } else if (strcasecmp(key, "Configuration") == 0) {
>> +            DBusMessageIter array;
>> +            if (var != DBUS_TYPE_ARRAY)
>> +                goto fail;
>> +
>> +            dbus_message_iter_recurse(&value, &array);
>> +            dbus_message_iter_get_fixed_array(&array, &config, &size);
>
> You should check that it's a byte array before reading the array
> contents.
>

Ok.

>> +        }
>> +
>> +        dbus_message_iter_next(&props);
>> +    }
>> +
>> +    d = pa_bluetooth_discovery_get_device_by_path(y, dev_path);
>> +    if (!d) {
>> +        pa_log_warn("SetConfiguration() received for unknown device %s", dev_path);
>> +        d = pa_bluetooth_discovery_create_device(y, dev_path);
>> +        if (!d)
>> +            goto fail;
>> +        /* If this point is reached the InterfacesAdded signal is probably on it's way */
>
> Can things really happen in this order? If they can, should the device
> created here be removed in the fail section?
>

The point the comments references is when the if branch is not taken,
there is, d is not NULL, so we don't go to the fail section. If we
take the if branch, going to the fail section, no device has been
created, so there is nothing to be removed.

And because of the way the D-Bus helpers are implemented inside BlueZ,
I think it is possible that the SetConfiguration() call arrives before
the InterfacesAdded signal, since signals are always sent in an idler
and method calls are sent right away. If that happens there is no need
to remove the device either, when the InterfacesAdded signal arrives
the device properties will be just updated and the transport will
already be stored in the pa_bluetooth_device struct.

>> +    }
>> +
>> +    endpoint_path = dbus_message_get_path(m);
>> +    if (strcasecmp(endpoint_path, A2DP_SOURCE_ENDPOINT) == 0) {
>
> This surely is an instance where there's no reason to use strcasecmp().
>

Same as above.

>> +        if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0)
>> +            p = PROFILE_A2DP_SINK;
>> +    } else if (strcasecmp(endpoint_path, A2DP_SINK_ENDPOINT) == 0) {
>> +        if (strcasecmp(uuid, A2DP_SINK_UUID) == 0)
>> +            p = PROFILE_A2DP_SOURCE;
>> +    }
>> +
>> +    if (p == PROFILE_OFF) {
>> +        pa_log_warn("UUID %s of transport %s incompatible with endpoint %s", uuid, path, endpoint_path);
>
> Not that this matters much to me, but why warning instead of an error?
>

You're right, it looks more like an error than a warn.

>> +        goto fail;
>> +    }
>> +
>> +    if (d->transports[p] != NULL) {
>> +        pa_log_error("Cannot configure transport %s because profile %d is already used", path, p);
>
> Please convert the profile to a human-readable string in log messages.
>

Ok.

>> +        goto fail2;
>> +    }
>> +
>> +    old_any_connected = pa_bluetooth_device_any_transport_connected(d);
>> +
>> +    sender = dbus_message_get_sender(m);
>> +
>> +    t = pa_bluetooth_transport_new(d, sender, path, p, config, size, bluez5_transport_acquire, bluez5_transport_release, NULL);
>> +    d->transports[p] = t;
>> +
>> +    pa_log_debug("Transport %s available for profile %d", t->path, t->profile);
>
> Same as above.
>

Ok.

>> +
>> +    pa_assert_se(r = dbus_message_new_method_return(m));
>> +    pa_assert_se(dbus_connection_send(pa_dbus_connection_get(y->connection), r, NULL));
>> +    dbus_message_unref(r);
>> +
>> +    if (old_any_connected != pa_bluetooth_device_any_transport_connected(d)) {
>> +        pa_hook_fire(&y->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED], d);
>> +    }
>
> Unnecessary braces.
>

Yes, I've missed that.

-- 
João Paulo Rechi Vita
http://about.me/jprvita