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

[João Paulo Rechi Vita]

On Sun, Aug 18, 2013 at 10:27 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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".
>

Ok.

>> +    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.
>

Ok.

>> +
>> +    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).
>

Ok, but I prefer to add a comment here than to waste memory with an
unnecessary copy.

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