[pulseaudio-discuss] [PATCHv2 59/60] bluetooth: Fail to load driver is discovery module is not loaded

[Tanu Kaskinen]

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