Define local mapping to glib path, otherwise this has to be inside central
place in the build repository.
Retrieve Bluetooth version from configure.ac.
---
.gitignore | 2 +
Android.mk | 9 ++++
Makefile.am | 1 +
Makefile.android | 7 ++++
android/Android.mk | 24 +++++++++++
android/main.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
configure.ac | 5 +++
7 files changed, 167 insertions(+)
create mode 100644 Android.mk
create mode 100644 Makefile.android
create mode 100644 android/Android.mk
create mode 100644 android/main.c
diff --git a/.gitignore b/.gitignore
index 8a25a3e..331a18b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -98,3 +98,5 @@ unit/test-gobex-packet
unit/test-gobex-transfer
unit/test-*.log
unit/test-*.trs
+
+android/bluezd
diff --git a/Android.mk b/Android.mk
new file mode 100644
index 0000000..5ec3bb7
--- /dev/null
+++ b/Android.mk
@@ -0,0 +1,9 @@
+LOCAL_PATH := $(call my-dir)
+
+# Retrieve BlueZ version from configure.ac file
+BLUEZ_VERSION := $(shell grep AC_INIT $(LOCAL_PATH)/configure.ac | tr -d ' ' | sed -e 's/.*(.*,\(.*\))/\1/')
+
+# Specify pathmap for glib
+pathmap_INCL += glib:external/bluetooth/glib
+
+include $(call all-subdir-makefiles)
diff --git a/Makefile.am b/Makefile.am
index 4e4b1c5..2c048bd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -179,6 +179,7 @@ test_scripts =
include Makefile.tools
include Makefile.obexd
+include Makefile.android
if HID2HCI
rulesdir = @UDEV_DIR@/rules.d
diff --git a/Makefile.android b/Makefile.android
new file mode 100644
index 0000000..7b901ff
--- /dev/null
+++ b/Makefile.android
@@ -0,0 +1,7 @@
+
+if ANDROID_DAEMON
+noinst_PROGRAMS += android/bluezd
+
+android_bluezd_SOURCES = android/main.c
+android_bluezd_LDADD = @GLIB_LIBS@
+endif
diff --git a/android/Android.mk b/android/Android.mk
new file mode 100644
index 0000000..50f3c36
--- /dev/null
+++ b/android/Android.mk
@@ -0,0 +1,24 @@
+LOCAL_PATH := $(call my-dir)
+
+#
+# Android BlueZ daemon (bluezd)
+#
+
+include $(CLEAR_VARS)
+
+LOCAL_SRC_FILES := \
+ main.c \
+
+LOCAL_C_INCLUDES := \
+ $(call include-path-for, glib) \
+ $(call include-path-for, glib)/glib \
+ $(LOCAL_PATH)/../src \
+
+LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\"
+
+LOCAL_SHARED_LIBRARIES := \
+ libglib \
+
+LOCAL_MODULE := bluezd
+
+include $(BUILD_EXECUTABLE)
diff --git a/android/main.c b/android/main.c
new file mode 100644
index 0000000..37a64c1
--- /dev/null
+++ b/android/main.c
@@ -0,0 +1,119 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2013 Intel Corporation. All rights reserved.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program 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 General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <signal.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <glib.h>
+
+#include "hcid.h"
+
+#define SHUTDOWN_GRACE_SECONDS 10
+
+static GMainLoop *event_loop;
+
+void btd_exit(void)
+{
+ g_main_loop_quit(event_loop);
+}
+
+static gboolean quit_eventloop(gpointer user_data)
+{
+ btd_exit();
+ return FALSE;
+}
+
+static void signal_handler(int sig)
+{
+ static unsigned int __terminated = 0;
+
+ switch (sig) {
+ case SIGINT:
+ case SIGTERM:
+ if (__terminated == 0) {
+ g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS,
+ quit_eventloop, NULL);
+ }
+
+ __terminated = 1;
+ break;
+ }
+}
+
+static gboolean option_detach = TRUE;
+static gboolean option_version = FALSE;
+
+static GOptionEntry options[] = {
+ { "nodetach", 'n', G_OPTION_FLAG_REVERSE,
+ G_OPTION_ARG_NONE, &option_detach,
+ "Run with logging in foreground", NULL },
+ { "version", 'v', 0, G_OPTION_ARG_NONE, &option_version,
+ "Show version information and exit", NULL },
+ { NULL }
+};
+
+int main(int argc, char *argv[])
+{
+ GOptionContext *context;
+ GError *err = NULL;
+ struct sigaction sa;
+
+ context = g_option_context_new(NULL);
+ g_option_context_add_main_entries(context, options, NULL);
+
+ if (g_option_context_parse(context, &argc, &argv, &err) == FALSE) {
+ if (err != NULL) {
+ g_printerr("%s\n", err->message);
+ g_error_free(err);
+ } else
+ g_printerr("An unknown error occurred\n");
+ exit(1);
+ }
+
+ g_option_context_free(context);
+
+ if (option_version == TRUE) {
+ printf("%s\n", VERSION);
+ exit(0);
+ }
+
+ event_loop = g_main_loop_new(NULL, FALSE);
+
+ memset(&sa, 0, sizeof(sa));
+ sa.sa_handler = signal_handler;
+ sigaction(SIGINT, &sa, NULL);
+ sigaction(SIGTERM, &sa, NULL);
+
+ g_main_loop_run(event_loop);
+
+ g_main_loop_unref(event_loop);
+
+ return 0;
+}
diff --git a/configure.ac b/configure.ac
index 41c2935..3b7a5d9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -242,4 +242,9 @@ AC_DEFINE_UNQUOTED(CONFIGDIR, "${configdir}",
[Directory for the configuration files])
AC_SUBST(CONFIGDIR, "${configdir}")
+AC_ARG_ENABLE(android-daemon, AC_HELP_STRING([--enable-android-daemon],
+ [enable BlueZ Android daemon]),
+ [android_daemon=${enableval}])
+AM_CONDITIONAL(ANDROID_DAEMON, test "${android_daemon}" = "yes")
+
AC_OUTPUT(Makefile src/bluetoothd.8 lib/bluez.pc)
--
1.7.9.5
Hi Marcel,
On Fri, Sep 27, 2013 at 03:56:14AM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> >>>>> Define local mapping to glib path, otherwise this has to be inside
> >>>>> central
> >>>>> place in the build repository.
> >>>>>
> >>>>> Retrieve Bluetooth version from configure.ac.
> >>>>> ---
> >>>>> .gitignore | 2 +
> >>>>> Android.mk | 9 ++++
> >>>>> Makefile.am | 1 +
> >>>>> Makefile.android | 7 ++++
> >>>>> android/Android.mk | 24 +++++++++++
> >>>>> android/main.c | 119
> >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>> configure.ac | 5 +++
> >>>>> 7 files changed, 167 insertions(+)
> >>>>> create mode 100644 Android.mk
> >>>>> create mode 100644 Makefile.android
> >>>>> create mode 100644 android/Android.mk
> >>>>> create mode 100644 android/main.c
> >>>>
> >>>>
> >>>> lets split this out a little bit. Code additions should not be intermixed
> >>>> with additions to the build system and especially configure options.
> >>>>
> >>>> I rather not have a top-level Android.mk. It should be enough to provide
> >>>> an android/Android.mk.
> >>
> >> BTW, do you expect to have only one Android.mk for the whole project or
> >> this is OK for you to have smaller Android.mk in different subfolders like
> >> /src/shared etc. Both ways are valid.
> >
> > While having a single huge Android makefile is somehow make sense it would have
> > several disadvantages:
> > - we cannot use local build with "mm" for building for example only tools
> > or libs
> > - Android.mk will become very large and difficult to read
>
> I have no idea what this means. Lets start with a single
> android/Android.mk and then go from there. I am strictly against
> cluttering the whole project with Android specific make files.
OK, we will use the single Android makefile, it is actually not that big.
Best regards
Andrei Emeltchenko
Hi Fred,
> <snip>
>>> @@ -110,10 +137,22 @@ int main(int argc, char *argv[])
>>> sa.sa_handler = signal_handler;
>>> sigaction(SIGINT, &sa, NULL);
>>> sigaction(SIGTERM, &sa, NULL);
>>> + sigaction(SIGUSR2, &sa, NULL);
>>> +
>>> + __btd_log_init(option_debug, option_detach);
>>> +
>>> + /* no need to keep parsed option in memory */
>>> + free_options();
>>> +
>>> + DBG("Entering main loop");
>>
>> Don't do this debug. It is not helpful. The main() is so short, no point in logging that.
>
> I need at least one DBG call to be able to build, otherwise I got "undefined reference to `__start___debug'" error messages.
> So, this one seems ok to me.
fair enough for the beginning. Just remove it once you have more than one DBG in the source code and this is no longer needed.
Regards
Marcel
Hi Andrei,
>>>>> Define local mapping to glib path, otherwise this has to be inside
>>>>> central
>>>>> place in the build repository.
>>>>>
>>>>> Retrieve Bluetooth version from configure.ac.
>>>>> ---
>>>>> .gitignore | 2 +
>>>>> Android.mk | 9 ++++
>>>>> Makefile.am | 1 +
>>>>> Makefile.android | 7 ++++
>>>>> android/Android.mk | 24 +++++++++++
>>>>> android/main.c | 119
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> configure.ac | 5 +++
>>>>> 7 files changed, 167 insertions(+)
>>>>> create mode 100644 Android.mk
>>>>> create mode 100644 Makefile.android
>>>>> create mode 100644 android/Android.mk
>>>>> create mode 100644 android/main.c
>>>>
>>>>
>>>> lets split this out a little bit. Code additions should not be intermixed
>>>> with additions to the build system and especially configure options.
>>>>
>>>> I rather not have a top-level Android.mk. It should be enough to provide
>>>> an android/Android.mk.
>>
>> BTW, do you expect to have only one Android.mk for the whole project or
>> this is OK for you to have smaller Android.mk in different subfolders like
>> /src/shared etc. Both ways are valid.
>
> While having a single huge Android makefile is somehow make sense it would have
> several disadvantages:
> - we cannot use local build with "mm" for building for example only tools
> or libs
> - Android.mk will become very large and difficult to read
I have no idea what this means. Lets start with a single android/Android.mk and then go from there. I am strictly against cluttering the whole project with Android specific make files.
Regards
Marcel
Hello Marcel,
On 21/09/2013 19:20, Marcel Holtmann wrote:
<snip>
>> @@ -110,10 +137,22 @@ int main(int argc, char *argv[])
>> sa.sa_handler = signal_handler;
>> sigaction(SIGINT, &sa, NULL);
>> sigaction(SIGTERM, &sa, NULL);
>> + sigaction(SIGUSR2, &sa, NULL);
>> +
>> + __btd_log_init(option_debug, option_detach);
>> +
>> + /* no need to keep parsed option in memory */
>> + free_options();
>> +
>> + DBG("Entering main loop");
>
> Don't do this debug. It is not helpful. The main() is so short, no point in logging that.
I need at least one DBG call to be able to build, otherwise I got
"undefined reference to `__start___debug'" error messages.
So, this one seems ok to me.
Regards
Fred
--
Frederic Danis Open Source Technology Center
[email protected] Intel Corporation
Hi All,
On Mon, Sep 23, 2013 at 09:28:44PM +0200, Lukasz Rymanowski wrote:
> Hi Marcel,
>
> On Mon, Sep 23, 2013 at 11:51 AM, Frederic Danis
> <[email protected]> wrote:
> > Hello Marcel,
> >
> > On 21/09/2013 19:14, Marcel Holtmann wrote:
> >>
> >> Hi Fred,
> >>
> >>> Define local mapping to glib path, otherwise this has to be inside
> >>> central
> >>> place in the build repository.
> >>>
> >>> Retrieve Bluetooth version from configure.ac.
> >>> ---
> >>> .gitignore | 2 +
> >>> Android.mk | 9 ++++
> >>> Makefile.am | 1 +
> >>> Makefile.android | 7 ++++
> >>> android/Android.mk | 24 +++++++++++
> >>> android/main.c | 119
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> configure.ac | 5 +++
> >>> 7 files changed, 167 insertions(+)
> >>> create mode 100644 Android.mk
> >>> create mode 100644 Makefile.android
> >>> create mode 100644 android/Android.mk
> >>> create mode 100644 android/main.c
> >>
> >>
> >> lets split this out a little bit. Code additions should not be intermixed
> >> with additions to the build system and especially configure options.
> >>
> >> I rather not have a top-level Android.mk. It should be enough to provide
> >> an android/Android.mk.
>
> BTW, do you expect to have only one Android.mk for the whole project or
> this is OK for you to have smaller Android.mk in different subfolders like
> /src/shared etc. Both ways are valid.
While having a single huge Android makefile is somehow make sense it would have
several disadvantages:
- we cannot use local build with "mm" for building for example only tools
or libs
- Android.mk will become very large and difficult to read
Best regards
Andrei Emeltchenko
Hi Marcel,
On Mon, Sep 23, 2013 at 11:51 AM, Frederic Danis
<[email protected]> wrote:
> Hello Marcel,
>
> On 21/09/2013 19:14, Marcel Holtmann wrote:
>>
>> Hi Fred,
>>
>>> Define local mapping to glib path, otherwise this has to be inside
>>> central
>>> place in the build repository.
>>>
>>> Retrieve Bluetooth version from configure.ac.
>>> ---
>>> .gitignore | 2 +
>>> Android.mk | 9 ++++
>>> Makefile.am | 1 +
>>> Makefile.android | 7 ++++
>>> android/Android.mk | 24 +++++++++++
>>> android/main.c | 119
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> configure.ac | 5 +++
>>> 7 files changed, 167 insertions(+)
>>> create mode 100644 Android.mk
>>> create mode 100644 Makefile.android
>>> create mode 100644 android/Android.mk
>>> create mode 100644 android/main.c
>>
>>
>> lets split this out a little bit. Code additions should not be intermixed
>> with additions to the build system and especially configure options.
>>
>> I rather not have a top-level Android.mk. It should be enough to provide
>> an android/Android.mk.
BTW, do you expect to have only one Android.mk for the whole project or
this is OK for you to have smaller Android.mk in different subfolders like
/src/shared etc. Both ways are valid.
Br
Lukasz
Hi Marcel
On Mon, Sep 23, 2013 at 11:51 AM, Frederic Danis <
[email protected]> wrote:
> Hello Marcel,
>
> On 21/09/2013 19:14, Marcel Holtmann wrote:
>
>> Hi Fred,
>>
>> Define local mapping to glib path, otherwise this has to be inside
>>> central
>>> place in the build repository.
>>>
>>> Retrieve Bluetooth version from configure.ac.
>>> ---
>>> .gitignore | 2 +
>>> Android.mk | 9 ++++
>>> Makefile.am | 1 +
>>> Makefile.android | 7 ++++
>>> android/Android.mk | 24 +++++++++++
>>> android/main.c | 119 ++++++++++++++++++++++++++++++**
>>> ++++++++++++++++++++++
>>> configure.ac | 5 +++
>>> 7 files changed, 167 insertions(+)
>>> create mode 100644 Android.mk
>>> create mode 100644 Makefile.android
>>> create mode 100644 android/Android.mk
>>> create mode 100644 android/main.c
>>>
>>
>> lets split this out a little bit. Code additions should not be intermixed
>> with additions to the build system and especially configure options.
>>
>> I rather not have a top-level Android.mk. It should be enough to provide
>> an android/Android.mk.
>>
>> BTW, do you expect to have only one Android.mk for the whole project or
this is OK for you to have smaller Android.mk in different subfolders like
/src/shared etc. Both ways are valid.
BR
Lukasz
Hello Marcel,
On 21/09/2013 19:14, Marcel Holtmann wrote:
> Hi Fred,
>
>> Define local mapping to glib path, otherwise this has to be inside central
>> place in the build repository.
>>
>> Retrieve Bluetooth version from configure.ac.
>> ---
>> .gitignore | 2 +
>> Android.mk | 9 ++++
>> Makefile.am | 1 +
>> Makefile.android | 7 ++++
>> android/Android.mk | 24 +++++++++++
>> android/main.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> configure.ac | 5 +++
>> 7 files changed, 167 insertions(+)
>> create mode 100644 Android.mk
>> create mode 100644 Makefile.android
>> create mode 100644 android/Android.mk
>> create mode 100644 android/main.c
>
> lets split this out a little bit. Code additions should not be intermixed with additions to the build system and especially configure options.
>
> I rather not have a top-level Android.mk. It should be enough to provide an android/Android.mk.
>
>> diff --git a/.gitignore b/.gitignore
>> index 8a25a3e..331a18b 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -98,3 +98,5 @@ unit/test-gobex-packet
>> unit/test-gobex-transfer
>> unit/test-*.log
>> unit/test-*.trs
>> +
>> +android/bluezd
>
> If we keep using prefix btd_ for certain set of functions, then it might be better if the daemon is build as android/bluetoothd or android/btd.
OK, I will rename it android/bluetoothd
>
> <snip>
>
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <signal.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +
>> +#include <glib.h>
>> +
>> +#include "hcid.h"
>
> I do not think we need to include hcid.h here. If we for some reason still do, then please try to cleanup that first.
OK, I will remove it
>
>> +
>> +#define SHUTDOWN_GRACE_SECONDS 10
>> +
>> +static GMainLoop *event_loop;
>> +
>> +void btd_exit(void)
>> +{
>> + g_main_loop_quit(event_loop);
>> +}
>> +
>> +static gboolean quit_eventloop(gpointer user_data)
>> +{
>> + btd_exit();
>> + return FALSE;
>> +}
>> +
>> +static void signal_handler(int sig)
>> +{
>> + static unsigned int __terminated = 0;
>
> Make this static volatile bool __terminated = false instead. Remember that you are actually using signals here and not signalfd.
OK, I will do this.
>
>> +
>> + switch (sig) {
>> + case SIGINT:
>> + case SIGTERM:
>> + if (__terminated == 0) {
>> + g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS,
>> + quit_eventloop, NULL);
>> + }
>> +
>> + __terminated = 1;
>> + break;
>> + }
>> +}
>> +
>> +static gboolean option_detach = TRUE;
>> +static gboolean option_version = FALSE;
>> +
>> +static GOptionEntry options[] = {
>> + { "nodetach", 'n', G_OPTION_FLAG_REVERSE,
>> + G_OPTION_ARG_NONE, &option_detach,
>> + "Run with logging in foreground", NULL },
>
> Do we need this option at all. I think we should always run in foreground and the system manager need to make sure we are spawned.
I think this is useful when debugging in conjonction with debug option
and logging to console.
>
>> + { "version", 'v', 0, G_OPTION_ARG_NONE, &option_version,
>> + "Show version information and exit", NULL },
>> + { NULL }
>> +};
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + GOptionContext *context;
>> + GError *err = NULL;
>> + struct sigaction sa;
>> +
>> + context = g_option_context_new(NULL);
>> + g_option_context_add_main_entries(context, options, NULL);
>> +
>> + if (g_option_context_parse(context, &argc, &argv, &err) == FALSE) {
>> + if (err != NULL) {
>> + g_printerr("%s\n", err->message);
>> + g_error_free(err);
>> + } else
>> + g_printerr("An unknown error occurred\n");
>> + exit(1);
>> + }
>> +
>> + g_option_context_free(context);
>> +
>> + if (option_version == TRUE) {
>> + printf("%s\n", VERSION);
>> + exit(0);
>
> I prefer if we start using return EXIT_SUCCESS and in error case EXIT_FAILURE.
OK, I will do this.
>
>> + }
>> +
>> + event_loop = g_main_loop_new(NULL, FALSE);
>> +
>> + memset(&sa, 0, sizeof(sa));
>> + sa.sa_handler = signal_handler;
>> + sigaction(SIGINT, &sa, NULL);
>> + sigaction(SIGTERM, &sa, NULL);
>> +
>> + g_main_loop_run(event_loop);
>> +
>> + g_main_loop_unref(event_loop);
>> +
>> + return 0;
>> +}
>> diff --git a/configure.ac b/configure.ac
>> index 41c2935..3b7a5d9 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -242,4 +242,9 @@ AC_DEFINE_UNQUOTED(CONFIGDIR, "${configdir}",
>> [Directory for the configuration files])
>> AC_SUBST(CONFIGDIR, "${configdir}")
>>
>> +AC_ARG_ENABLE(android-daemon, AC_HELP_STRING([--enable-android-daemon],
>> + [enable BlueZ Android daemon]),
>> + [android_daemon=${enableval}])
>> +AM_CONDITIONAL(ANDROID_DAEMON, test "${android_daemon}" = "yes")
>> +
>
> This needs to be a separate patch. And it should just say --enable-android.
>
> In addition it needs to be added to bootstrap-configure and distcheck handling.
OK
Regards
Fred
--
Frederic Danis Open Source Technology Center
[email protected] Intel Corporation
On Sat, Sep 21, 2013 at 12:14:55PM -0500, Marcel Holtmann wrote:
> Hi Fred,
>
> > Define local mapping to glib path, otherwise this has to be inside central
> > place in the build repository.
> >
> > Retrieve Bluetooth version from configure.ac.
> > ---
> > .gitignore | 2 +
> > Android.mk | 9 ++++
> > Makefile.am | 1 +
> > Makefile.android | 7 ++++
> > android/Android.mk | 24 +++++++++++
> > android/main.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > configure.ac | 5 +++
> > 7 files changed, 167 insertions(+)
> > create mode 100644 Android.mk
> > create mode 100644 Makefile.android
> > create mode 100644 android/Android.mk
> > create mode 100644 android/main.c
>
> lets split this out a little bit. Code additions should not be intermixed with additions to the build system and especially configure options.
>
> I rather not have a top-level Android.mk. It should be enough to provide an android/Android.mk.
>
I believe this way we cannot build our project.
Best regards
Andrei Emeltchenko
Hi all,
On Sat, Sep 21, 2013 at 12:20:52PM -0500, Marcel Holtmann wrote:
> Hi Fred,
> > +
> > +static void android_log(int pri, const char *fmt, va_list ap)
> > +{
> > + char *msg;
> > + struct iovec vec[3];
> > +
> > + msg = g_strdup_vprintf(fmt, ap);
> > +
> > + if (!detached) {
> > + vec[0].iov_base = (void *) msg;
> > + vec[0].iov_len = strlen(msg) + 1;
> > + vec[1].iov_base = "\n";
> > + vec[1].iov_len = 1;
> > + writev(STDERR_FILENO, vec, 2);
> > + }
>
> Lets not worry about logging to stderr at all.
Then I believe this might be simplified a lot. We might use
__android_log_print - like functions in this case.
Best regards
Andrei Emeltchenko
Hi Marcel,
On Sat, Sep 21, 2013, Marcel Holtmann wrote:
> > Define local mapping to glib path, otherwise this has to be inside central
> > place in the build repository.
> >
> > Retrieve Bluetooth version from configure.ac.
> > ---
> > .gitignore | 2 +
> > Android.mk | 9 ++++
> > Makefile.am | 1 +
> > Makefile.android | 7 ++++
> > android/Android.mk | 24 +++++++++++
> > android/main.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > configure.ac | 5 +++
> > 7 files changed, 167 insertions(+)
> > create mode 100644 Android.mk
> > create mode 100644 Makefile.android
> > create mode 100644 android/Android.mk
> > create mode 100644 android/main.c
>
> lets split this out a little bit. Code additions should not be
> intermixed with additions to the build system and especially configure
> options.
I agree about keeping separate build system changes such as adding
completely new makefiles or adding configure options. However, what I
think doesn't hurt (and the guidance I've given) is to at least include
the matching build system change to the same commit that adds new c/h
files. This way you don't end up having "dead code" in the tree in
between commits that just lies around there but can't be compiled and
wont be included in any release tarballs.
Johan
Hi Fred,
> Add logging system to BlueZ Android daemon.
> Android build will use android/log.c file while autotools build will use
> src/log.c instead.
> ---
> Makefile.android | 2 +-
> android/Android.mk | 1 +
> android/log.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> android/main.c | 39 +++++++++++
> 4 files changed, 225 insertions(+), 1 deletion(-)
> create mode 100644 android/log.c
>
> diff --git a/Makefile.android b/Makefile.android
> index 7b901ff..8f65dbf 100644
> --- a/Makefile.android
> +++ b/Makefile.android
> @@ -2,6 +2,6 @@
> if ANDROID_DAEMON
> noinst_PROGRAMS += android/bluezd
>
> -android_bluezd_SOURCES = android/main.c
> +android_bluezd_SOURCES = android/main.c src/log.c
> android_bluezd_LDADD = @GLIB_LIBS@
> endif
> diff --git a/android/Android.mk b/android/Android.mk
> index 50f3c36..25099b3 100644
> --- a/android/Android.mk
> +++ b/android/Android.mk
> @@ -8,6 +8,7 @@ include $(CLEAR_VARS)
>
> LOCAL_SRC_FILES := \
> main.c \
> + log.c \
>
> LOCAL_C_INCLUDES := \
> $(call include-path-for, glib) \
> diff --git a/android/log.c b/android/log.c
> new file mode 100644
> index 0000000..1f98196
> --- /dev/null
> +++ b/android/log.c
> @@ -0,0 +1,184 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2013 Intel Corporation. All rights reserved.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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 General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <unistd.h>
> +#include <sys/uio.h>
> +
> +#include <glib.h>
> +
> +#include "log.h"
> +
> +#define LOG_DEBUG 3
> +#define LOG_INFO 4
> +#define LOG_WARN 5
> +#define LOG_ERR 6
> +
> +const char tag[] = "BlueZ";
> +int system_fd;
> +int detached;
Local variables should be static.
> +
> +static void android_log(int pri, const char *fmt, va_list ap)
> +{
> + char *msg;
> + struct iovec vec[3];
> +
> + msg = g_strdup_vprintf(fmt, ap);
> +
> + if (!detached) {
> + vec[0].iov_base = (void *) msg;
> + vec[0].iov_len = strlen(msg) + 1;
> + vec[1].iov_base = "\n";
> + vec[1].iov_len = 1;
> + writev(STDERR_FILENO, vec, 2);
> + }
Lets not worry about logging to stderr at all.
> +
> + if (system_fd == -1)
> + goto done;
> +
> + vec[0].iov_base = (unsigned char *) &pri;
> + vec[0].iov_len = 1;
> + vec[1].iov_base = (void *) tag;
> + vec[1].iov_len = strlen(tag) + 1;
> + vec[2].iov_base = (void *) msg;
> + vec[2].iov_len = strlen(msg) + 1;
> +
> + writev(system_fd, vec, 3);
> +
> +done:
> + g_free(msg);
> +}
> +
> +void info(const char *format, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, format);
> +
> + android_log(LOG_INFO, format, ap);
> +
> + va_end(ap);
> +}
> +
> +void warn(const char *format, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, format);
> +
> + android_log(LOG_WARN, format, ap);
> +
> + va_end(ap);
> +}
> +
> +void error(const char *format, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, format);
> +
> + android_log(LOG_ERR, format, ap);
> +
> + va_end(ap);
> +}
> +
> +void btd_debug(const char *format, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, format);
> +
> + android_log(LOG_DEBUG, format, ap);
> +
> + va_end(ap);
> +}
> +
> +extern struct btd_debug_desc __start___debug[];
> +extern struct btd_debug_desc __stop___debug[];
> +
> +static char **enabled = NULL;
> +
> +static gboolean is_enabled(struct btd_debug_desc *desc)
> +{
> + int i;
> +
> + if (enabled == NULL)
> + return 0;
> +
> + for (i = 0; enabled[i] != NULL; i++)
> + if (desc->file != NULL && g_pattern_match_simple(enabled[i],
> + desc->file) == TRUE)
> + return 1;
> +
> + return 0;
> +}
> +
> +void __btd_enable_debug(struct btd_debug_desc *start,
> + struct btd_debug_desc *stop)
> +{
> + struct btd_debug_desc *desc;
> +
> + if (start == NULL || stop == NULL)
> + return;
> +
> + for (desc = start; desc < stop; desc++) {
> + if (is_enabled(desc))
> + desc->flags |= BTD_DEBUG_FLAG_PRINT;
> + }
> +}
> +
> +void __btd_toggle_debug(void)
> +{
> + struct btd_debug_desc *desc;
> +
> + for (desc = __start___debug; desc < __stop___debug; desc++)
> + desc->flags |= BTD_DEBUG_FLAG_PRINT;
> +}
> +
> +void __btd_log_init(const char *debug, int detach)
> +{
> + if (debug != NULL)
> + enabled = g_strsplit_set(debug, ":, ", 0);
> +
> + __btd_enable_debug(__start___debug, __stop___debug);
> +
> + detached = detach;
> +
> + system_fd = open("/dev/log/system", O_WRONLY);
> +
> + info("Bluetooth daemon %s", VERSION);
> +}
> +
> +void __btd_log_cleanup(void)
> +{
> + close(system_fd);
> + system_fd = -1;
> +
> + g_strfreev(enabled);
> +}
> diff --git a/android/main.c b/android/main.c
> index 37a64c1..ef62b3d 100644
> --- a/android/main.c
> +++ b/android/main.c
> @@ -33,6 +33,7 @@
>
> #include <glib.h>
>
> +#include "log.h"
> #include "hcid.h"
>
> #define SHUTDOWN_GRACE_SECONDS 10
> @@ -58,19 +59,45 @@ static void signal_handler(int sig)
> case SIGINT:
> case SIGTERM:
> if (__terminated == 0) {
> + info("Terminating");
Since you are in a signal handler and not using signalfd, this is racy.
> g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS,
> quit_eventloop, NULL);
> }
>
> __terminated = 1;
> break;
> +
> + case SIGUSR2:
> + __btd_toggle_debug();
> + break;
> }
Lets leave this handling out right now. I know that bluetoothd historically supported it, but that is historic baggage. Neither oFono nor ConnMan does it that way. So just do no bother with this. If it becomes useful, we will add it.
> }
>
> +static char *option_debug = NULL;
> static gboolean option_detach = TRUE;
> static gboolean option_version = FALSE;
>
> +static void free_options(void)
> +{
> + g_free(option_debug);
> + option_debug = NULL;
> +}
> +
> +static gboolean parse_debug(const char *key, const char *value,
> + gpointer user_data, GError **error)
> +{
> + if (value)
> + option_debug = g_strdup(value);
> + else
> + option_debug = g_strdup("*");
> +
> + return TRUE;
> +}
> +
> static GOptionEntry options[] = {
> + { "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
> + G_OPTION_ARG_CALLBACK, parse_debug,
> + "Specify debug options to enable", "DEBUG" },
> { "nodetach", 'n', G_OPTION_FLAG_REVERSE,
> G_OPTION_ARG_NONE, &option_detach,
> "Run with logging in foreground", NULL },
> @@ -110,10 +137,22 @@ int main(int argc, char *argv[])
> sa.sa_handler = signal_handler;
> sigaction(SIGINT, &sa, NULL);
> sigaction(SIGTERM, &sa, NULL);
> + sigaction(SIGUSR2, &sa, NULL);
> +
> + __btd_log_init(option_debug, option_detach);
> +
> + /* no need to keep parsed option in memory */
> + free_options();
> +
> + DBG("Entering main loop");
Don't do this debug. It is not helpful. The main() is so short, no point in logging that.
>
> g_main_loop_run(event_loop);
>
> g_main_loop_unref(event_loop);
>
> + info("Exit");
> +
> + __btd_log_cleanup();
> +
> return 0;
> }
Regards
Marcel
Hi Fred,
> Define local mapping to glib path, otherwise this has to be inside central
> place in the build repository.
>
> Retrieve Bluetooth version from configure.ac.
> ---
> .gitignore | 2 +
> Android.mk | 9 ++++
> Makefile.am | 1 +
> Makefile.android | 7 ++++
> android/Android.mk | 24 +++++++++++
> android/main.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> configure.ac | 5 +++
> 7 files changed, 167 insertions(+)
> create mode 100644 Android.mk
> create mode 100644 Makefile.android
> create mode 100644 android/Android.mk
> create mode 100644 android/main.c
lets split this out a little bit. Code additions should not be intermixed with additions to the build system and especially configure options.
I rather not have a top-level Android.mk. It should be enough to provide an android/Android.mk.
> diff --git a/.gitignore b/.gitignore
> index 8a25a3e..331a18b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -98,3 +98,5 @@ unit/test-gobex-packet
> unit/test-gobex-transfer
> unit/test-*.log
> unit/test-*.trs
> +
> +android/bluezd
If we keep using prefix btd_ for certain set of functions, then it might be better if the daemon is build as android/bluetoothd or android/btd.
<snip>
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <signal.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <glib.h>
> +
> +#include "hcid.h"
I do not think we need to include hcid.h here. If we for some reason still do, then please try to cleanup that first.
> +
> +#define SHUTDOWN_GRACE_SECONDS 10
> +
> +static GMainLoop *event_loop;
> +
> +void btd_exit(void)
> +{
> + g_main_loop_quit(event_loop);
> +}
> +
> +static gboolean quit_eventloop(gpointer user_data)
> +{
> + btd_exit();
> + return FALSE;
> +}
> +
> +static void signal_handler(int sig)
> +{
> + static unsigned int __terminated = 0;
Make this static volatile bool __terminated = false instead. Remember that you are actually using signals here and not signalfd.
> +
> + switch (sig) {
> + case SIGINT:
> + case SIGTERM:
> + if (__terminated == 0) {
> + g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS,
> + quit_eventloop, NULL);
> + }
> +
> + __terminated = 1;
> + break;
> + }
> +}
> +
> +static gboolean option_detach = TRUE;
> +static gboolean option_version = FALSE;
> +
> +static GOptionEntry options[] = {
> + { "nodetach", 'n', G_OPTION_FLAG_REVERSE,
> + G_OPTION_ARG_NONE, &option_detach,
> + "Run with logging in foreground", NULL },
Do we need this option at all. I think we should always run in foreground and the system manager need to make sure we are spawned.
> + { "version", 'v', 0, G_OPTION_ARG_NONE, &option_version,
> + "Show version information and exit", NULL },
> + { NULL }
> +};
> +
> +int main(int argc, char *argv[])
> +{
> + GOptionContext *context;
> + GError *err = NULL;
> + struct sigaction sa;
> +
> + context = g_option_context_new(NULL);
> + g_option_context_add_main_entries(context, options, NULL);
> +
> + if (g_option_context_parse(context, &argc, &argv, &err) == FALSE) {
> + if (err != NULL) {
> + g_printerr("%s\n", err->message);
> + g_error_free(err);
> + } else
> + g_printerr("An unknown error occurred\n");
> + exit(1);
> + }
> +
> + g_option_context_free(context);
> +
> + if (option_version == TRUE) {
> + printf("%s\n", VERSION);
> + exit(0);
I prefer if we start using return EXIT_SUCCESS and in error case EXIT_FAILURE.
> + }
> +
> + event_loop = g_main_loop_new(NULL, FALSE);
> +
> + memset(&sa, 0, sizeof(sa));
> + sa.sa_handler = signal_handler;
> + sigaction(SIGINT, &sa, NULL);
> + sigaction(SIGTERM, &sa, NULL);
> +
> + g_main_loop_run(event_loop);
> +
> + g_main_loop_unref(event_loop);
> +
> + return 0;
> +}
> diff --git a/configure.ac b/configure.ac
> index 41c2935..3b7a5d9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -242,4 +242,9 @@ AC_DEFINE_UNQUOTED(CONFIGDIR, "${configdir}",
> [Directory for the configuration files])
> AC_SUBST(CONFIGDIR, "${configdir}")
>
> +AC_ARG_ENABLE(android-daemon, AC_HELP_STRING([--enable-android-daemon],
> + [enable BlueZ Android daemon]),
> + [android_daemon=${enableval}])
> +AM_CONDITIONAL(ANDROID_DAEMON, test "${android_daemon}" = "yes")
> +
This needs to be a separate patch. And it should just say --enable-android.
In addition it needs to be added to bootstrap-configure and distcheck handling.
Regards
Marcel
Add logging system to BlueZ Android daemon.
Android build will use android/log.c file while autotools build will use
src/log.c instead.
---
Makefile.android | 2 +-
android/Android.mk | 1 +
android/log.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++
android/main.c | 39 +++++++++++
4 files changed, 225 insertions(+), 1 deletion(-)
create mode 100644 android/log.c
diff --git a/Makefile.android b/Makefile.android
index 7b901ff..8f65dbf 100644
--- a/Makefile.android
+++ b/Makefile.android
@@ -2,6 +2,6 @@
if ANDROID_DAEMON
noinst_PROGRAMS += android/bluezd
-android_bluezd_SOURCES = android/main.c
+android_bluezd_SOURCES = android/main.c src/log.c
android_bluezd_LDADD = @GLIB_LIBS@
endif
diff --git a/android/Android.mk b/android/Android.mk
index 50f3c36..25099b3 100644
--- a/android/Android.mk
+++ b/android/Android.mk
@@ -8,6 +8,7 @@ include $(CLEAR_VARS)
LOCAL_SRC_FILES := \
main.c \
+ log.c \
LOCAL_C_INCLUDES := \
$(call include-path-for, glib) \
diff --git a/android/log.c b/android/log.c
new file mode 100644
index 0000000..1f98196
--- /dev/null
+++ b/android/log.c
@@ -0,0 +1,184 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2013 Intel Corporation. All rights reserved.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program 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 General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdarg.h>
+#include <unistd.h>
+#include <sys/uio.h>
+
+#include <glib.h>
+
+#include "log.h"
+
+#define LOG_DEBUG 3
+#define LOG_INFO 4
+#define LOG_WARN 5
+#define LOG_ERR 6
+
+const char tag[] = "BlueZ";
+int system_fd;
+int detached;
+
+static void android_log(int pri, const char *fmt, va_list ap)
+{
+ char *msg;
+ struct iovec vec[3];
+
+ msg = g_strdup_vprintf(fmt, ap);
+
+ if (!detached) {
+ vec[0].iov_base = (void *) msg;
+ vec[0].iov_len = strlen(msg) + 1;
+ vec[1].iov_base = "\n";
+ vec[1].iov_len = 1;
+ writev(STDERR_FILENO, vec, 2);
+ }
+
+ if (system_fd == -1)
+ goto done;
+
+ vec[0].iov_base = (unsigned char *) &pri;
+ vec[0].iov_len = 1;
+ vec[1].iov_base = (void *) tag;
+ vec[1].iov_len = strlen(tag) + 1;
+ vec[2].iov_base = (void *) msg;
+ vec[2].iov_len = strlen(msg) + 1;
+
+ writev(system_fd, vec, 3);
+
+done:
+ g_free(msg);
+}
+
+void info(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+
+ android_log(LOG_INFO, format, ap);
+
+ va_end(ap);
+}
+
+void warn(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+
+ android_log(LOG_WARN, format, ap);
+
+ va_end(ap);
+}
+
+void error(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+
+ android_log(LOG_ERR, format, ap);
+
+ va_end(ap);
+}
+
+void btd_debug(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+
+ android_log(LOG_DEBUG, format, ap);
+
+ va_end(ap);
+}
+
+extern struct btd_debug_desc __start___debug[];
+extern struct btd_debug_desc __stop___debug[];
+
+static char **enabled = NULL;
+
+static gboolean is_enabled(struct btd_debug_desc *desc)
+{
+ int i;
+
+ if (enabled == NULL)
+ return 0;
+
+ for (i = 0; enabled[i] != NULL; i++)
+ if (desc->file != NULL && g_pattern_match_simple(enabled[i],
+ desc->file) == TRUE)
+ return 1;
+
+ return 0;
+}
+
+void __btd_enable_debug(struct btd_debug_desc *start,
+ struct btd_debug_desc *stop)
+{
+ struct btd_debug_desc *desc;
+
+ if (start == NULL || stop == NULL)
+ return;
+
+ for (desc = start; desc < stop; desc++) {
+ if (is_enabled(desc))
+ desc->flags |= BTD_DEBUG_FLAG_PRINT;
+ }
+}
+
+void __btd_toggle_debug(void)
+{
+ struct btd_debug_desc *desc;
+
+ for (desc = __start___debug; desc < __stop___debug; desc++)
+ desc->flags |= BTD_DEBUG_FLAG_PRINT;
+}
+
+void __btd_log_init(const char *debug, int detach)
+{
+ if (debug != NULL)
+ enabled = g_strsplit_set(debug, ":, ", 0);
+
+ __btd_enable_debug(__start___debug, __stop___debug);
+
+ detached = detach;
+
+ system_fd = open("/dev/log/system", O_WRONLY);
+
+ info("Bluetooth daemon %s", VERSION);
+}
+
+void __btd_log_cleanup(void)
+{
+ close(system_fd);
+ system_fd = -1;
+
+ g_strfreev(enabled);
+}
diff --git a/android/main.c b/android/main.c
index 37a64c1..ef62b3d 100644
--- a/android/main.c
+++ b/android/main.c
@@ -33,6 +33,7 @@
#include <glib.h>
+#include "log.h"
#include "hcid.h"
#define SHUTDOWN_GRACE_SECONDS 10
@@ -58,19 +59,45 @@ static void signal_handler(int sig)
case SIGINT:
case SIGTERM:
if (__terminated == 0) {
+ info("Terminating");
g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS,
quit_eventloop, NULL);
}
__terminated = 1;
break;
+
+ case SIGUSR2:
+ __btd_toggle_debug();
+ break;
}
}
+static char *option_debug = NULL;
static gboolean option_detach = TRUE;
static gboolean option_version = FALSE;
+static void free_options(void)
+{
+ g_free(option_debug);
+ option_debug = NULL;
+}
+
+static gboolean parse_debug(const char *key, const char *value,
+ gpointer user_data, GError **error)
+{
+ if (value)
+ option_debug = g_strdup(value);
+ else
+ option_debug = g_strdup("*");
+
+ return TRUE;
+}
+
static GOptionEntry options[] = {
+ { "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
+ G_OPTION_ARG_CALLBACK, parse_debug,
+ "Specify debug options to enable", "DEBUG" },
{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
G_OPTION_ARG_NONE, &option_detach,
"Run with logging in foreground", NULL },
@@ -110,10 +137,22 @@ int main(int argc, char *argv[])
sa.sa_handler = signal_handler;
sigaction(SIGINT, &sa, NULL);
sigaction(SIGTERM, &sa, NULL);
+ sigaction(SIGUSR2, &sa, NULL);
+
+ __btd_log_init(option_debug, option_detach);
+
+ /* no need to keep parsed option in memory */
+ free_options();
+
+ DBG("Entering main loop");
g_main_loop_run(event_loop);
g_main_loop_unref(event_loop);
+ info("Exit");
+
+ __btd_log_cleanup();
+
return 0;
}
--
1.7.9.5