* [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
@ 2013-09-20 12:24 Frederic Danis
2013-09-20 12:24 ` [PATCH 2/2] android: Android version of log.c Frederic Danis
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
0 siblings, 2 replies; 15+ messages in thread
From: Frederic Danis @ 2013-09-20 12:24 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] android: Android version of log.c
2013-09-20 12:24 [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Frederic Danis
@ 2013-09-20 12:24 ` Frederic Danis
2013-09-21 17:20 ` Marcel Holtmann
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
1 sibling, 1 reply; 15+ messages in thread
From: Frederic Danis @ 2013-09-20 12:24 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-20 12:24 [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Frederic Danis
2013-09-20 12:24 ` [PATCH 2/2] android: Android version of log.c Frederic Danis
@ 2013-09-21 17:14 ` Marcel Holtmann
2013-09-21 18:09 ` Johan Hedberg
` (2 more replies)
1 sibling, 3 replies; 15+ messages in thread
From: Marcel Holtmann @ 2013-09-21 17:14 UTC (permalink / raw)
To: Frederic Danis; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] android: Android version of log.c
2013-09-20 12:24 ` [PATCH 2/2] android: Android version of log.c Frederic Danis
@ 2013-09-21 17:20 ` Marcel Holtmann
2013-09-23 7:48 ` Andrei Emeltchenko
2013-09-24 9:05 ` Frederic Danis
0 siblings, 2 replies; 15+ messages in thread
From: Marcel Holtmann @ 2013-09-21 17:20 UTC (permalink / raw)
To: Frederic Danis; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
@ 2013-09-21 18:09 ` Johan Hedberg
2013-09-23 7:59 ` Andrei Emeltchenko
2013-09-23 9:51 ` Frederic Danis
2 siblings, 0 replies; 15+ messages in thread
From: Johan Hedberg @ 2013-09-21 18:09 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Frederic Danis, linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] android: Android version of log.c
2013-09-21 17:20 ` Marcel Holtmann
@ 2013-09-23 7:48 ` Andrei Emeltchenko
2013-09-24 9:05 ` Frederic Danis
1 sibling, 0 replies; 15+ messages in thread
From: Andrei Emeltchenko @ 2013-09-23 7:48 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Frederic Danis, linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
2013-09-21 18:09 ` Johan Hedberg
@ 2013-09-23 7:59 ` Andrei Emeltchenko
2013-09-23 9:51 ` Frederic Danis
2 siblings, 0 replies; 15+ messages in thread
From: Andrei Emeltchenko @ 2013-09-23 7:59 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Frederic Danis, linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
2013-09-21 18:09 ` Johan Hedberg
2013-09-23 7:59 ` Andrei Emeltchenko
@ 2013-09-23 9:51 ` Frederic Danis
2013-09-23 19:26 ` Lukasz Rymanowski
2013-09-23 19:28 ` Lukasz Rymanowski
2 siblings, 2 replies; 15+ messages in thread
From: Frederic Danis @ 2013-09-23 9:51 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
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
frederic.danis@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-23 9:51 ` Frederic Danis
@ 2013-09-23 19:26 ` Lukasz Rymanowski
2013-09-23 19:28 ` Lukasz Rymanowski
1 sibling, 0 replies; 15+ messages in thread
From: Lukasz Rymanowski @ 2013-09-23 19:26 UTC (permalink / raw)
To: Frederic Danis; +Cc: Marcel Holtmann, linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]
Hi Marcel
On Mon, Sep 23, 2013 at 11:51 AM, Frederic Danis <
frederic.danis@linux.intel.com> 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
[-- Attachment #2: Type: text/html, Size: 2206 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-23 9:51 ` Frederic Danis
2013-09-23 19:26 ` Lukasz Rymanowski
@ 2013-09-23 19:28 ` Lukasz Rymanowski
2013-09-24 7:32 ` Andrei Emeltchenko
1 sibling, 1 reply; 15+ messages in thread
From: Lukasz Rymanowski @ 2013-09-23 19:28 UTC (permalink / raw)
To: Frederic Danis; +Cc: Marcel Holtmann, linux-bluetooth
Hi Marcel,
On Mon, Sep 23, 2013 at 11:51 AM, Frederic Danis
<frederic.danis@linux.intel.com> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-23 19:28 ` Lukasz Rymanowski
@ 2013-09-24 7:32 ` Andrei Emeltchenko
2013-09-27 1:56 ` Marcel Holtmann
0 siblings, 1 reply; 15+ messages in thread
From: Andrei Emeltchenko @ 2013-09-24 7:32 UTC (permalink / raw)
To: Lukasz Rymanowski; +Cc: Frederic Danis, Marcel Holtmann, linux-bluetooth
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
> <frederic.danis@linux.intel.com> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] android: Android version of log.c
2013-09-21 17:20 ` Marcel Holtmann
2013-09-23 7:48 ` Andrei Emeltchenko
@ 2013-09-24 9:05 ` Frederic Danis
2013-09-27 1:58 ` Marcel Holtmann
1 sibling, 1 reply; 15+ messages in thread
From: Frederic Danis @ 2013-09-24 9:05 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
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
frederic.danis@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-24 7:32 ` Andrei Emeltchenko
@ 2013-09-27 1:56 ` Marcel Holtmann
2013-09-27 6:50 ` Andrei Emeltchenko
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2013-09-27 1:56 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Lukasz Rymanowski, Frederic Danis, linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] android: Android version of log.c
2013-09-24 9:05 ` Frederic Danis
@ 2013-09-27 1:58 ` Marcel Holtmann
0 siblings, 0 replies; 15+ messages in thread
From: Marcel Holtmann @ 2013-09-27 1:58 UTC (permalink / raw)
To: Frederic Danis; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] android: Add skeleton of BlueZ Android daemon
2013-09-27 1:56 ` Marcel Holtmann
@ 2013-09-27 6:50 ` Andrei Emeltchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andrei Emeltchenko @ 2013-09-27 6:50 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Lukasz Rymanowski, Frederic Danis, linux-bluetooth
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-09-27 6:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-20 12:24 [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Frederic Danis
2013-09-20 12:24 ` [PATCH 2/2] android: Android version of log.c Frederic Danis
2013-09-21 17:20 ` Marcel Holtmann
2013-09-23 7:48 ` Andrei Emeltchenko
2013-09-24 9:05 ` Frederic Danis
2013-09-27 1:58 ` Marcel Holtmann
2013-09-21 17:14 ` [PATCH 1/2] android: Add skeleton of BlueZ Android daemon Marcel Holtmann
2013-09-21 18:09 ` Johan Hedberg
2013-09-23 7:59 ` Andrei Emeltchenko
2013-09-23 9:51 ` Frederic Danis
2013-09-23 19:26 ` Lukasz Rymanowski
2013-09-23 19:28 ` Lukasz Rymanowski
2013-09-24 7:32 ` Andrei Emeltchenko
2013-09-27 1:56 ` Marcel Holtmann
2013-09-27 6:50 ` Andrei Emeltchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).