[pulseaudio-discuss] [PATCH 37/56] bluetooth: Parse BlueZ 5 device properties

[Tanu Kaskinen]

On Thu, 2013-07-25 at 01:44 -0300, João Paulo Rechi Vita wrote:
> On Mon, Jul 22, 2013 at 7:33 AM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
> >> +static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
> >> +    const char *key;
> >> +    DBusMessageIter variant_i;
> >> +
> >> +    pa_assert(d);
> >> +
> >> +    key = pa_bluetooth_dbus_check_variant_property(i);
> >> +    if (key == NULL) {
> >> +        pa_log_error("Received invalid property for device %s", d->path);
> >> +        return -1;
> >> +    }
> >> +
> >> +    dbus_message_iter_recurse(i, &variant_i);
> >> +
> >> +    switch (dbus_message_iter_get_arg_type(&variant_i)) {
> >> +
> >> +        case DBUS_TYPE_STRING: {
> >> +            const char *value;
> >> +            dbus_message_iter_get_basic(&variant_i, &value);
> >> +
> >> +            if (pa_streq(key, "Alias")) {
> >> +                pa_xfree(d->alias);
> >> +                d->alias = pa_xstrdup(value);
> >> +            } else if (pa_streq(key, "Address")) {
> >> +                if (is_property_change) {
> >> +                    pa_log_error("Device property 'Address' expected to be constant but changed for %s", d->path);
> >> +                    return -1;
> >> +                }
> >> +
> >> +                if (d->address) {
> >> +                    pa_log_error("Device %s: Received a duplicate Address property.", d->path);
> >> +                    return -1;
> >> +                }
> >> +
> >> +                d->address = pa_xstrdup(value);
> >> +            }
> >> +
> >> +            pa_log_debug("%s: %s", key, value);
> >
> > Please use proper sentences in the log. Or is this a temporary log
> > message that you just forgot to remove from the final patch?
> >
> 
> This is usually received when the device is created, together with a
> lot of other properties, so I think is better to minimize verbosity
> here. What I'll change to improve this scenario is add a log in the
> beginning of the function with the device path.

I would actually prefer to drop the message altogether. If the property
is interesting to us, its value will probably appear elsewhere anyway
(when the property changes, a change message should be printed, and when
receiving the initial properties, the properties should be printed once
the parsing is complete). This will also print properties that aren't
interesting to us, which just adds noise, and it's not even consistent,
because only string and uint32 properties will be printed.

> >> +
> >> +            pa_log_debug("%s: %d", key, value);
> >> +            break;
> >> +        }
> >> +
> >> +        case DBUS_TYPE_ARRAY: {
> >> +            DBusMessageIter ai;
> >> +            dbus_message_iter_recurse(&variant_i, &ai);
> >> +
> >> +            if (dbus_message_iter_get_arg_type(&ai) == DBUS_TYPE_STRING && pa_streq(key, "UUIDs")) {
> >> +                while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
> >> +                    pa_bluetooth_uuid *node;
> >> +                    const char *value;
> >> +
> >> +                    dbus_message_iter_get_basic(&ai, &value);
> >> +
> >> +                    if (pa_bluetooth_uuid_has(d->uuids, value)) {
> >> +                        dbus_message_iter_next(&ai);
> >> +                        continue;
> >> +                    }
> >> +
> >> +                    node = pa_bluetooth_uuid_new(value);
> >> +                    PA_LLIST_PREPEND(pa_bluetooth_uuid, d->uuids, node);
> >> +
> >> +                    pa_log_debug("%s: %s", key, value);
> >> +                    dbus_message_iter_next(&ai);
> >
> > This code only adds UUIDs to pa_bluetooth_device, never removes them. Is
> > this intentional? If so, I think a comment explaining the rationale
> > would be good.
> >
> > There aren't any hooks that would be fired when the properties (Alias,
> > Class, UUIDs) change. Perhaps you implement them in some later patches?
> >
> 
> This is used to create card profiles on load of module-bluez5-device.
> It is possible (although not likely) that a device change the
> implemented UUIDs during the connection lifetime. Alias can change,
> but I think this should be handled in a separate patch (I've not
> written that yet).

It's not clear what changes you plan to do (as separate patches or
otherwise). If it's possible that UUIDs get removed, we should handle
that.

> >> @@ -430,8 +604,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
> >>
> >>              pa_log_debug("Device %s found", path);
> >>
> >> -            pa_bluetooth_discovery_create_device(y, path);
> >> -            /* TODO: parse device properties */
> >> +            d = pa_bluetooth_discovery_create_device(y, path);
> >> +            parse_device_properties(d, &iface_i, false);
> >
> > If parsing the device properties fails, we shouldn't create the device.
> >
> 
> parse_device_properties() will fail even if the parsing of an optional
> property fails, but the device is still fully usable, so we should
> still create the device in this case.

That's a matter of taste, and my taste is to fail if bluetoothd
misbehaves in order to catch bugs earlier.

> To differentiate between these
> two situations there is the device_info_valid field. Maybe we can
> review the necessity of the device_info_valid field, but since this
> affects the modules working logic and may introduce bugs I would
> prefer to change that in a latter moment to minimize the impact.

Good point, so the correct way to fail would be to set device_info_valid
to -1.

-- 
Tanu