On Fri, 2013-09-13 at 19:19 -0300, Jo?o Paulo Rechi Vita wrote:
> On Mon, Aug 19, 2013 at 6:54 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>
> >>
> >> Create a wrapper module called module-bluetooth-discover to avoid
> >> breaking backward-compatibility of default.pa. This wrapper may
> >> eventually be dropped altoghether with BlueZ 4 support.
> >> ---
> >> src/Makefile.am | 8 ++
> >> src/modules/bluetooth/module-bluetooth-discover.c | 107 ++++++++++++++++++++++
> >> 2 files changed, 115 insertions(+)
> >> create mode 100644 src/modules/bluetooth/module-bluetooth-discover.c
> >>
> >> diff --git a/src/Makefile.am b/src/Makefile.am
> >> index 0973d31..a283220 100644
> >> --- a/src/Makefile.am
> >> +++ b/src/Makefile.am
> >> @@ -1320,6 +1320,7 @@ endif
> >>
> >> if HAVE_BLUEZ
> >> modlibexec_LTLIBRARIES += \
> >> + module-bluetooth-discover.la \
> >> module-bluetooth-policy.la
> >> endif
> >>
> >> @@ -1427,6 +1428,7 @@ SYMDEF_FILES = \
> >> module-systemd-login-symdef.h \
> >> module-bluetooth-proximity-symdef.h \
> >> module-bluetooth-policy-symdef.h \
> >> + module-bluetooth-discover-symdef.h \
> >> module-bluez4-discover-symdef.h \
> >> module-bluez4-device-symdef.h \
> >> module-bluez5-discover-symdef.h \
> >> @@ -2035,6 +2037,12 @@ module_bluetooth_policy_la_LDFLAGS = $(MODULE_LDFLAGS)
> >> module_bluetooth_policy_la_LIBADD = $(MODULE_LIBADD)
> >> module_bluetooth_policy_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
> >>
> >> +# Bluetooth discover
> >> +module_bluetooth_discover_la_SOURCES = modules/bluetooth/module-bluetooth-discover.c
> >> +module_bluetooth_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
> >> +module_bluetooth_discover_la_LIBADD = $(MODULE_LIBADD)
> >> +module_bluetooth_discover_la_CFLAGS = $(AM_CFLAGS)
> >> +
> >> # Bluetooth BlueZ 4 sink / source
> >> module_bluez4_discover_la_SOURCES = modules/bluetooth/module-bluez4-discover.c
> >> module_bluez4_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
> >> diff --git a/src/modules/bluetooth/module-bluetooth-discover.c b/src/modules/bluetooth/module-bluetooth-discover.c
> >> new file mode 100644
> >> index 0000000..ffedca2
> >> --- /dev/null
> >> +++ b/src/modules/bluetooth/module-bluetooth-discover.c
> >> @@ -0,0 +1,107 @@
> >> +/***
> >> + This file is part of PulseAudio.
> >> +
> >> + Copyright 2013 Jo?o Paulo Rechi Vita
> >> +
> >> + PulseAudio is free software; you can redistribute it and/or modify
> >> + it under the terms of the GNU Lesser General Public License as
> >> + published by the Free Software Foundation; either version 2.1 of the
> >> + License, or (at your option) any later version.
> >> +
> >> + PulseAudio is distributed in the hope that it will be useful, but
> >> + WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + General Public License for more details.
> >> +
> >> + You should have received a copy of the GNU Lesser General Public
> >> + License along with PulseAudio; if not, write to the Free Software
> >> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> >> + USA.
> >> +***/
> >> +
> >> +#ifdef HAVE_CONFIG_H
> >> +#include <config.h>
> >> +#endif
> >> +
> >> +#include <pulsecore/core-util.h>
> >> +#include <pulsecore/macro.h>
> >> +#include <pulsecore/module.h>
> >> +
> >> +#include "module-bluetooth-discover-symdef.h"
> >> +
> >> +PA_MODULE_AUTHOR("Jo?o Paulo Rechi Vita");
> >> +PA_MODULE_DESCRIPTION("Detect available Bluetooth daemon and load the corresponding discovery module");
> >> +PA_MODULE_VERSION(PACKAGE_VERSION);
> >> +PA_MODULE_LOAD_ONCE(true);
> >> +
> >> +struct userdata {
> >> + pa_module *bluez5_module;
> >> + pa_module *bluez4_module;
> >> +};
> >> +
> >> +/* This function is heavily inspired in the implementation of ".ifexists" */
> >
> > Would it be possible to share the implementation between
> > module-bluetooth-discover and the ".ifexists" handler? We could have a
> > function called pa_module_exists(), for example.
> >
>
> Yes, I've thought about sharing code here but in cli-command.h, which
> would be a bit weird. Having it in module.h makes much more sense. I
> have created pa_module_exists() which can handle module names (i.e.:
> module-bluetooth-discover), relative file names (i.e.:
> module-bluetooth-discover.so), and absolute file names (i.e.:
> /tmp/module-bluetooth-discover.so). I may have missed some corner
> cases, so it would be a good idea if you can double check it.
>
> >> +static bool exists(const char *filename) {
> >> + const char *paths, *state = NULL;
> >> + char *p, *pathname;
> >> + bool result;
> >> +
> >> + if (!(paths = lt_dlgetsearchpath()))
> >
> > undefined reference to `lt_dlgetsearchpath'
> >
> > Does this compile on your machine?
> >
>
> Yes, it does. I don't have this problem here.
>
> >> + return false;
> >> +
> >> + while ((p = pa_split(paths, ":", &state))) {
> >> + pathname = pa_sprintf_malloc("%s" PA_PATH_SEP "%s", p, filename);
> >> + result = access(pathname, F_OK) == 0 ? true : false;
> >> + pa_log_debug("Checking for existence of '%s': %s", pathname, result ? "success" : "failure");
> >> + pa_xfree(pathname);
> >> + pa_xfree(p);
> >> + if (result)
> >> + return true;
> >> + }
> >> +
> >> + state = NULL;
> >> + if (PA_UNLIKELY(pa_run_from_build_tree())) {
> >> + while ((p = pa_split(paths, ":", &state))) {
> >> + pathname = pa_sprintf_malloc("%s" PA_PATH_SEP ".libs" PA_PATH_SEP "%s", p, filename);
> >> + result = access(pathname, F_OK) == 0 ? true : false;
> >> + pa_log_debug("Checking for existence of '%s': %s", pathname, result ? "success" : "failure");
> >> + pa_xfree(pathname);
> >> + pa_xfree(p);
> >> + if (result)
> >> + return true;
> >> + }
> >> + }
> >> + return false;
> >> +}
> >> +
> >> +int pa__init(pa_module* m) {
> >> + struct userdata *u;
> >> +
> >> + pa_assert(m);
> >> +
> >> + m->userdata = u = pa_xnew0(struct userdata, 1);
> >> +
> >> + if (exists("module-bluez5-discover.so"))
> >> + u->bluez5_module = pa_module_load(m->core, "module-bluez5-discover", NULL);
> >> +
> >> + if (exists("module-bluez4-discover.so"))
> >> + u->bluez4_module = pa_module_load(m->core, "module-bluez4-discover", NULL);
> >> +
> >> + return 0;
> >
> > I think loading module-bluetooth-discover should fail if neither of the
> > modules exist or load successfully.
> >
>
> Ok.
>
> > Also, keeping module-bluetooth-discover running doesn't seem useful, so
> > once the module has successfully loaded, it should immediately call
> > pa_module_unload_request() on itself.
> >
>
> I've made the changes according to your comment, but afterwards I
> started thinking if it wouldn't be a good idea to keep
> module-bluetooth-discovery around so the user can unload the bluetooth
> modules without having to know if the system have BlueZ 4 or BlueZ 5.
> What do you think?
I'm OK with either alternative. If you keep module-bluetooth-discover
loaded, I think you should pass "true" as the force parameter to
pa_module_unload(). This doesn't have a big practical effect, but
generally the force parameter should be "false" only in code that
implements some kind of a client interface (native protocol, D-Bus
protocol).
--
Tanu
