* Re: [PATCHv3 09/15] android: Add Android Makefile for libbluetooth
From: Andrei Emeltchenko @ 2013-10-10 12:52 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <EE03C055-2E09-4B8D-90BB-5E827BA227E0@holtmann.org>
Hi Marcel,
On Thu, Oct 10, 2013 at 02:45:35PM +0200, Marcel Holtmann wrote:
> >>> +
> >>> +lib_headers := \
> >>> + bluetooth.h \
> >>> + hci.h \
> >>> + hci_lib.h \
> >>> + sco.h \
> >>> + l2cap.h \
> >>> + sdp.h \
> >>> + sdp_lib.h \
> >>> + rfcomm.h \
> >>> + bnep.h \
> >>> + cmtp.h \
> >>> + hidp.h \
> >>
> >> I would really prefer we do not install these.
> >
> > this creates directly bluetooth with links to headers since this is they
> > way BlueZ is referring to them. What do you propose here?
>
> I prefer that we do not install these headers or the shared library.
> Nothing outside of BlueZ build should need them.
They are not installed anywhere. Just links created locally.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv3 07/15] android: Create HAL API header skeleton
From: Andrei Emeltchenko @ 2013-10-10 12:48 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <56EFB0C2-81B7-4E25-B1ED-B61465248141@holtmann.org>
Hi Marcel,
On Thu, Oct 10, 2013 at 02:35:37PM +0200, Marcel Holtmann wrote:
> >>> +#define HAL_MSG_OP_BT_SET_REMOTE_DEVICE_PROP 0x08
> >>> +struct hal_msg_cp_bt_set_remote_device_prop {
> >>> + __bdaddr_t bdaddr;
> >>> + uint8_t type;
> >>> + uint16_t len;
> >>
> >> Lets make these all align properly.
> >
> > Align with tabs? So this will be very different from lib/mgmt.h ?
>
> In monitor/bt.h, I did align them with spaces.
Just to be sure: align structures with spaces always?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv3 09/15] android: Add Android Makefile for libbluetooth
From: Marcel Holtmann @ 2013-10-10 12:45 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131010124347.GM23879@aemeltch-MOBL1>
Hi Andrei,
>>> Build libbluetooth library for Android.
>>> ---
>>> android/Android.mk | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 53 insertions(+)
>>>
>>> diff --git a/android/Android.mk b/android/Android.mk
>>> index f5fd863..8f95a66 100644
>>> --- a/android/Android.mk
>>> +++ b/android/Android.mk
>>> @@ -41,6 +41,7 @@ LOCAL_SHARED_LIBRARIES := \
>>> libglib \
>>>
>>> LOCAL_MODULE := bluetoothd
>>> +LOCAL_REQUIRED_MODULES := libbluetooth
>>>
>>> include $(BUILD_EXECUTABLE)
>>>
>>> @@ -63,3 +64,55 @@ LOCAL_MODULE_TAGS := optional
>>> LOCAL_MODULE_CLASS := SHARED_LIBRARIES
>>>
>>> include $(BUILD_SHARED_LIBRARY)
>>> +
>>> +#
>>> +# libbluetooth library
>>> +#
>>> +
>>> +include $(CLEAR_VARS)
>>> +
>>> +LOCAL_SRC_FILES := \
>>> + ../lib/bluetooth.c \
>>> + ../lib/sdp.c \
>>> + ../lib/hci.c \
>>> + ../lib/uuid.c \
>>
>> why are we building a shared library here. What is it useful for. Even
>> within BlueZ we do not install libbluetooth anymore.
>
> It is used for tools also like btmon, shall we include those files
> directly?
if that works, then yes, just include the files that are needed. The newer tools are actually limited to only a few functions. And we could work on getting libbluetooth dependency out of it.
For an autoconf based build it is not high priority sine we only build the library as an internal version that in the end gets statically linked. And unused functions will just be optimized out.
>
>>
>>> +
>>> +LOCAL_C_INCLUDES := \
>>> + $(LOCAL_PATH)/../lib/ \
>>> + $(LOCAL_PATH)/../src/shared \
>>
>> The library clearly does not include anything from src/shared/. So what is that doing here.
>>
>>> +
>>> +LOCAL_C_INCLUDES += \
>>> + $(call include-path-for, glib) \
>>> + $(call include-path-for, glib)/glib \
>>
>> The library clearly does not need GLib. So why are we doing that here.
>>
>>> +
>>> +LOCAL_CFLAGS += -O3
>>> +
>>> +# to suppress the "pointer of type 'void *' used in arithmetic" warning
>>> +LOCAL_CFLAGS += -Wno-pointer-arith
>>> +
>>> +# to suppress the "missing initializer near initialization" warning
>>> +LOCAL_CFLAGS += -Wno-missing-field-initializers
>>
>> Lets fix the actual warnings.
>>
>>> +
>>> +# Define missing flags for Android 4.2
>>> +LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000
>>
>> And are we sure that these flags are needed for the library.
>>
>>> +
>>> +lib_headers := \
>>> + bluetooth.h \
>>> + hci.h \
>>> + hci_lib.h \
>>> + sco.h \
>>> + l2cap.h \
>>> + sdp.h \
>>> + sdp_lib.h \
>>> + rfcomm.h \
>>> + bnep.h \
>>> + cmtp.h \
>>> + hidp.h \
>>
>> I would really prefer we do not install these.
>
> this creates directly bluetooth with links to headers since this is they
> way BlueZ is referring to them. What do you propose here?
I prefer that we do not install these headers or the shared library. Nothing outside of BlueZ build should need them.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv3 09/15] android: Add Android Makefile for libbluetooth
From: Andrei Emeltchenko @ 2013-10-10 12:43 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <67FF7DAD-8C17-4ADB-8050-5F35CC7C34D4@holtmann.org>
Hi Marcel,
On Wed, Oct 09, 2013 at 09:43:18PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> > Build libbluetooth library for Android.
> > ---
> > android/Android.mk | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 53 insertions(+)
> >
> > diff --git a/android/Android.mk b/android/Android.mk
> > index f5fd863..8f95a66 100644
> > --- a/android/Android.mk
> > +++ b/android/Android.mk
> > @@ -41,6 +41,7 @@ LOCAL_SHARED_LIBRARIES := \
> > libglib \
> >
> > LOCAL_MODULE := bluetoothd
> > +LOCAL_REQUIRED_MODULES := libbluetooth
> >
> > include $(BUILD_EXECUTABLE)
> >
> > @@ -63,3 +64,55 @@ LOCAL_MODULE_TAGS := optional
> > LOCAL_MODULE_CLASS := SHARED_LIBRARIES
> >
> > include $(BUILD_SHARED_LIBRARY)
> > +
> > +#
> > +# libbluetooth library
> > +#
> > +
> > +include $(CLEAR_VARS)
> > +
> > +LOCAL_SRC_FILES := \
> > + ../lib/bluetooth.c \
> > + ../lib/sdp.c \
> > + ../lib/hci.c \
> > + ../lib/uuid.c \
>
> why are we building a shared library here. What is it useful for. Even
> within BlueZ we do not install libbluetooth anymore.
It is used for tools also like btmon, shall we include those files
directly?
>
> > +
> > +LOCAL_C_INCLUDES := \
> > + $(LOCAL_PATH)/../lib/ \
> > + $(LOCAL_PATH)/../src/shared \
>
> The library clearly does not include anything from src/shared/. So what is that doing here.
>
> > +
> > +LOCAL_C_INCLUDES += \
> > + $(call include-path-for, glib) \
> > + $(call include-path-for, glib)/glib \
>
> The library clearly does not need GLib. So why are we doing that here.
>
> > +
> > +LOCAL_CFLAGS += -O3
> > +
> > +# to suppress the "pointer of type 'void *' used in arithmetic" warning
> > +LOCAL_CFLAGS += -Wno-pointer-arith
> > +
> > +# to suppress the "missing initializer near initialization" warning
> > +LOCAL_CFLAGS += -Wno-missing-field-initializers
>
> Lets fix the actual warnings.
>
> > +
> > +# Define missing flags for Android 4.2
> > +LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000
>
> And are we sure that these flags are needed for the library.
>
> > +
> > +lib_headers := \
> > + bluetooth.h \
> > + hci.h \
> > + hci_lib.h \
> > + sco.h \
> > + l2cap.h \
> > + sdp.h \
> > + sdp_lib.h \
> > + rfcomm.h \
> > + bnep.h \
> > + cmtp.h \
> > + hidp.h \
>
> I would really prefer we do not install these.
this creates directly bluetooth with links to headers since this is they
way BlueZ is referring to them. What do you propose here?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv3 08/15] android: Add adapter and device struct for BlueZ daemon
From: Marcel Holtmann @ 2013-10-10 12:40 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131010123638.GL23879@aemeltch-MOBL1>
Hi Andrei,
>>> Adapter structure in BlueZ daemon keeps track of default adapter
>>> and device structure keeps track about found devices.
>>> ---
>>> android/bt_adapter.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
>>> android/bt_adapter.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 116 insertions(+)
>>> create mode 100644 android/bt_adapter.c
>>> create mode 100644 android/bt_adapter.h
>>>
>>> diff --git a/android/bt_adapter.c b/android/bt_adapter.c
>>> new file mode 100644
>>> index 0000000..e21d50c
>>> --- /dev/null
>>> +++ b/android/bt_adapter.c
>>> @@ -0,0 +1,56 @@
>>> +/*
>>> + *
>>> + * 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
>>> + *
>>> + */
>>> +
>>> +#include "bt_adapter.h"
>>
>> the include of its own header should always be the last one. Never the first one.
>>
>>
>>> +#include "log.h"
>>> +#include "src/shared/mgmt.h"
>>> +
>>> +struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
>>> +{
>>> + struct bt_adapter *adapter;
>>> +
>>> + adapter = g_try_new0(struct bt_adapter, 1);
>>> + if (!adapter)
>>> + return NULL;
>>
>> Since this all brand new code, use g_new0 and we just exit on memory allocation failures for our own data structures.
>>
>
> Do you mean exit(1)?
please understand on how g_new0 works.
>
>>> +
>>> + adapter->dev_id = index;
>>> + adapter->mgmt = mgmt_ref(mgmt_if);
>>> +
>>> + return adapter;
>>> +}
>>> +
>>> +void adapter_start(struct bt_adapter *adapter)
>>
>> Why does this now have no bt_ prefix?
>>
>>> +{
>>> + DBG("enabled %u", adapter->dev_id);
>>> +
>>> + /* TODO: CB: report scan mode */
>>> +
>>> + /* TODO: SDP start here */
>>> +
>>> + /* TODO: CB: report state on */
>>> +}
>>> +
>>> +void adapter_stop(struct bt_adapter *adapter)
>>> +{
>>> + DBG("disabled %u", adapter->dev_id);
>>> +}
>>> diff --git a/android/bt_adapter.h b/android/bt_adapter.h
>>> new file mode 100644
>>> index 0000000..6877cc7
>>> --- /dev/null
>>> +++ b/android/bt_adapter.h
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + *
>>> + * 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
>>> + *
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +#include <glib.h>
>>> +
>>> +#include "lib/bluetooth.h"
>>> +
>>> +struct bt_device {
>>> + int refcnt;
>>> +
>>> + bdaddr_t bdaddr;
>>> + uint8_t bdaddr_type;
>>> + uint32_t cod;
>>> + char *name;
>>> +};
>>
>> Why is bt_device declared in bt_adapter.
>> And why are these public in the first place. Please hide these details.
>
> So do you want it to be added in main.c? Those structs are public also in
> BlueZ.
struct bt_adapter;
Just forward declare them in the header. The actual fields do not need to be public. As I said before, this is new code and not BlueZ code that is many years old. We follow brand new style here and not copy old mistakes.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv3 06/15] android: Add basic mgmt initialization sequence
From: Marcel Holtmann @ 2013-10-10 12:38 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131010095952.GH23879@aemeltch-MOBL1>
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.
>>
>
> So how do you want it to be? Here I just added BlueZ top-dir.
>
>>> 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.
>>
>
> Is this fix good:
>
> diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
> index 2c79886..b2f5506 100644
> --- a/src/shared/mgmt.c
> +++ b/src/shared/mgmt.c
> @@ -55,7 +55,7 @@ struct mgmt {
> unsigned int next_notify_id;
> bool in_notify;
> bool destroyed;
> - void *buf;
> + uint8_t *buf;
> uint16_t len;
> mgmt_debug_func_t debug_callback;
> mgmt_destroy_func_t debug_destroy;
> @@ -299,7 +299,7 @@ static gboolean received_data(GIOChannel *channel,
> GIOCondition cond,
> if (bytes_read < MGMT_HDR_SIZE)
> return TRUE;
>
> - hdr = mgmt->buf;
> + hdr = (struct mgmt_hdr *) mgmt->buf;
are we using a different compiler or why does this happen on Android and not on a regular system like Fedora.
>>> +
>>> +# 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.
>
> The problem here is that our base android-ia from 01.org is based on
> 4.2.2. Do you still want to remove this?
I don't think we should care much about Android 4.2.2. Why is our base not updated to Android 4.3 yet. And as soon as 4.4 is out, we can ignore 4.3 as well. Chasing after old versions seems a bit pointless until someone actually integrated this into a product.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv3 08/15] android: Add adapter and device struct for BlueZ daemon
From: Andrei Emeltchenko @ 2013-10-10 12:36 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <02A1A12C-50FA-4B1E-A85F-979EE842C2D4@holtmann.org>
On Wed, Oct 09, 2013 at 09:39:34PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> > Adapter structure in BlueZ daemon keeps track of default adapter
> > and device structure keeps track about found devices.
> > ---
> > android/bt_adapter.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
> > android/bt_adapter.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 116 insertions(+)
> > create mode 100644 android/bt_adapter.c
> > create mode 100644 android/bt_adapter.h
> >
> > diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> > new file mode 100644
> > index 0000000..e21d50c
> > --- /dev/null
> > +++ b/android/bt_adapter.c
> > @@ -0,0 +1,56 @@
> > +/*
> > + *
> > + * 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
> > + *
> > + */
> > +
> > +#include "bt_adapter.h"
>
> the include of its own header should always be the last one. Never the first one.
>
>
> > +#include "log.h"
> > +#include "src/shared/mgmt.h"
> > +
> > +struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> > +{
> > + struct bt_adapter *adapter;
> > +
> > + adapter = g_try_new0(struct bt_adapter, 1);
> > + if (!adapter)
> > + return NULL;
>
> Since this all brand new code, use g_new0 and we just exit on memory allocation failures for our own data structures.
>
Do you mean exit(1)?
> > +
> > + adapter->dev_id = index;
> > + adapter->mgmt = mgmt_ref(mgmt_if);
> > +
> > + return adapter;
> > +}
> > +
> > +void adapter_start(struct bt_adapter *adapter)
>
> Why does this now have no bt_ prefix?
>
> > +{
> > + DBG("enabled %u", adapter->dev_id);
> > +
> > + /* TODO: CB: report scan mode */
> > +
> > + /* TODO: SDP start here */
> > +
> > + /* TODO: CB: report state on */
> > +}
> > +
> > +void adapter_stop(struct bt_adapter *adapter)
> > +{
> > + DBG("disabled %u", adapter->dev_id);
> > +}
> > diff --git a/android/bt_adapter.h b/android/bt_adapter.h
> > new file mode 100644
> > index 0000000..6877cc7
> > --- /dev/null
> > +++ b/android/bt_adapter.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + *
> > + * 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
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <glib.h>
> > +
> > +#include "lib/bluetooth.h"
> > +
> > +struct bt_device {
> > + int refcnt;
> > +
> > + bdaddr_t bdaddr;
> > + uint8_t bdaddr_type;
> > + uint32_t cod;
> > + char *name;
> > +};
>
> Why is bt_device declared in bt_adapter.
> And why are these public in the first place. Please hide these details.
So do you want it to be added in main.c? Those structs are public also in
BlueZ.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv3 07/15] android: Create HAL API header skeleton
From: Marcel Holtmann @ 2013-10-10 12:35 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131010122911.GJ23879@aemeltch-MOBL1>
Hi Andrei,
>>> Header describes the protocol between Android HAL threads and BlueZ
>>> daemon.
>>> ---
>>> android/hal_msg.h | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 255 insertions(+)
>>> create mode 100644 android/hal_msg.h
>>>
>>> diff --git a/android/hal_msg.h b/android/hal_msg.h
>>> new file mode 100644
>>> index 0000000..c6bc883
>>> --- /dev/null
>>> +++ b/android/hal_msg.h
>>> @@ -0,0 +1,255 @@
>>> +/*
>>> + *
>>> + * BlueZ - Bluetooth protocol stack for Linux
>>> + *
>>> + * Copyright (C) 2013 Intel Corporation. All rights reserved.
>>> + *
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library 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
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>>> + *
>>> + */
>>> +
>>> +#ifndef __packed
>>> +#define __packed __attribute__((packed))
>>> +#endif
>
> you want different style from lib/mgmt.h ?
no idea why that is in there. It should not be.
>>
>> nowhere in the code we are using __packed. That is more a kernel thing. Just use the gcc style directly.
>>
>>> +
>>> +typedef struct {
>>> + uint8_t b[6];
>>> +} __packed __bdaddr_t;
>>
>> Just use a uint8_t bdaddr[6] instead.
>>
>>> +
>>> +struct hal_msg_hdr {
>>> + uint8_t service_id;
>>> + uint8_t opcode;
>>> + uint16_t len;
>>> + uint8_t payload[0];
>>
>> I would not include the payload[0] here.
>
> I know that the current approach is to magically add to pointer header
> size, but doesn't it looks better?
Not to me.
>
>>
>>> +} __packed;
>>> +
>>> +#define HAL_SERVICE_ID_CORE 0
>>> +#define HAL_SERVICE_ID_BLUETOOTH 1
>>> +#define HAL_SERVICE_ID_SOCK 2
>>> +#define HAL_SERVICE_ID_HIDHOST 3
>>> +#define HAL_SERVICE_ID_PAN 4
>>> +#define HAL_SERVICE_ID_HANDSFREE 5
>>> +#define HAL_SERVICE_ID_AD2P 6
>>> +#define HAL_SERVICE_ID_HEALTH 7
>>> +#define HAL_SERVICE_ID_AVRCP 8
>>> +#define HAL_SERVICE_ID_GATT 9
>>> +
>>> +/* Core Service */
>>> +
>>> +#define HAL_MSG_OP_ERROR 0x00
>>> +struct hal_msg_rp_error {
>>> + uint8_t status;
>>> +} __packed;
>>
>> using RSP, rsp, CMD, cmd, EVT and EVT seems to be clearer I think.
>
> So I want to be sure that I understand this right:
>
> hal_msg_rp_error => hal_msg_rsp_error
>
> hal_msg_cp_register_module => hal_msg_cmd_register_module
>
> this would be different from lib/mgmt.h
Since this is not a mgmt.h API, it does not need to follow it. mgmt is a kernel API and that is where some its parts are coming from. This is not a kernel API.
>
>>> +
>>> +#define HAL_MSG_OP_REGISTER_MODULE 0x01
>>> +struct hal_msg_cp_register_module {
>>> + uint8_t service_id;
>>> +} __packed;
>>> +struct hal_msg_rp_register_module {
>>> + uint8_t service_id;
>>> +} __packed;
>>> +
>>> +#define HAL_MSG_OP_UNREGISTER_MODULE 0x02
>>> +struct hal_msg_cp_unregister_module {
>>> + uint8_t service_id;
>>> +} __packed;
>>> +
>>> +/* Bluetooth Core HAL API */
>>> +
>>> +#define HAL_MSG_OP_BT_ENABLE 0x01
>>> +
>>> +#define HAL_MSG_OP_BT_DISABLE 0x02
>>> +
>>> +#define HAL_MSG_OP_BT_GET_ADAPTER_PROPS 0x03
>>> +
>>> +#define HAL_MSG_OP_BT_GET_ADAPTER_PROP 0x04
>>> +struct hal_msg_cp_bt_get_adapter_prop {
>>> + uint8_t type;
>>> +} __packed;
>>> +
>>> +#define HAL_MSG_OP_BT_SET_ADAPTER_PROP 0x05
>>> +struct hal_msg_cp_bt_set_adapter_prop {
>>> + uint8_t type;
>>> + uint16_t len;
>>> + uint8_t val[0];
>>> +} __packed;
>>> +
>>> +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROPS 0x06
>>> +struct hal_msg_cp_bt_get_remote_device_props {
>>> + __bdaddr_t bdaddr;
>>> +} __packed;
>>> +
>>> +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROP 0x07
>>> +struct hal_msg_cp_bt_get_remote_device_prop {
>>> + __bdaddr_t bdaddr;
>>> + uint8_t type;
>>> +} __packed;
>>> +
>>> +#define HAL_MSG_OP_BT_SET_REMOTE_DEVICE_PROP 0x08
>>> +struct hal_msg_cp_bt_set_remote_device_prop {
>>> + __bdaddr_t bdaddr;
>>> + uint8_t type;
>>> + uint16_t len;
>>
>> Lets make these all align properly.
>
> Align with tabs? So this will be very different from lib/mgmt.h ?
In monitor/bt.h, I did align them with spaces.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv3 08/15] android: Add adapter and device struct for BlueZ daemon
From: Andrei Emeltchenko @ 2013-10-10 12:30 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth
In-Reply-To: <CABD6X-+RLquYcRhyMiO9N4j+HsmfvQrDN+CwKEaRri2qzqAaFA@mail.gmail.com>
Hi Marcin,
On Thu, Oct 10, 2013 at 11:07:00AM +0200, Marcin Kraglak wrote:
> Hi Andrei,
>
> On 8 October 2013 16:51, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Adapter structure in BlueZ daemon keeps track of default adapter
> > and device structure keeps track about found devices.
> > ---
> > android/bt_adapter.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
> > android/bt_adapter.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 116 insertions(+)
> > create mode 100644 android/bt_adapter.c
> > create mode 100644 android/bt_adapter.h
> >
> > diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> > new file mode 100644
> > index 0000000..e21d50c
> > --- /dev/null
> > +++ b/android/bt_adapter.c
> > @@ -0,0 +1,56 @@
> > +/*
> > + *
> > + * 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
> > + *
> > + */
> > +
> > +#include "bt_adapter.h"
> > +#include "log.h"
> > +#include "src/shared/mgmt.h"
> > +
> > +struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> > +{
> > + struct bt_adapter *adapter;
> > +
> > + adapter = g_try_new0(struct bt_adapter, 1);
> > + if (!adapter)
> > + return NULL;
> > +
> > + adapter->dev_id = index;
> > + adapter->mgmt = mgmt_ref(mgmt_if);
> > +
> > + return adapter;
> > +}
> > +
> > +void adapter_start(struct bt_adapter *adapter)
> > +{
> > + DBG("enabled %u", adapter->dev_id);
> > +
> > + /* TODO: CB: report scan mode */
> > +
> > + /* TODO: SDP start here */
> > +
> > + /* TODO: CB: report state on */
> > +}
> > +
> > +void adapter_stop(struct bt_adapter *adapter)
> > +{
> > + DBG("disabled %u", adapter->dev_id);
> > +}
> > diff --git a/android/bt_adapter.h b/android/bt_adapter.h
> > new file mode 100644
> > index 0000000..6877cc7
> > --- /dev/null
> > +++ b/android/bt_adapter.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + *
> > + * 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
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <glib.h>
> > +
> > +#include "lib/bluetooth.h"
> > +
> > +struct bt_device {
> > + int refcnt;
> > +
> > + bdaddr_t bdaddr;
> > + uint8_t bdaddr_type;
> > + uint32_t cod;
> > + char *name;
> > +};
>
> I'm not sure if we need struct bt_device. Shouldn't we just use bdaddr_t?
> All data from this struct will be cached in upper layers, so we don't need to
> store it in daemon.
which upper layer?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv3 07/15] android: Create HAL API header skeleton
From: Andrei Emeltchenko @ 2013-10-10 12:29 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <69ADBAAE-9261-4C56-9779-5708DE897CBA@holtmann.org>
On Wed, Oct 09, 2013 at 09:34:17PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> > Header describes the protocol between Android HAL threads and BlueZ
> > daemon.
> > ---
> > android/hal_msg.h | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 255 insertions(+)
> > create mode 100644 android/hal_msg.h
> >
> > diff --git a/android/hal_msg.h b/android/hal_msg.h
> > new file mode 100644
> > index 0000000..c6bc883
> > --- /dev/null
> > +++ b/android/hal_msg.h
> > @@ -0,0 +1,255 @@
> > +/*
> > + *
> > + * BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + * Copyright (C) 2013 Intel Corporation. All rights reserved.
> > + *
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library 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
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > + *
> > + */
> > +
> > +#ifndef __packed
> > +#define __packed __attribute__((packed))
> > +#endif
you want different style from lib/mgmt.h ?
>
> nowhere in the code we are using __packed. That is more a kernel thing. Just use the gcc style directly.
>
> > +
> > +typedef struct {
> > + uint8_t b[6];
> > +} __packed __bdaddr_t;
>
> Just use a uint8_t bdaddr[6] instead.
>
> > +
> > +struct hal_msg_hdr {
> > + uint8_t service_id;
> > + uint8_t opcode;
> > + uint16_t len;
> > + uint8_t payload[0];
>
> I would not include the payload[0] here.
I know that the current approach is to magically add to pointer header
size, but doesn't it looks better?
>
> > +} __packed;
> > +
> > +#define HAL_SERVICE_ID_CORE 0
> > +#define HAL_SERVICE_ID_BLUETOOTH 1
> > +#define HAL_SERVICE_ID_SOCK 2
> > +#define HAL_SERVICE_ID_HIDHOST 3
> > +#define HAL_SERVICE_ID_PAN 4
> > +#define HAL_SERVICE_ID_HANDSFREE 5
> > +#define HAL_SERVICE_ID_AD2P 6
> > +#define HAL_SERVICE_ID_HEALTH 7
> > +#define HAL_SERVICE_ID_AVRCP 8
> > +#define HAL_SERVICE_ID_GATT 9
> > +
> > +/* Core Service */
> > +
> > +#define HAL_MSG_OP_ERROR 0x00
> > +struct hal_msg_rp_error {
> > + uint8_t status;
> > +} __packed;
>
> using RSP, rsp, CMD, cmd, EVT and EVT seems to be clearer I think.
So I want to be sure that I understand this right:
hal_msg_rp_error => hal_msg_rsp_error
hal_msg_cp_register_module => hal_msg_cmd_register_module
this would be different from lib/mgmt.h
> > +
> > +#define HAL_MSG_OP_REGISTER_MODULE 0x01
> > +struct hal_msg_cp_register_module {
> > + uint8_t service_id;
> > +} __packed;
> > +struct hal_msg_rp_register_module {
> > + uint8_t service_id;
> > +} __packed;
> > +
> > +#define HAL_MSG_OP_UNREGISTER_MODULE 0x02
> > +struct hal_msg_cp_unregister_module {
> > + uint8_t service_id;
> > +} __packed;
> > +
> > +/* Bluetooth Core HAL API */
> > +
> > +#define HAL_MSG_OP_BT_ENABLE 0x01
> > +
> > +#define HAL_MSG_OP_BT_DISABLE 0x02
> > +
> > +#define HAL_MSG_OP_BT_GET_ADAPTER_PROPS 0x03
> > +
> > +#define HAL_MSG_OP_BT_GET_ADAPTER_PROP 0x04
> > +struct hal_msg_cp_bt_get_adapter_prop {
> > + uint8_t type;
> > +} __packed;
> > +
> > +#define HAL_MSG_OP_BT_SET_ADAPTER_PROP 0x05
> > +struct hal_msg_cp_bt_set_adapter_prop {
> > + uint8_t type;
> > + uint16_t len;
> > + uint8_t val[0];
> > +} __packed;
> > +
> > +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROPS 0x06
> > +struct hal_msg_cp_bt_get_remote_device_props {
> > + __bdaddr_t bdaddr;
> > +} __packed;
> > +
> > +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROP 0x07
> > +struct hal_msg_cp_bt_get_remote_device_prop {
> > + __bdaddr_t bdaddr;
> > + uint8_t type;
> > +} __packed;
> > +
> > +#define HAL_MSG_OP_BT_SET_REMOTE_DEVICE_PROP 0x08
> > +struct hal_msg_cp_bt_set_remote_device_prop {
> > + __bdaddr_t bdaddr;
> > + uint8_t type;
> > + uint16_t len;
>
> Lets make these all align properly.
Align with tabs? So this will be very different from lib/mgmt.h ?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix potential double-frees of L2CAP skbs
From: Marcel Holtmann @ 2013-10-10 12:01 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1381404817-5645-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> The l2cap_recv_frame function is expected to take ownership and
> eventually free the skb passed to it. We need to ensure that the
> conn->rx_skb pointer is no longer reachable when calling
> l2cap_recv_frame so that no other function, such as l2cap_conn_del, may
> think that it can free conn->rx_skb.
>
> An actual situation when this can happen is when smp_sig_channel (called
> from l2cap_recv_frame) fails and l2cap_conn_del gets called as a
> consequence. The l2cap_conn_del function would then try to free
> conn->rx_skb, but as the same skb was just passed to smp_sig_channel and
> freed we get a double-free.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH] Bluetooth: Fix potential double-frees of L2CAP skbs
From: johan.hedberg @ 2013-10-10 11:33 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
The l2cap_recv_frame function is expected to take ownership and
eventually free the skb passed to it. We need to ensure that the
conn->rx_skb pointer is no longer reachable when calling
l2cap_recv_frame so that no other function, such as l2cap_conn_del, may
think that it can free conn->rx_skb.
An actual situation when this can happen is when smp_sig_channel (called
from l2cap_recv_frame) fails and l2cap_conn_del gets called as a
consequence. The l2cap_conn_del function would then try to free
conn->rx_skb, but as the same skb was just passed to smp_sig_channel and
freed we get a double-free.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a9a7df6..06e7173 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -6798,9 +6798,13 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
conn->rx_len -= skb->len;
if (!conn->rx_len) {
- /* Complete frame received */
- l2cap_recv_frame(conn, conn->rx_skb);
+ /* Complete frame received. l2cap_recv_frame
+ * takes ownership of the skb so set the global
+ * rx_skb pointer to NULL first.
+ */
+ struct sk_buff *rx_skb = conn->rx_skb;
conn->rx_skb = NULL;
+ l2cap_recv_frame(conn, rx_skb);
}
break;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Marcel Holtmann @ 2013-10-10 11:07 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131010110117.GI23879@aemeltch-MOBL1>
Hi Andrei,
>> The variable val in the set_ssp() function of the management interface
>> is not needed. Just use cp->val directly since its input values have
>> already been validated.
>>
>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>> ---
>> net/bluetooth/mgmt.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index a9d7506..2fb4d35 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -1290,7 +1290,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>> {
>> struct mgmt_mode *cp = data;
>> struct pending_cmd *cmd;
>> - u8 val, status;
>> + u8 status;
>> int err;
>>
>> BT_DBG("request for %s", hdev->name);
>> @@ -1309,8 +1309,6 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>>
>> hci_dev_lock(hdev);
>>
>> - val = !!cp->val;
>> -
>> if (!hdev_is_powered(hdev)) {
>> bool changed = false;
>>
>> @@ -1335,7 +1333,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>> goto failed;
>> }
>>
>> - if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) == val) {
>> + if (!!cp->val == test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
>> err = send_settings_rsp(sk, MGMT_OP_SET_SSP, hdev);
>> goto failed;
>> }
>> @@ -1346,7 +1344,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>> goto failed;
>> }
>>
>> - err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, sizeof(val), &val);
>> + err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &cp->val);
>
> I think sizeof is read better then magic number
if you look at the whole code base, for cases of single u8, we use the "magic" number 1 a lot to keep the lines shorter.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Marcel Holtmann @ 2013-10-10 11:06 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: BlueZ development
In-Reply-To: <CAJdJm_Ne5rQ2G8_Je_en1kr9dxw76G_qW9gsm0WvH2On9x3dPw@mail.gmail.com>
Hi Anderson,
>> The variable val in the set_ssp() function of the management interface
>> is not needed. Just use cp->val directly since its input values have
>> already been validated.
>> [...]
>> - if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) == val) {
>> + if (!!cp->val == test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
>
> In this case, the "!!" trick is unnecessary as the only allowed values
> are 0x00 and 0x01 (I believe this has been removed in other similar
> cases).
it has not been removed. That is why I changed it and made it similar to the other case.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Andrei Emeltchenko @ 2013-10-10 11:01 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1381399691-42391-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 10, 2013 at 03:08:10AM -0700, Marcel Holtmann wrote:
> The variable val in the set_ssp() function of the management interface
> is not needed. Just use cp->val directly since its input values have
> already been validated.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/mgmt.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a9d7506..2fb4d35 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1290,7 +1290,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> {
> struct mgmt_mode *cp = data;
> struct pending_cmd *cmd;
> - u8 val, status;
> + u8 status;
> int err;
>
> BT_DBG("request for %s", hdev->name);
> @@ -1309,8 +1309,6 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>
> hci_dev_lock(hdev);
>
> - val = !!cp->val;
> -
> if (!hdev_is_powered(hdev)) {
> bool changed = false;
>
> @@ -1335,7 +1333,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> goto failed;
> }
>
> - if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) == val) {
> + if (!!cp->val == test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
> err = send_settings_rsp(sk, MGMT_OP_SET_SSP, hdev);
> goto failed;
> }
> @@ -1346,7 +1344,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> goto failed;
> }
>
> - err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, sizeof(val), &val);
> + err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &cp->val);
I think sizeof is read better then magic number
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Anderson Lizardo @ 2013-10-10 11:00 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <1381399691-42391-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 10, 2013 at 6:08 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> The variable val in the set_ssp() function of the management interface
> is not needed. Just use cp->val directly since its input values have
> already been validated.
> [...]
> - if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) == val) {
> + if (!!cp->val == test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
In this case, the "!!" trick is unnecessary as the only allowed values
are 0x00 and 0x01 (I believe this has been removed in other similar
cases).
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Johan Hedberg @ 2013-10-10 10:56 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1381399691-42391-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 10, 2013, Marcel Holtmann wrote:
> The variable val in the set_ssp() function of the management interface
> is not needed. Just use cp->val directly since its input values have
> already been validated.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/mgmt.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
Both patches have been applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* [PATCH 2/2] Bluetooth: Restrict high speed support to SSP enabled controllers
From: Marcel Holtmann @ 2013-10-10 10:08 UTC (permalink / raw)
To: linux-bluetooth
The support for Bluetooth High Speed can only be enabled on controllers
where also Secure Simple Pairing has been enabled. Trying to enable
high speed when SSP is disabled will result into an error. Disabling
SSP will at the same time disable high speed as well.
It is required to enforce this dependency on SSP since high speed
support is only defined for authenticated, unauthenticated and
debug link keys. These link key types require SSP.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 2fb4d35..e7ffd39 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1310,11 +1310,19 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
hci_dev_lock(hdev);
if (!hdev_is_powered(hdev)) {
- bool changed = false;
+ bool changed;
- if (val != test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
- change_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
- changed = true;
+ if (cp->val) {
+ changed = !test_and_set_bit(HCI_SSP_ENABLED,
+ &hdev->dev_flags);
+ } else {
+ changed = test_and_clear_bit(HCI_SSP_ENABLED,
+ &hdev->dev_flags);
+ if (!changed)
+ changed = test_and_clear_bit(HCI_HS_ENABLED,
+ &hdev->dev_flags);
+ else
+ clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
}
err = send_settings_rsp(sk, MGMT_OP_SET_SSP, hdev);
@@ -1327,7 +1335,8 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
goto failed;
}
- if (mgmt_pending_find(MGMT_OP_SET_SSP, hdev)) {
+ if (mgmt_pending_find(MGMT_OP_SET_SSP, hdev) ||
+ mgmt_pending_find(MGMT_OP_SET_HS, hdev)) {
err = cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
MGMT_STATUS_BUSY);
goto failed;
@@ -1368,6 +1377,14 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
if (status)
return cmd_status(sk, hdev->id, MGMT_OP_SET_HS, status);
+ if (!lmp_ssp_capable(hdev))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+ MGMT_STATUS_NOT_SUPPORTED);
+
+ if (!test_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+ MGMT_STATUS_REJECTED);
+
if (cp->val != 0x00 && cp->val != 0x01)
return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
MGMT_STATUS_INVALID_PARAMS);
@@ -4403,8 +4420,10 @@ int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
u8 mgmt_err = mgmt_status(status);
if (enable && test_and_clear_bit(HCI_SSP_ENABLED,
- &hdev->dev_flags))
+ &hdev->dev_flags)) {
+ clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
err = new_settings(hdev, NULL);
+ }
mgmt_pending_foreach(MGMT_OP_SET_SSP, hdev, cmd_status_rsp,
&mgmt_err);
@@ -4413,11 +4432,14 @@ int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
}
if (enable) {
- if (!test_and_set_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
- changed = true;
+ changed = !test_and_set_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
} else {
- if (test_and_clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
- changed = true;
+ changed = test_and_clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
+ if (!changed)
+ changed = test_and_clear_bit(HCI_HS_ENABLED,
+ &hdev->dev_flags);
+ else
+ clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
}
mgmt_pending_foreach(MGMT_OP_SET_SSP, hdev, settings_rsp, &match);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] Bluetooth: Remove unneeded val variable when setting SSP
From: Marcel Holtmann @ 2013-10-10 10:08 UTC (permalink / raw)
To: linux-bluetooth
The variable val in the set_ssp() function of the management interface
is not needed. Just use cp->val directly since its input values have
already been validated.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/mgmt.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a9d7506..2fb4d35 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1290,7 +1290,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
{
struct mgmt_mode *cp = data;
struct pending_cmd *cmd;
- u8 val, status;
+ u8 status;
int err;
BT_DBG("request for %s", hdev->name);
@@ -1309,8 +1309,6 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
hci_dev_lock(hdev);
- val = !!cp->val;
-
if (!hdev_is_powered(hdev)) {
bool changed = false;
@@ -1335,7 +1333,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
goto failed;
}
- if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) == val) {
+ if (!!cp->val == test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
err = send_settings_rsp(sk, MGMT_OP_SET_SSP, hdev);
goto failed;
}
@@ -1346,7 +1344,7 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
goto failed;
}
- err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, sizeof(val), &val);
+ err = hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &cp->val);
if (err < 0) {
mgmt_pending_remove(cmd);
goto failed;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCHv3 06/15] android: Add basic mgmt initialization sequence
From: Andrei Emeltchenko @ 2013-10-10 9:59 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <0147C8F8-0491-44CD-BCDB-51DF9F6D3315@holtmann.org>
Hi Marcel,
On Wed, Oct 09, 2013 at 09:30:22PM +0200, Marcel Holtmann wrote:
> 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.
>
So how do you want it to be? Here I just added BlueZ top-dir.
> > 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.
>
Is this fix good:
diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
index 2c79886..b2f5506 100644
--- a/src/shared/mgmt.c
+++ b/src/shared/mgmt.c
@@ -55,7 +55,7 @@ struct mgmt {
unsigned int next_notify_id;
bool in_notify;
bool destroyed;
- void *buf;
+ uint8_t *buf;
uint16_t len;
mgmt_debug_func_t debug_callback;
mgmt_destroy_func_t debug_destroy;
@@ -299,7 +299,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition cond,
if (bytes_read < MGMT_HDR_SIZE)
return TRUE;
- hdr = mgmt->buf;
+ hdr = (struct mgmt_hdr *) mgmt->buf;
event = btohs(hdr->opcode);
index = btohs(hdr->index);
length = btohs(hdr->len);
@@ -309,7 +309,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition cond,
switch (event) {
case MGMT_EV_CMD_COMPLETE:
- cc = mgmt->buf + MGMT_HDR_SIZE;
+ cc = (struct mgmt_ev_cmd_complete *) mgmt->buf +
MGMT_HDR_SIZE;
opcode = btohs(cc->opcode);
util_debug(mgmt->debug_callback, mgmt->debug_data,
@@ -320,7 +320,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition cond,
mgmt->buf + MGMT_HDR_SIZE
+ 3);
break;
case MGMT_EV_CMD_STATUS:
- cs = mgmt->buf + MGMT_HDR_SIZE;
+ cs = (struct mgmt_ev_cmd_status *) mgmt->buf +
MGMT_HDR_SIZE;
opcode = btohs(cs->opcode);
util_debug(mgmt->debug_callback, mgmt->debug_data,
> > +
> > +# 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.
The problem here is that our base android-ia from 01.org is based on
4.2.2. Do you still want to remove this?
Best regards
Andrei Emeltchenko
^ permalink raw reply related
* [PATCH v2] Don't register Device ID record on sdp server start
From: Szymon Janc @ 2013-10-10 9:38 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1381397344-7389-1-git-send-email-szymon.janc@tieto.com>
This makes SDP code no longer depends on main_opts. DID record is now
registered from main() after sdp server was started. This is OK since
mainloop is not yet running and record will be present when first
request comes.
---
v2 - commit message improved
src/main.c | 4 ++++
src/sdpd-server.c | 5 -----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/main.c b/src/main.c
index eafe2ed..91d90b4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -550,6 +550,10 @@ int main(int argc, char *argv[])
start_sdp_server(sdp_mtu, sdp_flags);
+ if (main_opts.did_source > 0)
+ register_device_id(main_opts.did_source, main_opts.did_vendor,
+ main_opts.did_product, main_opts.did_version);
+
/* Loading plugins has to be done after D-Bus has been setup since
* the plugins might wanna expose some paths on the bus. However the
* best order of how to init various subsystems of the Bluetooth
diff --git a/src/sdpd-server.c b/src/sdpd-server.c
index 10e46a1..b411abe 100644
--- a/src/sdpd-server.c
+++ b/src/sdpd-server.c
@@ -42,7 +42,6 @@
#include <glib.h>
-#include "hcid.h"
#include "log.h"
#include "sdpd.h"
@@ -238,10 +237,6 @@ int start_sdp_server(uint16_t mtu, uint32_t flags)
return -1;
}
- if (main_opts.did_source > 0)
- register_device_id(main_opts.did_source, main_opts.did_vendor,
- main_opts.did_product, main_opts.did_version);
-
io = g_io_channel_unix_new(l2cap_sock);
g_io_channel_set_close_on_unref(io, TRUE);
--
1.8.4
^ permalink raw reply related
* [PATCH] Don't register Device ID record on sdp server start
From: Szymon Janc @ 2013-10-10 9:29 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
DID record is now registered from main() after sdp server was started.
This is OK since mainloop is not yet running and record will be present
when first request comes.
Another benefit is that sdp code no longer depends on main_opts.
---
src/main.c | 4 ++++
src/sdpd-server.c | 5 -----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/main.c b/src/main.c
index eafe2ed..91d90b4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -550,6 +550,10 @@ int main(int argc, char *argv[])
start_sdp_server(sdp_mtu, sdp_flags);
+ if (main_opts.did_source > 0)
+ register_device_id(main_opts.did_source, main_opts.did_vendor,
+ main_opts.did_product, main_opts.did_version);
+
/* Loading plugins has to be done after D-Bus has been setup since
* the plugins might wanna expose some paths on the bus. However the
* best order of how to init various subsystems of the Bluetooth
diff --git a/src/sdpd-server.c b/src/sdpd-server.c
index 10e46a1..b411abe 100644
--- a/src/sdpd-server.c
+++ b/src/sdpd-server.c
@@ -42,7 +42,6 @@
#include <glib.h>
-#include "hcid.h"
#include "log.h"
#include "sdpd.h"
@@ -238,10 +237,6 @@ int start_sdp_server(uint16_t mtu, uint32_t flags)
return -1;
}
- if (main_opts.did_source > 0)
- register_device_id(main_opts.did_source, main_opts.did_vendor,
- main_opts.did_product, main_opts.did_version);
-
io = g_io_channel_unix_new(l2cap_sock);
g_io_channel_set_close_on_unref(io, TRUE);
--
1.8.4
^ permalink raw reply related
* Re: [PATCHv3 08/15] android: Add adapter and device struct for BlueZ daemon
From: Marcin Kraglak @ 2013-10-10 9:07 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1381243883-2745-9-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On 8 October 2013 16:51, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Adapter structure in BlueZ daemon keeps track of default adapter
> and device structure keeps track about found devices.
> ---
> android/bt_adapter.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
> android/bt_adapter.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 116 insertions(+)
> create mode 100644 android/bt_adapter.c
> create mode 100644 android/bt_adapter.h
>
> diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> new file mode 100644
> index 0000000..e21d50c
> --- /dev/null
> +++ b/android/bt_adapter.c
> @@ -0,0 +1,56 @@
> +/*
> + *
> + * 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
> + *
> + */
> +
> +#include "bt_adapter.h"
> +#include "log.h"
> +#include "src/shared/mgmt.h"
> +
> +struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> +{
> + struct bt_adapter *adapter;
> +
> + adapter = g_try_new0(struct bt_adapter, 1);
> + if (!adapter)
> + return NULL;
> +
> + adapter->dev_id = index;
> + adapter->mgmt = mgmt_ref(mgmt_if);
> +
> + return adapter;
> +}
> +
> +void adapter_start(struct bt_adapter *adapter)
> +{
> + DBG("enabled %u", adapter->dev_id);
> +
> + /* TODO: CB: report scan mode */
> +
> + /* TODO: SDP start here */
> +
> + /* TODO: CB: report state on */
> +}
> +
> +void adapter_stop(struct bt_adapter *adapter)
> +{
> + DBG("disabled %u", adapter->dev_id);
> +}
> diff --git a/android/bt_adapter.h b/android/bt_adapter.h
> new file mode 100644
> index 0000000..6877cc7
> --- /dev/null
> +++ b/android/bt_adapter.h
> @@ -0,0 +1,60 @@
> +/*
> + *
> + * 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
> + *
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <glib.h>
> +
> +#include "lib/bluetooth.h"
> +
> +struct bt_device {
> + int refcnt;
> +
> + bdaddr_t bdaddr;
> + uint8_t bdaddr_type;
> + uint32_t cod;
> + char *name;
> +};
I'm not sure if we need struct bt_device. Shouldn't we just use bdaddr_t?
All data from this struct will be cached in upper layers, so we don't need to
store it in daemon.
> +
> +struct bt_adapter {
> + int refcnt;
> +
> + uint16_t dev_id;
> + struct mgmt *mgmt;
> + bdaddr_t bdaddr;
> + uint32_t dev_class;
> +
> + char *name;
> + char *short_name;
> +
> + uint32_t supported_settings;
> + uint32_t current_settings;
> +
> + GList *found_devices;
> +};
> +
> +struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if);
> +
> +void adapter_start(struct bt_adapter *adapter);
> +void adapter_stop(struct bt_adapter *adapter);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Best regards
Marcin
^ permalink raw reply
* [PATCH 6/6] avdtp: Remove not needed forward declaration
From: Szymon Janc @ 2013-10-10 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1381395221-5067-1-git-send-email-szymon.janc@tieto.com>
auth_cb function is not used before definition so no need to forward
declare it.
---
profiles/audio/avdtp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index e5d4139..5b8d7b1 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -442,7 +442,6 @@ static int process_queue(struct avdtp *session);
static void avdtp_sep_set_state(struct avdtp *session,
struct avdtp_local_sep *sep,
avdtp_state_t state);
-static void auth_cb(DBusError *derr, void *user_data);
static struct avdtp_server *find_server(GSList *list, struct btd_adapter *a)
{
--
1.8.4
^ permalink raw reply related
* [PATCH 5/6] avdtp: Move connection_lost function to avoid forward declaration
From: Szymon Janc @ 2013-10-10 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1381395221-5067-1-git-send-email-szymon.janc@tieto.com>
---
profiles/audio/avdtp.c | 53 +++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index bf15935..e5d4139 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -439,7 +439,6 @@ static gboolean avdtp_parse_rej(struct avdtp *session,
uint8_t transaction, uint8_t signal_id,
void *buf, int size);
static int process_queue(struct avdtp *session);
-static void connection_lost(struct avdtp *session, int err);
static void avdtp_sep_set_state(struct avdtp *session,
struct avdtp_local_sep *sep,
avdtp_state_t state);
@@ -1137,6 +1136,32 @@ static void avdtp_free(void *data)
g_free(session);
}
+static void connection_lost(struct avdtp *session, int err)
+{
+ struct avdtp_server *server = session->server;
+ char address[18];
+
+ ba2str(device_get_address(session->device), address);
+ DBG("Disconnected from %s", address);
+
+ if (err != EACCES)
+ avdtp_cancel_authorization(session);
+
+ g_slist_foreach(session->streams, (GFunc) release_stream, session);
+ session->streams = NULL;
+
+ finalize_discovery(session, err);
+
+ avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
+
+ if (session->ref > 0)
+ return;
+
+ server->sessions = g_slist_remove(server->sessions, session);
+ btd_device_unref(session->device);
+ avdtp_free(session);
+}
+
static gboolean disconnect_timeout(gpointer user_data)
{
struct avdtp *session = user_data;
@@ -1180,32 +1205,6 @@ static void set_disconnect_timer(struct avdtp *session)
session);
}
-static void connection_lost(struct avdtp *session, int err)
-{
- struct avdtp_server *server = session->server;
- char address[18];
-
- ba2str(device_get_address(session->device), address);
- DBG("Disconnected from %s", address);
-
- if (err != EACCES)
- avdtp_cancel_authorization(session);
-
- g_slist_foreach(session->streams, (GFunc) release_stream, session);
- session->streams = NULL;
-
- finalize_discovery(session, err);
-
- avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
-
- if (session->ref > 0)
- return;
-
- server->sessions = g_slist_remove(server->sessions, session);
- btd_device_unref(session->device);
- avdtp_free(session);
-}
-
void avdtp_unref(struct avdtp *session)
{
if (!session)
--
1.8.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox