On Tue, 2013-09-24 at 18:16 -0300, Jo?o Paulo Rechi Vita wrote: > On Mon, Aug 19, 2013 at 6:33 AM, Tanu Kaskinen > <tanu.kaskinen at linux.intel.com> wrote: > > There's a typo in the subject line: the first "is" should be "if". I > > also think "device" would be better than "driver", but that's > > nitpicking... > > > > I'm changing only the "is" for "if". > > > On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote: > >> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org> > >> > >> For quite some time now the device driver module doesn't work well > >> without the discovery module, so for the BlueZ 5 support we'll prevent > >> the device driver module to be loaded if the discovery module is not > >> loaded. > >> --- > >> src/modules/bluetooth/bluez5-util.c | 3 --- > >> src/modules/bluetooth/module-bluez5-device.c | 7 ++++++- > >> src/modules/bluetooth/module-bluez5-discover.c | 5 ++++- > >> 3 files changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c > >> index 3aad10f..3563d57 100644 > >> --- a/src/modules/bluetooth/bluez5-util.c > >> +++ b/src/modules/bluetooth/bluez5-util.c > >> @@ -1420,9 +1420,6 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c) { > >> DBusConnection *conn; > >> unsigned i; > >> > >> - if ((y = pa_shared_get(c, "bluetooth-discovery"))) > >> - return pa_bluetooth_discovery_ref(y); > >> - > >> y = pa_xnew0(pa_bluetooth_discovery, 1); > >> PA_REFCNT_INIT(y); > >> y->core = c; > >> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c > >> index dd9f40f..3c66c17 100644 > >> --- a/src/modules/bluetooth/module-bluez5-device.c > >> +++ b/src/modules/bluetooth/module-bluez5-device.c > >> @@ -39,6 +39,7 @@ > >> #include <pulsecore/modargs.h> > >> #include <pulsecore/poll.h> > >> #include <pulsecore/rtpoll.h> > >> +#include <pulsecore/shared.h> > >> #include <pulsecore/socket-util.h> > >> #include <pulsecore/thread.h> > >> #include <pulsecore/thread-mq.h> > >> @@ -1796,8 +1797,12 @@ int pa__init(pa_module* m) { > >> u->core = m->core; > >> u->modargs = ma; > >> > >> - if (!(u->discovery = pa_bluetooth_discovery_get(m->core))) > >> + if ((u->discovery = pa_shared_get(u->core, "bluetooth-discovery"))) > >> + pa_bluetooth_discovery_ref(u->discovery); > >> + else { > >> + pa_log_error("module-bluez5-discover doesn't seem to be loaded, refusing to load module-bluez5-device"); > >> goto fail; > >> + } > >> > >> if (!(u->device = pa_bluetooth_discovery_get_device_by_path(u->discovery, path))) { > >> pa_log_error("%s is unknown", path); > >> diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c > >> index e782b2e..a50b9ef 100644 > >> --- a/src/modules/bluetooth/module-bluez5-discover.c > >> +++ b/src/modules/bluetooth/module-bluez5-discover.c > >> @@ -27,6 +27,7 @@ > >> #include <pulsecore/core-util.h> > >> #include <pulsecore/macro.h> > >> #include <pulsecore/module.h> > >> +#include <pulsecore/shared.h> > >> > >> #include "bluez5-util.h" > >> > >> @@ -90,7 +91,9 @@ int pa__init(pa_module* m) { > >> u->core = m->core; > >> u->device_modules = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func); > >> > >> - if (!(u->discovery = pa_bluetooth_discovery_get(u->core))) > >> + if ((u->discovery = pa_shared_get(u->core, "bluetooth-discovery"))) > >> + pa_bluetooth_discovery_ref(u->discovery); > >> + else if (!(u->discovery = pa_bluetooth_discovery_get(u->core))) > >> goto fail; > >> > >> u->device_connection_changed_slot = > > > > module-bluez5-discover doesn't load devices that already are known to > > pa_bluetooth_discovery. Perhaps it should do that, but I think it would > > be simpler and yet good enough to just make module-bluez5-discover the > > sole owner of pa_bluetooth_discovery. That means removing refcounting > > from pa_bluetooth_discovery. module-bluez5-device will have to be > > notified before pa_bluetooth_discovery is freed, so there should be hook > > PA_BLUETOOTH_HOOK_DISCOVERY_UNLINK or something like that. > > > > How would a device be already known to pa_bluetooth_discovery when > module-bluez5-discover is loaded, if it is created by > module-bluez5-discover. We should even remove the pa_shared_get() in > module-bluez5-discover since it will always return NULL. It's possible with this sequence of events: 1. Load module-bluez5-discover. 2. Devices are found, module-bluez5-device instances are loaded (let's say devices A and B). 3. Unload module-bluez5-discover. 4. Unload module-bluez5-device for device A. 4. Load module-bluez5-discover. The pa_bluetooth_discovery object is kept alive by the module-bluez5-device modules after module-bluez5-discover is unloaded, so the pa_bluetooth_discovery object already exists when module-bluez5-discover is loaded for the second time. The newly loaded module-bluez5-discover won't load module-bluez5-device for device A, even though it should. > The refcounting is usefull if we want to unload module-bluez5-discover > when a device is already loaded. In this case it can continue to be > used, just new devices will not be discovered. True, this use case would become unsupported, if the refcounting was removed, but I wouldn't mind that. -- Tanu