[pulseaudio-discuss] [RFC v0 2/3] bluetooth: Replace deprecated ListAdapters()

[Mikel Astiz]

Hi Tanu,

On Thu, Jul 26, 2012 at 7:59 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Wed, 2012-07-25 at 16:29 +0200, Mikel Astiz wrote:
>> +static int parse_manager_property(pa_bluetooth_discovery *y, DBusMessageIter *i) {
>> +    const char *key;
>> +    DBusMessageIter variant_i;
>> +
>> +    pa_assert(y);
>> +
>> +    key = check_variant_property(i);
>> +    if (key == NULL)
>> +     return -1;
>
> The same tab (see my previous message) spotted here too.

Sorry for that.

>> +
>> +    dbus_message_iter_recurse(i, &variant_i);
>> +
>> +    switch (dbus_message_iter_get_arg_type(&variant_i)) {
>> +
>> +        case DBUS_TYPE_ARRAY: {
>> +
>> +            DBusMessageIter ai;
>> +            dbus_message_iter_recurse(&variant_i, &ai);
>> +
>> +            if (dbus_message_iter_get_arg_type(&ai) == DBUS_TYPE_OBJECT_PATH &&
>> +                pa_streq(key, "Adapters")) {
>> +
>> +                while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
>> +                    const char *value;
>> +
>> +                    dbus_message_iter_get_basic(&ai, &value);
>> +
>> +                    found_adapter(y, value);
>> +
>> +                    if (!dbus_message_iter_next(&ai))
>> +                        break;
>
> No need to check dbus_message_iter_next() return value here. The loop
> will terminate anyway, because dbus_message_iter_get_arg_type() will
> return DBUS_TYPE_INVALID.

I will fix this and send a separate patch fixing other parts of the
code were this check is done unnecessarily.

>> @@ -430,7 +472,8 @@ static void get_properties_reply(DBusPendingCall *pending, void *userdata) {
>>      /* We don't use p->call_data here right-away since the device
>>       * might already be invalidated at this point */
>>
>> -    if (!(d = pa_hashmap_get(y->devices, dbus_message_get_path(p->message))))
>> +    d = pa_hashmap_get(y->devices, dbus_message_get_path(p->message));
>> +    if (d == NULL && !dbus_message_has_interface(p->message, "org.bluez.Manager"))
>>          return;
>>
>>      pa_assert(p->call_data == d);
>> @@ -469,7 +512,11 @@ static void get_properties_reply(DBusPendingCall *pending, void *userdata) {
>>
>>              dbus_message_iter_recurse(&element_i, &dict_i);
>>
>> -            if (dbus_message_has_interface(p->message, "org.bluez.Device")) {
>> +            if (dbus_message_has_interface(p->message, "org.bluez.Manager")) {
>> +                if (parse_manager_property(y, &dict_i) < 0)
>> +                    goto finish;
>> +
>> +            } else if (dbus_message_has_interface(p->message, "org.bluez.Device")) {
>>                  if (parse_device_property(y, d, &dict_i) < 0)
>>                      goto finish;
>>
>> @@ -501,7 +548,8 @@ static void get_properties_reply(DBusPendingCall *pending, void *userdata) {
>>      }
>>
>>  finish:
>> -    run_callback(y, d, FALSE);
>> +    if (d != NULL)
>> +        run_callback(y, d, FALSE);
>
> We don't want to call run_callback() when we have processed Manager
> properties, but the "d != NULL" check doesn't check that. It checks that
> did the message come from an object path that has previously been
> associated with a device. If we get Manager properties from a device
> object, then run_callback() will be called. It of course also means that
> bluetoothd is broken, but we should protect us against broken
> bluetoothd.
>

My proposal here is to change the way d is initialized: we can search
the hashmap only if the interface is not Manager.

Second version of the patches soon.

Cheers,
Mikel