On Sat, Sep 21, 2013 at 5:08 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Wed, 2013-09-18 at 16:17 -0500, jprvita at gmail.com wrote:
>> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> ---
>> src/modules/bluetooth/bluez5-util.c | 176 +++++++++++++++++++++++++++++++++++-
>> src/modules/bluetooth/bluez5-util.h | 5 +
>> 2 files changed, 179 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index 983f707..b0d3864 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -33,6 +33,8 @@
>> #include <pulsecore/refcnt.h>
>> #include <pulsecore/shared.h>
>>
>> +#include "a2dp-codecs.h"
>> +
>> #include "bluez5-util.h"
>>
>> #define BLUEZ_SERVICE "org.bluez"
>> @@ -406,12 +408,182 @@ fail:
>> return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>> }
>>
>> +const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
>> + switch(profile) {
>> + case PA_BLUETOOTH_PROFILE_A2DP_SINK:
>> + return "a2dp_sink";
>> + case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
>> + return "a2dp_source";
>> + case PA_BLUETOOTH_PROFILE_OFF:
>> + return "off";
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> 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;
>> + const uint8_t *config = NULL;
>> + int size = 0;
>> + pa_bluetooth_profile_t p = PA_BLUETOOTH_PROFILE_OFF;
>> + DBusMessageIter args, props;
>> DBusMessage *r;
>>
>> - 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 (pa_streq(key, "UUID")) {
>> + if (var != DBUS_TYPE_STRING) {
>> + pa_log_error("Property %s of wrong type %c", key, (char)var);
>> + goto fail;
>> + }
>> +
>> + dbus_message_iter_get_basic(&value, &uuid);
>> +
>> + endpoint_path = dbus_message_get_path(m);
>> + if (pa_streq(endpoint_path, A2DP_SOURCE_ENDPOINT)) {
>> + if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
>> + p = PA_BLUETOOTH_PROFILE_A2DP_SINK;
>> + } else if (pa_streq(endpoint_path, A2DP_SINK_ENDPOINT)) {
>> + if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
>> + p = PA_BLUETOOTH_PROFILE_A2DP_SOURCE;
>> + }
>> +
>> + if (p == PA_BLUETOOTH_PROFILE_OFF) {
>> + pa_log_error("UUID %s of transport %s incompatible with endpoint %s", uuid, path, endpoint_path);
>> + goto fail;
>> + }
>> + } else if (pa_streq(key, "Device")) {
>> + if (var != DBUS_TYPE_OBJECT_PATH) {
>> + pa_log_error("Property %s of wrong type %c", key, (char)var);
>> + goto fail;
>> + }
>> +
>> + dbus_message_iter_get_basic(&value, &dev_path);
>> + } else if (pa_streq(key, "Configuration")) {
>> + DBusMessageIter array;
>> + a2dp_sbc_t *c;
>> +
>> + if (var != DBUS_TYPE_ARRAY) {
>> + pa_log_error("Property %s of wrong type %c", key, (char)var);
>> + goto fail;
>> + }
>> +
>> + dbus_message_iter_recurse(&value, &array);
>> + var = dbus_message_iter_get_arg_type(&array);
>> + if (var != DBUS_TYPE_BYTE) {
>> + pa_log_error("%s is an array of wrong type %c", key, (char)var);
>> + goto fail;
>> + }
>> +
>> + dbus_message_iter_get_fixed_array(&array, &config, &size);
>> + if (size != sizeof(a2dp_sbc_t)) {
>> + pa_log_error("Properties array of invalid size");
>
> "Properties array" is not very accurate choice of words. "Configuration
> array" or "Configuration blob" would be better.
>
Ok.
>> + goto fail;
>> + }
>> +
>> + c = (a2dp_sbc_t *) config;
>> +
>> + if (c->frequency != SBC_SAMPLING_FREQ_16000 && c->frequency != SBC_SAMPLING_FREQ_32000 &&
>> + c->frequency != SBC_SAMPLING_FREQ_44100 && c->frequency != SBC_SAMPLING_FREQ_48000) {
>> + pa_log_error("Invalid sampling frequency in configuration");
>> + goto fail;
>> + }
>> +
>> + if (c->channel_mode != SBC_CHANNEL_MODE_MONO && c->channel_mode != SBC_CHANNEL_MODE_DUAL_CHANNEL &&
>> + c->channel_mode != SBC_CHANNEL_MODE_STEREO && c->channel_mode != SBC_CHANNEL_MODE_JOINT_STEREO) {
>> + pa_log_error("Invalid channel mode in configuration");
>> + goto fail;
>> + }
>> +
>> + if (c->allocation_method != SBC_ALLOCATION_SNR && c->allocation_method != SBC_ALLOCATION_LOUDNESS) {
>> + pa_log_error("Invalid allocation method in configuration");
>> + goto fail;
>> + }
>> +
>> + if (c->subbands != SBC_SUBBANDS_4 && c->subbands != SBC_SUBBANDS_8) {
>> + pa_log_error("Invalid SBC subbands in configuration");
>> + goto fail;
>> + }
>> +
>> + if (c->block_length != SBC_BLOCK_LENGTH_4 && c->block_length != SBC_BLOCK_LENGTH_8 &&
>> + c->block_length != SBC_BLOCK_LENGTH_12 && c->block_length != SBC_BLOCK_LENGTH_16) {
>> + pa_log_error("Invalid block length in configuration");
>> + goto fail;
>> + }
>
> My earlier criticism is still valid: "It would better to do the config
> validation later, after checking that the UUID matches the endpoint
> type, because if the UUID doesn't match, then the config is probably not
> what you expect it to be either. Checking the config too early may
> result in less clear error messages (it's better to complain about UUID
> mismatch than errors in the config)."
>
> You moved the UUID check to happen earlier, but there's still no
> guarantee that the UUID check happens before the configuration
> validation, because the "Configuration" property may be located at an
> earlier position in the D-Bus message than the "UUID" property.
>
Whichever property arrives first will print an error message and abort
parsing the other properties. And it may very well be true that the
UUID doesn't match and the configuration is valid, since the format
for A2DP sink and source configuration blob is the same. I don't think
there is any gain in making the change you propose here.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
