[pulseaudio-discuss] [PATCH 3/5] bluetooth: Fix notifying about new devices

[João Paulo Rechi Vita]

On Sun, Sep 29, 2013 at 12:49 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> Normally devices are created and their properties are parsed before
> creating any transports for the device, and the
> DEVICE_CONNECTION_CHANGED hook is fired when the first transport is
> created. It's possible, however, that a transport is created before
> the device that it belongs to, and in this case
> DEVICE_CONNECTION_CHANGED should be fired when the properties have
> been successfully parsed for the device. The old code didn't fire the
> hook at this situation, and this patch fixes that.
> ---
>  src/modules/bluetooth/bluez5-util.c | 43 +++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index eaff4b1..c7e934e 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -438,14 +438,29 @@ static void device_remove(pa_bluetooth_discovery *y, const char *path) {
>      }
>  }
>
> +static void set_device_info_valid(pa_bluetooth_device *device, int valid) {
> +    bool old_any_connected;
> +
> +    pa_assert(device);
> +    pa_assert(valid == -1 || valid == 0 || valid == 1);
> +
> +    if (valid == device->device_info_valid)
> +        return;
> +
> +    old_any_connected = pa_bluetooth_device_any_transport_connected(device);
> +    device->device_info_valid = valid;
> +
> +    if (pa_bluetooth_device_any_transport_connected(device) != old_any_connected)
> +        pa_hook_fire(&device->discovery->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED], device);
> +}
> +

I like the idea of having a helper function here. But wouldn't be
better to factor the occurences of "device->device_info_valid = X"
into set_device_info_valid() in one commit and the
DEVICE_CONNECTION_CHANGED hook fire, together with the respective
checks, in a separate commit? I think they are two different
modifications, so the would belong to separate commits, making the
history cleaner and easier to read. If for some reason you don't want
to separate them it's ok for me (just please explain me why you think
so).

>  static void device_remove_all(pa_bluetooth_discovery *y) {
>      pa_bluetooth_device *d;
>
>      pa_assert(y);
>
>      while ((d = pa_hashmap_steal_first(y->devices))) {
> -        d->device_info_valid = -1;
> -        pa_hook_fire(&y->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED], d);
> +        set_device_info_valid(d, -1);
>          device_free(d);
>     }
>  }
> @@ -607,7 +622,6 @@ static void parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, bo
>
>  static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
>      DBusMessageIter element_i;
> -    int ret = 0;
>
>      dbus_message_iter_recurse(i, &element_i);
>
> @@ -621,12 +635,17 @@ static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i, b
>
>      if (!d->address || !d->adapter_path || !d->alias) {
>          pa_log_error("Non-optional information missing for device %s", d->path);
> -        d->device_info_valid = -1;
> +        set_device_info_valid(d, -1);
>          return -1;
>      }
>
> -    d->device_info_valid = 1;
> -    return ret;
> +    if (!is_property_change && d->adapter)
> +        set_device_info_valid(d, 1);
> +
> +    /* If d->adapter is NULL, device_info_valid will be left as 0, and updated
> +     * after all interfaces have been parsed. */
> +
> +    return 0;
>  }
>
>  static void parse_adapter_properties(pa_bluetooth_adapter *a, DBusMessageIter *i, bool is_property_change) {
> @@ -804,14 +823,20 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>          dbus_message_iter_next(&element_i);
>      }
>
> -    PA_HASHMAP_FOREACH(d, y->devices, state)
> +    PA_HASHMAP_FOREACH(d, y->devices, state) {
> +        if (d->device_info_valid != 0)
> +            continue;
> +
>          if (!d->adapter && d->adapter_path) {
>              d->adapter = pa_hashmap_get(d->discovery->adapters, d->adapter_path);
> +
>              if (!d->adapter) {
>                  pa_log_error("Device %s is child of nonexistent adapter %s", d->path, d->adapter_path);
> -                d->device_info_valid = -1;
> -            }
> +                set_device_info_valid(d, -1);
> +            } else
> +                set_device_info_valid(d, 1);
>          }
> +    }
>
>      return;
>  }

Did you manage to test these changes?

-- 
João Paulo Rechi Vita
http://about.me/jprvita