(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()?
> + 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.
> + }
> +
> + 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?
> + }
> +
> + 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().
> + 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?
> + 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.
> + 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.
> +
> + 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.
--
Tanu
