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
