Hi Tanu,
On Sun, Dec 30, 2012 at 2:21 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -366,7 +366,7 @@ static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i) {
>>
>> dbus_message_iter_recurse(i, &variant_i);
>>
>> -/* pa_log_debug("Parsing property org.bluez.Device.%s", key); */
>> +/* pa_log_debug("Parsing device property '%s'", key); */
>
> Not that this bothers me much, but I'd personally remove any commented
> out code instead of trying to keep it up to date.
>
>> +static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i) {
>> + DBusMessageIter element_i;
>> +
>> + dbus_message_iter_recurse(i, &element_i);
>> +
>> + while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>> + DBusMessageIter dict_i;
>> +
>> + dbus_message_iter_recurse(&element_i, &dict_i);
>> +
>> + if (parse_device_property(d, &dict_i) < 0)
>> + return -1;
>> +
>> + dbus_message_iter_next(&element_i);
>> + }
>> +
>> + d->device_info_valid = d->name && d->address;
>> +
>> + return 0;
>
> Shouldn't the return value be -1 if device_info_valid is 0? And why do
Makes sense. Furthermore, device_info_valid should be set to -1 in this case.
> you only check that the name and address are set, why not other
> properties?
I chose these two because of being non-optional in both BlueZ 4 and 5
and are at the same time being used by PA. We could also check the
alias, paired and trusted properties.
>> +}
>> +
>> +static int parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessageIter *dict_i) {
>> + DBusMessageIter element_i;
>> + const char *path;
>> +
>> + if (dbus_message_iter_get_arg_type(dict_i) != DBUS_TYPE_OBJECT_PATH) {
>> + pa_log("Expected D-Bus object path in GetManagedObjects() dictionary.");
>> + return -1;
>> + }
>> +
>> + dbus_message_iter_get_basic(dict_i, &path);
>> +
>> + if (!dbus_message_iter_next(dict_i) || dbus_message_iter_get_arg_type(dict_i) != DBUS_TYPE_ARRAY) {
>> + pa_log("D-Bus interfaces missing for object %s", path);
>> + return -1;
>> + }
>> +
>> + dbus_message_iter_recurse(dict_i, &element_i);
>> +
>> + while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>> + DBusMessageIter iface_i;
>> + const char *interface;
>> +
>> + dbus_message_iter_recurse(&element_i, &iface_i);
>> +
>> + if (dbus_message_iter_get_arg_type(&iface_i) != DBUS_TYPE_STRING) {
>> + pa_log("Expected interface name in ObjectManager dictionary.");
>> + return -1;
>> + }
>> +
>> + dbus_message_iter_get_basic(&iface_i, &interface);
>> +
>> + if (!dbus_message_iter_next(&iface_i) || dbus_message_iter_get_arg_type(&iface_i) != DBUS_TYPE_ARRAY) {
>> + pa_log("Interface properties missing for %s interface %s", path, interface);
>> + return -1;
>> + }
>> +
>> + if (pa_streq(interface, "org.bluez.Adapter1")) {
>> + pa_log_debug("Adapter %s found", path);
>> + register_adapter_endpoints(y, path);
>> + } else if (pa_streq(interface, "org.bluez.Device1")) {
>> + pa_bluetooth_device *d;
>> +
>> + if (pa_hashmap_get(y->devices, path)) {
>> + pa_log("Found duplicated D-Bus address for device %s", path);
>
> Nitpick: s/address/object path/
>
>> + return -1;
>> + }
>> +
>> + pa_log_debug("Device %s found", path);
>> +
>> + d = device_new(y, path);
>> + pa_hashmap_put(y->devices, d->path, d);
>> +
>> + /* FIXME: BlueZ 5 doesn't support the old Audio interface, and thus it's not possible to know if any audio
>> + profile is about to be connected. This can introduce regressions with modules such as module-card-restore */
>
> Please wrap comments, at least multi-line comments, at 80 chars. (This
> rule didn't seem to be documented in the coding style wiki page... now
> it is.)
OK, I didn't know about this rule.
>
> Btw, is it in your plans to fix module-card-restore?
I gave this a quick try and I found it more complicated than I first
thought. It's not my highest priority so it might take a while until I
could address this issue.
>
>> + d->audio_state = PA_BT_AUDIO_STATE_DISCONNECTED;
>> +
>> + if (parse_device_properties(d, &iface_i) < 0)
>> + return -1;
>
> The broken device is now left in y->devices. I think it would be better
> to only add the device to y->devices if parsing the properties succeeds,
> and otherwise free the device here.
device_info_valid==-1 seems to serve exactly this purpose, so I'd
avoid changing this behavior in this patch and leave it for later.
>
>> @@ -896,6 +993,23 @@ static void get_managed_objects_reply(DBusPendingCall *pending, void *userdata)
>> pa_log_info("D-Bus ObjectManager detected so assuming BlueZ version 5.");
>> y->version = BLUEZ_VERSION_5;
>>
>> + if (!dbus_message_iter_init(r, &arg_i) || dbus_message_iter_get_arg_type(&arg_i) != DBUS_TYPE_ARRAY) {
>> + pa_log("GetManagedObjects() result is not a dictionary.");
>> + goto finish;
>> + }
>
> libdbus guarantees that the message contents match the message
> signature, so rather than checking the message argument types separately
> for each argument, you can just check that the message signature is what
> you expect ("a{oa{sa{sv}}}" in this case). Only when you start parsing
> variants it's necessary to check the types again.
OK.
>
>> +
>> + dbus_message_iter_recurse(&arg_i, &element_i);
>> + while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>> + DBusMessageIter dict_i;
>> +
>> + dbus_message_iter_recurse(&element_i, &dict_i);
>> +
>> + if (parse_interfaces_and_properties(y, &dict_i) < 0)
>> + goto finish;
>
> I think you should either ignore it here if parsing the properties of an
> object fails, or fail properly (that is, remove all objects and do
> nothing until bluetoothd is restarted). Stopping the discovery process
> half-way if one object is broken doesn't seem like a good error handling
> approach.
I'll ignore the error and proceed with the following objects.
Cheers,
Mikel
