Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 6.6 \(1510\)) Subject: Re: [PATCHv3 06/15] android: Add basic mgmt initialization sequence From: Marcel Holtmann In-Reply-To: <1381243883-2745-7-git-send-email-Andrei.Emeltchenko.news@gmail.com> Date: Wed, 9 Oct 2013 21:30:22 +0200 Cc: linux-bluetooth@vger.kernel.org Message-Id: <0147C8F8-0491-44CD-BCDB-51DF9F6D3315@holtmann.org> References: <1381131496-9417-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1381243883-2745-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1381243883-2745-7-git-send-email-Andrei.Emeltchenko.news@gmail.com> To: Andrei Emeltchenko Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andrei, > Initialize bluetooth controller via mgmt interface. > --- > Makefile.android | 4 +- > android/Android.mk | 11 +++ > android/main.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 203 insertions(+), 1 deletion(-) > > diff --git a/Makefile.android b/Makefile.android > index e161e6d..9a2c486 100644 > --- a/Makefile.android > +++ b/Makefile.android > @@ -1,7 +1,9 @@ > if ANDROID > noinst_PROGRAMS += android/bluetoothd > > -android_bluetoothd_SOURCES = android/main.c src/log.c > +android_bluetoothd_SOURCES = android/main.c src/log.c \ > + src/shared/util.h src/shared/util.c \ > + src/shared/mgmt.h src/shared/mgmt.c > android_bluetoothd_LDADD = @GLIB_LIBS@ > endif > > diff --git a/android/Android.mk b/android/Android.mk > index 2cabff4..f5fd863 100644 > --- a/android/Android.mk > +++ b/android/Android.mk > @@ -15,10 +15,15 @@ include $(CLEAR_VARS) > LOCAL_SRC_FILES := \ > main.c \ > log.c \ > + ../src/shared/mgmt.c \ > + ../src/shared/util.c \ > > LOCAL_C_INCLUDES := \ > $(call include-path-for, glib) \ > $(call include-path-for, glib)/glib \ > + > +LOCAL_C_INCLUDES += \ > + $(LOCAL_PATH)/../ \ > $(LOCAL_PATH)/../src \ do we need these nested includes actually? We could also just fix the includes. BlueZ historically has not been good with clear includes. I started to fix this, but it seems I have not gotten to all of them yet. > > LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\" > @@ -26,6 +31,12 @@ LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\" > # to suppress the "warning: missing initializer near initialization.." warning > LOCAL_CFLAGS += -Wno-missing-field-initializers > > +# to suppress the "pointer of type 'void *' used in arithmetic" warning > +LOCAL_CFLAGS += -Wno-pointer-arith Why do we need to suppress these warning. Can we not just fix them. > + > +# Define missing flags for Android 4.2 > +LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000 > + This thing is dangerous. Do we really bother with Android 4.2 support and can not rely on a newer version that has this fixed properly in bionic. > LOCAL_SHARED_LIBRARIES := \ > libglib \ > > diff --git a/android/main.c b/android/main.c > index f75b0a8..3580ac7 100644 > --- a/android/main.c > +++ b/android/main.c > @@ -25,6 +25,7 @@ > #include > #endif > > +#include > #include > #include > #include > @@ -36,9 +37,17 @@ > > #include "log.h" > > +#include "lib/bluetooth.h" > +#include "lib/mgmt.h" > +#include "src/shared/mgmt.h" > + > #define SHUTDOWN_GRACE_SECONDS 10 > > static GMainLoop *event_loop; > +static struct mgmt *mgmt_if = NULL; > + > +static uint8_t mgmt_version = 0; > +static uint8_t mgmt_revision = 0; > > static gboolean quit_eventloop(gpointer user_data) > { > @@ -67,6 +76,183 @@ static GOptionEntry options[] = { > { NULL } > }; > > +static void read_info_complete(uint8_t status, uint16_t length, > + const void *param, void *user_data) > +{ > + /* TODO: Store Controller information */ > + > + /** > + * Register all event notification handlers for controller. > + * > + * The handlers are registered after a succcesful read of the > + * controller info. From now on they can track updates and > + * notifications. > + */ This is not our comment style. > +} > + > + > +static void mgmt_index_added_event(uint16_t index, uint16_t length, > + const void *param, void *user_data) > +{ > + info("%s: index %u", __func__, index); > + > + DBG("sending read info command for index %u", index); > + > + if (mgmt_send(mgmt_if, MGMT_OP_READ_INFO, index, 0, NULL, > + read_info_complete, NULL, NULL) > 0) > + return; > + > + error("Failed to read adapter info for index %u", index); > + > +} I prefer if we focus on one controller index and one controller index only. There is no need to bother reading information from controllers that we will never use. > + > +static void mgmt_index_removed_event(uint16_t index, uint16_t length, > + const void *param, void *user_data) > +{ > + info("%s: index %u", __func__, index); > +} > + > +static void read_index_list_complete(uint8_t status, uint16_t length, > + const void *param, void *user_data) > +{ > + const struct mgmt_rp_read_index_list *rp = param; > + uint16_t num; > + int i; > + > + info(__func__); > + > + if (status != MGMT_STATUS_SUCCESS) { > + error("%s: Failed to read index list: %s (0x%02x)", > + __func__, mgmt_errstr(status), status); > + return; > + } > + > + if (length < sizeof(*rp)) { > + error("%s: Wrong size of read index list response", __func__); > + return; > + } > + > + num = btohs(rp->num_controllers); > + > + DBG("%s: Number of controllers: %d", __func__, num); > + > + if (num * sizeof(uint16_t) + sizeof(*rp) != length) { > + error("%s: Incorrect pkt size for index list rsp", __func__); > + return; > + } > + > + for (i = 0; i < num; i++) { > + uint16_t index; > + > + index = btohs(rp->index[i]); > + > + DBG("%s: Found index %u", __func__, index); > + > + /** > + * Use index added event notification. > + */ > + mgmt_index_added_event(index, 0, NULL, NULL); > + } Same here. Lets just pick one controller and be done with it. > +} > + > + Avoid double empty lines. > +static void read_commands_complete(uint8_t status, uint16_t length, > + const void *param, void *user_data) > +{ > + const struct mgmt_rp_read_commands *rp = param; > + uint16_t num_commands, num_events; > + > + info(__func__); These can not be info() ever. Make them DBG(). > + > + if (status != MGMT_STATUS_SUCCESS) { > + error("Failed to read supported commands: %s (0x%02x)", > + mgmt_errstr(status), status); > + return; > + } > + > + if (length < sizeof(*rp)) { > + error("Wrong size of read commands response"); > + return; > + } > + > + num_commands = btohs(rp->num_commands); > + num_events = btohs(rp->num_events); > + > + DBG("Number of commands: %d", num_commands); > + DBG("Number of events: %d", num_events); > +} When copying code from src/adapter.c can we please at least be smart about it. Code that has no benefit should not be run. If it is currently not used, then leave it out for now. > + > +static void read_version_complete(uint8_t status, uint16_t length, > + const void *param, void *user_data) > +{ > + const struct mgmt_rp_read_version *rp = param; > + > + info(__func__); > + > + if (status != MGMT_STATUS_SUCCESS) { > + error("Failed to read version information: %s (0x%02x)", > + mgmt_errstr(status), status); > + return; > + } > + > + if (length < sizeof(*rp)) { > + error("Wrong size response"); > + return; > + } > + > + mgmt_version = rp->version; > + mgmt_revision = btohs(rp->revision); > + > + info("Bluetooth management interface %u.%u initialized", > + mgmt_version, mgmt_revision); > + > + if (mgmt_version < 1) { > + error("Version 1.0 or later of management interface required"); > + abort(); > + } > + > + DBG("sending read supported commands command"); > + > + mgmt_send(mgmt_if, MGMT_OP_READ_COMMANDS, MGMT_INDEX_NONE, 0, NULL, > + read_commands_complete, NULL, NULL); > + > + mgmt_register(mgmt_if, MGMT_EV_INDEX_ADDED, MGMT_INDEX_NONE, > + mgmt_index_added_event, NULL, NULL); > + mgmt_register(mgmt_if, MGMT_EV_INDEX_REMOVED, MGMT_INDEX_NONE, > + mgmt_index_removed_event, NULL, NULL); > + > + DBG("sending read index list command"); > + > + if (mgmt_send(mgmt_if, MGMT_OP_READ_INDEX_LIST, MGMT_INDEX_NONE, 0, > + NULL, read_index_list_complete, NULL, NULL) > 0) > + return; > + > + error("Failed to read controller index list"); > +} > + > +static bool init_mgmt_interface(void) > +{ If you are not using the return value, then do not return it. > + mgmt_if = mgmt_new_default(); > + if (mgmt_if == NULL) { > + error("Failed to access management interface"); > + return false; > + } > + > + if (mgmt_send(mgmt_if, MGMT_OP_READ_VERSION, MGMT_INDEX_NONE, 0, NULL, > + read_version_complete, NULL, NULL) == 0) { > + error("Error sending READ_VERSION mgmt command"); > + return false; > + } > + > + return true; > +} > + > +static void cleanup_mgmt_interface(void) > +{ > + mgmt_unref(mgmt_if); > + mgmt_if = NULL; > +} > + > int main(int argc, char *argv[]) > { > GOptionContext *context; > @@ -100,10 +286,13 @@ int main(int argc, char *argv[]) > sigaction(SIGINT, &sa, NULL); > sigaction(SIGTERM, &sa, NULL); > > + init_mgmt_interface(); > + > DBG("Entering main loop"); > > g_main_loop_run(event_loop); > > + cleanup_mgmt_interface(); > g_main_loop_unref(event_loop); > > info("Exit"); Regards Marcel