[pulseaudio-discuss] [PATCHv2 42/60] bluetooth: Track devices in module-bluez5-discover

[Tanu Kaskinen]

On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> 
> ---
>  src/modules/bluetooth/module-bluez5-discover.c | 49 ++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c
> index 8409bd3..e782b2e 100644
> --- a/src/modules/bluetooth/module-bluez5-discover.c
> +++ b/src/modules/bluetooth/module-bluez5-discover.c
> @@ -24,6 +24,7 @@
>  #endif
>  
>  #include <pulsecore/core.h>
> +#include <pulsecore/core-util.h>
>  #include <pulsecore/macro.h>
>  #include <pulsecore/module.h>
>  
> @@ -39,9 +40,46 @@ PA_MODULE_LOAD_ONCE(true);
>  struct userdata {
>      pa_module *module;
>      pa_core *core;
> +    pa_hashmap *device_modules;

The hashmap doesn't store modules, it stores device paths. I suggest
name "loaded_device_paths".

> +    pa_hook_slot *device_connection_changed_slot;
>      pa_bluetooth_discovery *discovery;
>  };
>  
> +static pa_hook_result_t device_connection_changed_cb(pa_bluetooth_discovery *y, const pa_bluetooth_device *d, struct userdata *u) {
> +    void *mi;

bool would be a better type (set it to true if pa_hashmap_get() returns
non-NULL). I suggest "module_loaded" as the variable name.

> +
> +    pa_assert(d);
> +    pa_assert(u);
> +
> +    mi = pa_hashmap_get(u->device_modules, d->path);
> +
> +    if (mi && !pa_bluetooth_device_any_transport_connected(d)) {
> +        /* disconnection, the module unloads itself */
> +        pa_log_debug("Unregistering module for %s", d->path);
> +        pa_hashmap_remove(u->device_modules, d->path);
> +        return PA_HOOK_OK;
> +    }
> +
> +    if (!mi && pa_bluetooth_device_any_transport_connected(d)) {
> +        /* a new device has been connected */
> +        pa_module *m;
> +        char *args = pa_sprintf_malloc("path=%s", d->path);
> +
> +        pa_log_debug("Loading module-bluez5-device %s", args);
> +        m = pa_module_load(u->module->core, "module-bluez5-device", args);
> +        pa_xfree(args);
> +
> +        if (m)
> +            pa_hashmap_put(u->device_modules, d->path, (void *) 1);

I'd prefer pa_hashmap_put(u->device_paths, d->path, d->path). It doesn't
seem to be necessary to copy the string in this case, since the code
should ensure that the path is removed from the hashmap before the
pa_bluetooth_device object is freed, but I wouldn't mind copying either,
because it would make it unnecessary for the reader to wonder whether
this code is broken or not (since it's usually unsafe to store strings
this way in a hashmap).

-- 
Tanu