* Re: [RFC] Bluetooth Low energy support
From: Ville Tervo @ 2010-11-10 12:51 UTC (permalink / raw)
To: ext Anderson Lizardo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTi=iCXfycMzHbgwS3jbvCxJvzVh97iSDRf4WhQeQ@mail.gmail.com>
On Wed, Nov 10, 2010 at 11:46:47AM +0100, ext Anderson Lizardo wrote:
> On Wed, Nov 10, 2010 at 2:20 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
> >> While doing tests with your most recent trees (using devel HW from
> >> TI), I'm getting consistent panic on the following test:
> >>
> >> (dev1) hciconfig hciX leadv
> >> (dev2) hcitool -i hciX lecc <dev2_addr>
> >
> > Did you have server listening on dev 2 on ATT socket?
>
> I had no bluetoothd running on any side (not sure if that was your question).
That was actually my question :) Thanks.
--
VIlle
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Elvis Pfützenreuter @ 2010-11-10 12:40 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTim=3tZWao0Kzqq8nr4Z=dOrXL_aK76Ernx=UqQS@mail.gmail.com>
> Hi Elvis,
>
> 2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>>>> ---
>>>> health/hdp.c | 1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/health/hdp.c b/health/hdp.c
>>>> index 1eba8e1..d361b27 100644
>>>> --- a/health/hdp.c
>>>> +++ b/health/hdp.c
>>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>>> if (!hdp_device->mcl)
>>>> return;
>>>>
>>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>>> mcap_mcl_unref(hdp_device->mcl);
>>>> hdp_device->mcl = NULL;
>>>> hdp_device->mcl_conn = FALSE;
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>>
>> Yes, it seems to have fixed the problem. And far cleaner :)
>>
>> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
>
> This function doesn't disable the caching in general, it just closes
> this MCL without catching it, but caching is still active for all the
> other MCL's. Additionally, nothing happens if mcap_close_mcl is called
> more than once, so this patch takes care of this too.
Indeed, MCL disconnection and hdp_device destruction are different moments. Still fulfilling the caffeine quota for this morning :P
>>
>> So, perhaps it would be better to get rid of caching code in mcap.c?
>
> I can't understand this, can you explain this more?. As I see, MCAP
> should do the catching because it holds all the information about the
> channels that are connected and can cache it, HDP could not do this
> without MCAP. More over, if in the future more profiles use MCAP they
> may want to cache too.
I do have the opinion that caching in mcap.c is more complicated than it could be. I'd have used a single list with a CLOSED flag. Also, there is some confusion in nomenclature (mcap_uncache_mcl() recovers from the cache, while mcl_uncached_cb callback means that MCL has been invalidated). This is source of bugs like this one.
At the very least, cached MCLs should have mcl->cb zeroed. if MCAP level caches MCLs, very well, but HDP level should be asked to set callbacks and user data again, as if it were a new MCL. (Actually, mcl_reconnected in hdp.c seems to do that; not sure why it did not avoid the particular bug that is under discussion.)
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 11:49 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <126814B4-25DA-4BA4-A0B8-0E5D57001EF7@signove.com>
Hi Elvis,
2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>>> ---
>>> health/hdp.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/health/hdp.c b/health/hdp.c
>>> index 1eba8e1..d361b27 100644
>>> --- a/health/hdp.c
>>> +++ b/health/hdp.c
>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>> if (!hdp_device->mcl)
>>> return;
>>>
>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>> mcap_mcl_unref(hdp_device->mcl);
>>> hdp_device->mcl = NULL;
>>> hdp_device->mcl_conn = FALSE;
>>> --
>>> 1.7.1
>>>
>>>
>>
>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>
> Yes, it seems to have fixed the problem. And far cleaner :)
>
> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
This function doesn't disable the caching in general, it just closes
this MCL without catching it, but caching is still active for all the
other MCL's. Additionally, nothing happens if mcap_close_mcl is called
more than once, so this patch takes care of this too.
>
> The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
This is because HDP wants to cache as long as possible.
>
> So, perhaps it would be better to get rid of caching code in mcap.c?
I can't understand this, can you explain this more?. As I see, MCAP
should do the catching because it holds all the information about the
channels that are connected and can cache it, HDP could not do this
without MCAP. More over, if in the future more profiles use MCAP they
may want to cache too.
Regards
^ permalink raw reply
* [PATCH] Fix pull phonebook reply if filter not set
From: Lukasz Pawlik @ 2010-11-10 11:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lukasz Pawlik
According to the PBAP specification if filter is not set or is set to
0x00000000 in the application parameters header all attributes of the vCard
should be returned. Previously only mandatory attributes were returned in
phonebook pull reply. This patch fix this and now all currently supported
vCards attributes will be returned.
---
plugins/vcard.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/plugins/vcard.c b/plugins/vcard.c
index 41f9fbd..3f69189 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -457,10 +457,16 @@ static void vcard_printf_end(GString *vcards)
void phonebook_add_contact(GString *vcards, struct phonebook_contact *contact,
uint64_t filter, uint8_t format)
{
- if (format == FORMAT_VCARD30)
+ if (format == FORMAT_VCARD30 && filter)
filter |= (FILTER_VERSION | FILTER_FN | FILTER_N | FILTER_TEL);
- else if (format == FORMAT_VCARD21)
+ else if (format == FORMAT_VCARD21 && filter)
filter |= (FILTER_VERSION | FILTER_N | FILTER_TEL);
+ else
+ filter = (FILTER_VERSION | FILTER_UID | FILTER_N | FILTER_FN |
+ FILTER_TEL | FILTER_EMAIL | FILTER_ADR |
+ FILTER_BDAY | FILTER_NICKNAME | FILTER_URL |
+ FILTER_PHOTO | FILTER_ORG | FILTER_ROLE |
+ FILTER_TITLE | FILTER_X_IRMC_CALL_DATETIME);
vcard_printf_begin(vcards, format);
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Elvis Pfützenreuter @ 2010-11-10 11:36 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimoQRAHXJ65xa1-TMKGUfLujw8f9MhzxrxPXHmY@mail.gmail.com>
>> ---
>> health/hdp.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/health/hdp.c b/health/hdp.c
>> index 1eba8e1..d361b27 100644
>> --- a/health/hdp.c
>> +++ b/health/hdp.c
>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>> if (!hdp_device->mcl)
>> return;
>>
>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>> mcap_mcl_unref(hdp_device->mcl);
>> hdp_device->mcl = NULL;
>> hdp_device->mcl_conn = FALSE;
>> --
>> 1.7.1
>>
>>
>
> Please Elvis, try this solution and tell us if it fix the segfault problem.
Yes, it seems to have fixed the problem. And far cleaner :)
I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
So, perhaps it would be better to get rid of caching code in mcap.c?
^ permalink raw reply
* RE: [PATCH 1/4] Sim Access Profile API
From: Waldemar.Rymarkiewicz @ 2010-11-10 11:12 UTC (permalink / raw)
To: marcel; +Cc: linux-bluetooth, suraj, johan.hedberg, joakim.xj.ceder
In-Reply-To: <1289367190.9615.235.camel@aeonflux>
Hi Marcel,=20
>> + void Disable()
>> +
>> + Shudown SAP server and remove the SDP record.
>> +
>> + Possible errors: org.bluez.Error.Failed
>
>I don't like this. If you have properties then just changing=20
>the property should be enough. So a SetProperty is more appropriate.
I see another option to get rid of 'Enabled' property and leave the methods=
. What would you say on that?
>> +
>> + void Disconnect(boolean type)
>> +
>> + Disconnect SAP client from the server.=20
>The 'type'
>> + parameter indicates disconnection type.
>> +
>> + True - gracefull disconnection
>> + False - immediate disconnection
>> +
>> + Possible errors: org.bluez.Error.Failed
>
>I don't like this style of method names at all. Using method=20
>names like GracefulDisconnect and ImmediateDisconnect would be better.
That's fine.
>However I am not sure we should differentiate here at all. We=20
>should always to the graceful disconnect. What will the=20
>immediate disconnect bring us?
That's actually intended for testing only. One of PTS test cases expects th=
e tester to trigger immediate disconnect.
In practce, it is only used when connection to sim card is lost, but this i=
s obviously done internally.
Thanks,
/Waldek=
^ permalink raw reply
* Re: [RFC] Bluetooth Low energy support
From: Anderson Lizardo @ 2010-11-10 10:46 UTC (permalink / raw)
To: Ville Tervo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101110062030.GE20384@null>
On Wed, Nov 10, 2010 at 2:20 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
>> While doing tests with your most recent trees (using devel HW from
>> TI), I'm getting consistent panic on the following test:
>>
>> (dev1) hciconfig hciX leadv
>> (dev2) hcitool -i hciX lecc <dev2_addr>
>
> Did you have server listening on dev 2 on ATT socket?
I had no bluetoothd running on any side (not sure if that was your question).
>> I attached two logs. One is from dev1 machine (which has the oops),
>> the other is from the dev2 machine.
>
> I'll try to fix this today.
Thanks,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 9:49 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jose Antonio Santos Cadenas
In-Reply-To: <1289382461-10510-1-git-send-email-santoscadenas@gmail.com>
2010/11/10 Jose Antonio Santos Cadenas <santoscadenas@gmail.com>:
> ---
> health/hdp.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index 1eba8e1..d361b27 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
> if (!hdp_device->mcl)
> return;
>
> + mcap_close_mcl(hdp_device->mcl, FALSE);
> mcap_mcl_unref(hdp_device->mcl);
> hdp_device->mcl = NULL;
> hdp_device->mcl_conn = FALSE;
> --
> 1.7.1
>
>
Please Elvis, try this solution and tell us if it fix the segfault problem.
Regards
^ permalink raw reply
* [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 9:47 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jose Antonio Santos Cadenas
In-Reply-To: <1289358915-6612-1-git-send-email-epx@signove.com>
---
health/hdp.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index 1eba8e1..d361b27 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
if (!hdp_device->mcl)
return;
+ mcap_close_mcl(hdp_device->mcl, FALSE);
mcap_mcl_unref(hdp_device->mcl);
hdp_device->mcl = NULL;
hdp_device->mcl_conn = FALSE;
--
1.7.1
^ permalink raw reply related
* Re: Ubuntu 10.04 - BlueZ 4.60 - Console-only BlueZ setup
From: elroy @ 2010-11-10 8:11 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <930b6b5d0fcdfd10087260af984b94ac@libresoft.es>
José Antonio Santos Cadenas wrote:
> Hi Elroy,
>
>
>
> Did you try simple-agent? It is a python scrypt in the bluez
> test/folder that doesn't require any gui. I don't if Ubuntu installs it,
> but if you download the source code from the web page or the git repo
> you will find it. Probably with this agent you will be able to pair your
> cell phone.
>
>
>>
>>I have been documenting my progress so far, to try to aid others
>>following my path - this may be useful to elaborate on what I am doing
>>and have achieved so far.
>>
>>
>>Regards,
>>
>>Elroy Liddington.
Cheers Jose.
I'll add this gem to the page I'm documenting this on.
Looks like those other scripts would be useful too.
How would I be able to help in regards to knocking up a basic BlueZ MAN
page etc so that others could at least be pointed in the right direction
on where to go for help/docs?
Regards,
Elroy Liddington.
^ permalink raw reply
* Re: [PATCH] [RFC] Fix HDP-related segfault upon device recreation
From: Jose Antonio Santos Cadenas @ 2010-11-10 8:02 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <1289358915-6612-1-git-send-email-epx@signove.com>
Hi Elvis,
2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
> When device is removed and paired again, hdp_device is destroyed
> but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
> This patch ensures that such MCL is destroyed.
>
> There is probably a better solution to this e.g. changing cache
> strategy for accepted connections. A loop can be removed if 1:1
> relationship between hdp_device and MCL is guaranteed.
> ---
> health/hdp.c | 1 +
> health/mcap.c | 23 ++++++++++++++++++++++-
> health/mcap_lib.h | 2 ++
> 3 files changed, 25 insertions(+), 1 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index b141fe7..44ebe75 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
> }
>
> device_unref_mcl(device);
> + mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
>
> g_free(device);
> }
> diff --git a/health/mcap.c b/health/mcap.c
> index cb7b74c..34ccdaa 100644
> --- a/health/mcap.c
> +++ b/health/mcap.c
> @@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
> return TRUE;
> }
>
> +static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
> +{
> + const struct mcap_mcl *mcl = a;
> + gconstpointer user_data = b;
> +
> + if (mcl->cb)
> + if (mcl->cb->user_data == user_data)
> + return 0;
> +
> + return 1;
> +}
> +
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
> +{
> + GSList *l;
> +
> + while ((l = g_slist_find_custom(mi->cached, user_data,
> + cmp_mcl_user_data)))
> + mcap_close_mcl(l->data, FALSE);
> +}
> +
> void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
> {
> bacpy(addr, &mcl->addr);
> @@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
>
> return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
> BT_IO_OPT_INVALID);
> -}
> \ No newline at end of file
> +}
> diff --git a/health/mcap_lib.h b/health/mcap_lib.h
> index 7740623..cc10c17 100644
> --- a/health/mcap_lib.h
> +++ b/health/mcap_lib.h
> @@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
> struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
> void mcap_mcl_unref(struct mcap_mcl *mcl);
>
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
> +
> /* CSP operations */
>
> void mcap_enable_csp(struct mcap_instance *ms);
I'm not sure if this is the best way to face this issue. It seems that
this solution is a workaround to avoid the real problem. Let me have a
look and search for a better solution.
Regards.
^ permalink raw reply
* Re: [RFC] Bluetooth Low energy support
From: Ville Tervo @ 2010-11-10 6:20 UTC (permalink / raw)
To: ext Anderson Lizardo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTina_HUT3SGEUMemVLrD4N3+Ni+xCsaC57whHDZG@mail.gmail.com>
Hi,
On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
> Hi Ville,
>
> On Mon, Oct 18, 2010 at 9:02 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > Hi,
> >
> > Here is v2 of bluetooth low energy patch set.
> > Changes from previous version.
>
> While doing tests with your most recent trees (using devel HW from
> TI), I'm getting consistent panic on the following test:
>
> (dev1) hciconfig hciX leadv
> (dev2) hcitool -i hciX lecc <dev2_addr>
Did you have server listening on dev 2 on ATT socket?
>
> I attached two logs. One is from dev1 machine (which has the oops),
> the other is from the dev2 machine.
I'll try to fix this today.
--
Ville
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: btwilink driver
From: Marcel Holtmann @ 2010-11-10 6:08 UTC (permalink / raw)
To: pavan_savoy; +Cc: padovan, linux-bluetooth, linux-kernel
In-Reply-To: <1287525975-17187-1-git-send-email-pavan_savoy@ti.com>
Hi Pavan,
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..8e0de9a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> Say Y here to compile support for "Atheros firmware download driver"
> into the kernel or say M to compile it as module (ath3k).
>
> +config BT_WILINK
> + tristate "Texas Instruments WiLink7 driver"
> + depends on TI_ST
> + help
> + This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
> + combo devices. This makes use of shared transport line discipline
> + core driver to communicate with the BT core of the combo chip.
> +
> + Say Y here to compile support for Texas Instrument's WiLink7 driver
> + into the kernel or say M to compile it as module.
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..f4460f4 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o
> obj-$(CONFIG_BT_ATH3K) += ath3k.o
> obj-$(CONFIG_BT_MRVL) += btmrvl.o
> obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK) += btwilink.o
>
> btmrvl-y := btmrvl_main.o
> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..218efd6
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,411 @@
> +/*
> + * Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * Bluetooth Driver acts as interface between HCI core and
> + * TI Shared Transport Layer.
> + *
> + * Copyright (C) 2009-2010 Texas Instruments
> + * Author: Raja Mani <raja_mani@ti.com>
> + * Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> +
> +/**
> + * struct ti_st - driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @reg_status: ST registration callback status
> + * @st_write: write function provided by the ST driver
> + * to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + struct hci_dev *hdev;
> + char reg_status;
> + long (*st_write) (struct sk_buff *);
> + struct completion wait_reg_completion;
> +};
> +
> +static int reset;
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + struct hci_dev *hdev;
> + hdev = hst->hdev;
please do this properly. Just write it like this:
struct hci_dev *hdev = hst->hdev;
+
> + /* Update HCI stat counters */
> + switch (pkt_type) {
> + case HCI_COMMAND_PKT:
> + hdev->stat.cmd_tx++;
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + hdev->stat.acl_tx++;
> + break;
> +
> + case HCI_SCODATA_PKT:
> + hdev->stat.sco_tx++;
> + break;
> + }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> + struct ti_st *lhst = priv_data;
> +
> + /* Save registration status for use in ti_st_open() */
> + lhst->reg_status = data;
> + /* complete the wait in ti_st_open() */
> + complete(&lhst->wait_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> + int err;
> + struct ti_st *lhst = priv_data;
I really prefer if the variable with the assignment comes first.
> + if (!skb)
> + return -EFAULT;
> +
> + if (!lhst) {
> + kfree_skb(skb);
> + return -EFAULT;
> + }
> +
> + skb->dev = (struct net_device *)lhst->hdev;
Don't do this cast. See the other drivers where we just use (void *)
cast.
> + /* Forward skb to HCI core layer */
> + err = hci_recv_frame(skb);
> + if (err) {
> + kfree_skb(skb);
> + BT_ERR("Unable to push skb to HCI core(%d)", err);
> + return err;
> + }
So first of all, I prefer if you check like this:
if (err < 0) {
And then second, you are double freeing the SKB here. The hci_recv_frame
will free the SKB in an error case.
> +
> + lhst->hdev->stat.byte_rx += skb->len;
> +
> + return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto = {
> + .type = ST_BT,
> + .recv = st_receive,
> + .reg_complete_cb = st_registration_completion_cb,
> + .priv_data = NULL,
> +};
Please don't bother with NULL assignment. It should not be needed.
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + unsigned long timeleft;
> + struct ti_st *hst;
> + int err;
> +
> + BT_DBG("%s %p", hdev->name, hdev);
> +
> + /* provide contexts for callbacks from ST */
> + hst = hdev->driver_data;
> + ti_st_proto.priv_data = hst;
> +
> + err = st_register(&ti_st_proto);
> + if (err == -EINPROGRESS) {
> + /* Prepare wait-for-completion handler data structures.
> + * Needed to synchronize this and
> + * st_registration_completion_cb() functions.
> + */
> + init_completion(&hst->wait_reg_completion);
> +
> + /* Reset ST registration callback status flag , this value
> + * will be updated in ti_st_registration_completion_cb()
> + * function whenever it called from ST driver.
> + */
> + hst->reg_status = -EINPROGRESS;
> +
> + /* ST is busy with either protocol registration or firmware
> + * download. Wait until the registration callback is called
> + */
> + BT_DBG(" waiting for registration completion signal from ST");
> +
> + timeleft = wait_for_completion_timeout
> + (&hst->wait_reg_completion,
> + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + if (!timeleft) {
> + BT_ERR("Timeout(%d sec),didn't get reg "
> + "completion signal from ST",
> + BT_REGISTER_TIMEOUT / 1000);
> + return -ETIMEDOUT;
> + }
> +
> + /* Is ST registration callback called with ERROR status? */
> + if (hst->reg_status != 0) {
> + BT_ERR("ST registration completed with invalid "
> + "status %d", hst->reg_status);
> + return -EAGAIN;
> + }
> + err = 0;
> + } else if (err == -EPERM) {
> + BT_ERR("st_register failed %d", err);
> + return err;
> + }
> +
> + hst->st_write = ti_st_proto.write;
> + if (!hst->st_write) {
> + BT_ERR("undefined ST write function");
> +
> + /* Undo registration with ST */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> + return err;
> + }
> +
> + /* Registration with ST layer is successful,
> + * hardware is ready to accept commands from HCI core.
> + */
> + set_bit(HCI_RUNNING, &hdev->flags);
> +
> + return err;
> +}
I really don't like what you are doing here. So please use
test_and_set_bit and clear it in an error case.
Also you need to handle all error cases. Just not only two.
Where is the ti_st_proto.write coming from?
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + int err;
> + struct ti_st *hst = hdev->driver_data;
> +
> + /* continue to unregister from transport */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> +
> + return err;
> +}
You need a test_and_clear_bit for HCI_RUNNING. There is a huge imbalance
here. Have you tested this with consecutive hciconfig hci0 up/down
executions actually?
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + struct hci_dev *hdev;
> + struct ti_st *hst;
> + long len;
> +
> + if (!skb)
> + return -ENOMEM;
Pointless check. The core will not call this function with a NULL
pointer SKB.
> +
> + hdev = (struct hci_dev *)skb->dev;
> + if (!hdev)
> + return -ENODEV;
Even this can't really happen. Have you seen such a case?
> + if (!test_bit(HCI_RUNNING, &hdev->flags))
> + return -EBUSY;
> +
> + hst = hdev->driver_data;
> +
> + /* Prepend skb with frame type */
> + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> + skb->len);
> +
> + /* Insert skb to shared transport layer's transmit queue.
> + * Freeing skb memory is taken care in shared transport layer,
> + * so don't free skb memory here.
> + */
> + if (!hst->st_write) {
> + kfree_skb(skb);
> + BT_ERR(" Could not write to ST (st_write is NULL)");
> + return -EAGAIN;
> + }
I don't like these crappy checks on every packet. That is just stupid.
You have checked for st_write when open happens and you set the hdev to
HCI_RUNNING. Are you saying this could change during the lifetime of the
hdev? If so then you have a serious problem here.
> + len = hst->st_write(skb);
> + if (len < 0) {
> + kfree_skb(skb);
> + BT_ERR(" ST write failed (%ld)", len);
> + return -EAGAIN;
> + }
> +
> + /* ST accepted our skb. So, Go ahead and do rest */
> + hdev->stat.byte_tx += len;
> + ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + return 0;
> +}
What is the reason for this deferred stats update. That code looks
pretty much hackish to me.
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + if (!hdev)
> + return;
> +
> + BT_DBG("%s", hdev->name);
> +
> + /* free ti_st memory */
> + kfree(hdev->driver_data);
> +
> + return;
> +}
What are you checking here for? Why do you think that hdev would not be
valid? This is what the btusb and btsdio drivers do:
static void btusb_destruct(struct hci_dev *hdev)
{
struct btusb_data *data = hdev->driver_data;
BT_DBG("%s", hdev->name);
kfree(data);
}
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + int err;
> + struct hci_dev *hdev;
I prefer if err is last in the variable list.
> +
> + /* Initialize and register HCI device */
> + hdev = hci_alloc_dev();
> + if (!hdev)
> + return -ENOMEM;
> +
> + BT_DBG("hdev %p", hdev);
> +
> + hst->hdev = hdev;
> + hdev->bus = HCI_UART;
> + hdev->driver_data = hst;
> + hdev->open = ti_st_open;
> + hdev->close = ti_st_close;
> + hdev->flush = NULL;
Please implement a flush callback.
> + hdev->send = ti_st_send_frame;
> + hdev->destruct = ti_st_destruct;
> + hdev->owner = THIS_MODULE;
> +
> + if (reset)
> + set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
Why do you need this? This should only be crappy devices. Something like
Bluetooth 1.0b old devices.
> + err = hci_register_dev(hdev);
> + if (err < 0) {
> + BT_ERR("Can't register HCI device error %d", err);
> + hci_free_dev(hdev);
> + return err;
> + }
> +
> + BT_DBG(" HCI device registered (hdev %p)", hdev);
> + return 0;
> +}
> +
> +
No double empty lines please.
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + int err;
> + static struct ti_st *hst;
See above.
> +
> + BT_DBG(" Bluetooth Driver Version %s", VERSION);
This should be in the module_init function. And should be a BT_INFO and
be precise what driver this actually this.
> +
> + hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + if (!hst)
> + return -ENOMEM;
> +
> + /* Expose "hciX" device to user space */
> + err = ti_st_register_dev(hst);
> + if (err) {
> + kfree(hst);
> + return err;
> + }
Is this ti_st_register device use anywhere else. Then please just
include that code in here to make this clear. All other drivers do all
the work in their probe() callback.
> +
> + dev_set_drvdata(&pdev->dev, hst);
> + return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + struct ti_st *hst;
> + struct hci_dev *hdev;
> +
> + hst = dev_get_drvdata(&pdev->dev);
Here I would prefer this:
struct ti_st *hst = dev_get_drvdata(&pdev->dev);
> +
> + if (!hst)
> + return -EFAULT;
> +
> + /* Deallocate local resource's memory */
> + hdev = hst->hdev;
That comment doesn't match what you are doing here.
> +
> + if (!hdev) {
> + BT_ERR("Invalid hdev memory");
> + kfree(hst);
> + return -EFAULT;
> + }
No need to check for hdev here. If probe fails, then remove should never
be called, right?
And just to be safe you might wanna add this:
dev_set_drvdata(&pdev->dev, NULL);
> +
> + ti_st_close(hdev);
> + hci_unregister_dev(hdev);
> + /* Free HCI device memory */
> + hci_free_dev(hdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver btwilink_driver = {
> + .probe = bt_ti_probe,
> + .remove = bt_ti_remove,
> + .driver = {
> + .name = "btwilink",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> + long ret;
> +
> + ret = platform_driver_register(&btwilink_driver);
> + if (ret != 0) {
> + BT_ERR("btwilink platform driver registration failed");
> + return ret;
> + }
> + return 0;
> +}
please just do like we do with all other drivers;
BT_INFO(...)
return platform_driver_register(&btwilink_driver);
> +
> +static void __exit bt_drv_exit(void)
> +{
> + platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(bt_drv_init);
> +module_exit(bt_drv_exit);
And this should be btwilink_init and btwilink_exit. Please don't try to
grab some generic namespace.
> +
> +/* ------ Module Info ------ */
> +
> +module_param(reset, bool, 0644);
> +MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
As mentioned above, that one seems wrong to me. You need to know what
your device supports. And by default it should allow sending HCI_Reset
at init. If not, then just that quirk. No need for module parameter
here.
> +MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2] Bluetooth: Automate remote name requests
From: Marcel Holtmann @ 2010-11-10 5:44 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth, Johan Hedberg
In-Reply-To: <1288282824-30736-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> In Bluetooth there are no automatic updates of remote device names when
> they get changed on the remote side. Instead, it is a good idea to do a
> manual name request when a new connection gets created (for whatever
> reason) since at this point it is very cheap (no costly baseband
> connection creation needed just for the sake of the name request).
>
> So far userspace has been responsible for this extra name request but
> tighter control is needed in order not to flood Bluetooth controllers
> with two many commands during connection creation. It has been shown
> that some controllers simply fail to function correctly if they get too
> many (almost) simultaneous commands during connection creation. The
> simplest way to acheive better control of these commands is to move
> their sending completely to the kernel side.
>
> This patch inserts name requests into the sequence of events that the
> kernel performs during connection creation. It does this after the
> remote features have been successfully requested and before any pending
> authentication requests are performed. The code will work sub-optimally
> with userspace versions that still do the name requesting themselves (it
> shouldn't break anything though) so it is recommended to combine this
> with a userspace software version that doesn't have automated name
> requests.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
> v2 makes sure that the name request gets always sent instead of just
> doing it for cases when we're also going to request authentication.
>
> net/bluetooth/hci_event.c | 151 ++++++++++++++++++++++++++++++++-------------
> 1 files changed, 107 insertions(+), 44 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 84093b0..6e770e8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -677,9 +677,51 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
> hci_dev_unlock(hdev);
> }
>
> +static void request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +{
> + struct hci_cp_auth_requested cp;
> + struct hci_conn *conn;
> +
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> + if (!conn)
> + return;
> +
> + if (conn->state != BT_CONFIG || !conn->out)
> + return;
> +
> + if (conn->sec_level == BT_SECURITY_SDP)
> + return;
> +
> + /* Only request authentication for SSP connections or non-SSP
> + * devices with sec_level HIGH */
> + if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) &&
> + conn->sec_level != BT_SECURITY_HIGH)
> + return;
> +
> + cp.handle = __cpu_to_le16(conn->handle);
> + hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> +}
please split this patch into two. One creating the common function for
the authentication requested and one for adding the name request.
My brain actually core dumps when following the logic and making sure
that it is still correct. I pretty sure it is correct, but the whole
patch is damn hard to review.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()
From: Marcel Holtmann @ 2010-11-10 5:39 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Ville Tervo, linux-bluetooth@vger.kernel.org
In-Reply-To: <20101105143711.GB9116@vigoh>
Hi Gustavo,
> > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > It also have to change the name of the function to
> > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > >
> > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > ---
> > > net/bluetooth/l2cap.c | 17 ++++++-----------
> > > 1 files changed, 6 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > index 6f931cc..3d48867 100644
> > > --- a/net/bluetooth/l2cap.c
> > > +++ b/net/bluetooth/l2cap.c
> > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > > }
> > >
> > > /* ---- Socket interface ---- */
> > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > {
> > > struct sock *sk;
> > > struct hlist_node *node;
> > > +
> > > + write_lock_bh(&l2cap_sk_list.lock);
> >
> > Code is only reading so read_lock_bh would be enough?
>
> Sure, I didn't looked to that, I just keept the same code that we were
> using before. I'll fix it.
we might also not just bother with read/write locks. Since they are not
always the right thing to do. In a lot of cases a pure spinlock is just
better. And in case of Bluetooth I think we would be just fine with
using a pure spinlock. You might run some tests with this.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
From: Marcel Holtmann @ 2010-11-10 5:36 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
In-Reply-To: <1288780365-32099-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
> Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
> *v4: taken Gustavo comments about timer HZ -> HZ/5
>
> Do not delete l2cap channel and socket sk when sk is owned by user.
> To delete l2cap channel standard timer is used.
>
> lock_sock and release_sock do not hold a normal spinlock directly but
> instead hold the owner field. This means bh_lock_sock can still execute
> even if the socket is "locked". More info can be found here:
> http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
>
> When sending following sequence:
> ...
> No. Time Source Destination Protocol Info
> 89 1.951202 RFCOMM Rcvd DISC DLCI=20
> 90 1.951324 RFCOMM Sent UA DLCI=20
> 91 1.959381 HCI_EVT Number of Completed Packets
> 92 1.966461 RFCOMM Rcvd DISC DLCI=0
> 93 1.966492 L2CAP Rcvd Disconnect Request
> 94 1.972595 L2CAP Sent Disconnect Response
>
> ...
>
> krfcommd kernel thread is preempted with l2cap tasklet which remove l2cap_conn
> (L2CAP connection handler structure). Then rfcomm thread tries to send RFCOMM
> UA which is reply to RFCOMM DISC and when de-referencing l2cap_conn crash
> happens.
so I assume you have tested this extensively with various RFCOMM corner
cases like incoming RFCOMM. Since a lot of profiles require proper
disconnects and we have to ensure that our reference counting is
correct.
Other then that it seems fine to me.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/4] Sim Access Profile API
From: Marcel Holtmann @ 2010-11-10 5:33 UTC (permalink / raw)
To: Waldemar Rymarkiewicz
Cc: linux-bluetooth, suraj, Johan Hedberg, joakim.xj.ceder
In-Reply-To: <1288791271-13857-2-git-send-email-waldemar.rymarkiewicz@tieto.com>
Hi Waldemar,
> New API for Sim Access Profile.
> ---
> Makefile.am | 2 +-
> doc/sap-api.txt | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 1 deletions(-)
> create mode 100644 doc/sap-api.txt
>
> diff --git a/Makefile.am b/Makefile.am
> index 873f2df..e1183de 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -352,7 +352,7 @@ EXTRA_DIST += doc/manager-api.txt \
> doc/service-api.txt doc/agent-api.txt doc/attribute-api.txt \
> doc/serial-api.txt doc/network-api.txt \
> doc/input-api.txt doc/audio-api.txt doc/control-api.txt \
> - doc/hfp-api.txt doc/assigned-numbers.txt
> + doc/hfp-api.txt doc/assigned-numbers.txt doc/sap-api.txt
>
> AM_YFLAGS = -d
>
> diff --git a/doc/sap-api.txt b/doc/sap-api.txt
> new file mode 100644
> index 0000000..7951f56
> --- /dev/null
> +++ b/doc/sap-api.txt
> @@ -0,0 +1,57 @@
> +BlueZ D-Bus Sim Access Profile API description
> +***********************************
> +
> +Copyright (C) 2010 ST-Ericsson SA
> +
> +
> +Sim Access Profile hierarchy
> +============================
> +
> +Service org.bluez
> +Interface org.bluez.SimAccess
> +Object path [variable prefix]/{hci0,hci1,...}
> +
> +Methods void Enable()
> +
> + Start up SAP server and register SDP record for it.
> +
> + Possible errors: org.bluez.Error.Failed
> +
> + void Disable()
> +
> + Shudown SAP server and remove the SDP record.
> +
> + Possible errors: org.bluez.Error.Failed
I don't like this. If you have properties then just changing the
property should be enough. So a SetProperty is more appropriate.
> +
> + void Disconnect(boolean type)
> +
> + Disconnect SAP client from the server. The 'type'
> + parameter indicates disconnection type.
> +
> + True - gracefull disconnection
> + False - immediate disconnection
> +
> + Possible errors: org.bluez.Error.Failed
I don't like this style of method names at all. Using method names like
GracefulDisconnect and ImmediateDisconnect would be better.
However I am not sure we should differentiate here at all. We should
always to the graceful disconnect. What will the immediate disconnect
bring us?
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Move gattrib source files to src directory
From: Marcel Holtmann @ 2010-11-10 5:26 UTC (permalink / raw)
To: Johan Hedberg; +Cc: Claudio Takahasi, linux-bluetooth
In-Reply-To: <20101105050602.GD25270@jh-x301>
Hi Johan,
> > gattrib related functions will be required during the device creation
> > for GATT enabled devices(BR/EDR and LE). Primary service discovery is
> > a pre-condition to probe the GATT device driver.
> > ---
> > Makefile.am | 7 +-
> > attrib/att.c | 764 ------------------------------------------------------
> > attrib/att.h | 206 ---------------
> > attrib/gatt.c | 113 --------
> > attrib/gatt.h | 43 ---
> > attrib/gattrib.c | 535 --------------------------------------
> > attrib/gattrib.h | 72 -----
> > src/att.c | 764 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > src/att.h | 206 +++++++++++++++
> > src/gatt.c | 113 ++++++++
> > src/gatt.h | 43 +++
> > src/gattrib.c | 535 ++++++++++++++++++++++++++++++++++++++
> > src/gattrib.h | 72 +++++
> > 13 files changed, 1736 insertions(+), 1737 deletions(-)
> > delete mode 100644 attrib/att.c
> > delete mode 100644 attrib/att.h
> > delete mode 100644 attrib/gatt.c
> > delete mode 100644 attrib/gatt.h
> > delete mode 100644 attrib/gattrib.c
> > delete mode 100644 attrib/gattrib.h
> > create mode 100644 src/att.c
> > create mode 100644 src/att.h
> > create mode 100644 src/gatt.c
> > create mode 100644 src/gatt.h
> > create mode 100644 src/gattrib.c
> > create mode 100644 src/gattrib.h
>
> I'll wait a little bit with this one. I agree that the gattrib
> funcionality needs to be available within the core daemon, but does that
> necessarily mean that the source files for need to be in src? It'd be
> good to get some comment from Marcel about this too.
I think that this change is actually bad. Since we have non-recursive
build system, we are not bound to have code under the same directory.
So my advise would be to not do this and just link the attrib/* code
into the bluetoothd.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 0/9] Fixing DBus error system in BlueZ
From: Marcel Holtmann @ 2010-11-10 5:23 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101108173133.GA18818@vigoh>
Hi Gustavo,
> > On Mon, Nov 08, 2010, Gustavo F. Padovan wrote:
> > > Here are some patches that try to fix the mess of reporting error to
> > > DBus inside BlueZ. It follows the oFono and ConnMan error system.
> > >
> > > The goal is to get ride of any directly call to g_dbus_create_error()
> > > inside bluez code, changing that to __btd_error_*. This patch set
> > > doesn't fix all of them yet, but is a very good start. Please review.
> > >
> > >
> > > Gustavo F. Padovan (9):
> > > Create __btd_error_invalid_args()
> > > Add __btd_error_already_exists()
> > > Add __btd_error_not_supported()
> > > Add __btd_error_not_connected()
> > > Add __btd_error_in_progress()
> > > Add __btd_error_not_available()
> > > Add __btd_error_busy()
> > > Add __btd_error_does_not_exist()
> > > Add __btd_error_not_authorized()
> >
> > The patches seem fine to me, but before pushing upstream I'd like to
> > understand the reason for prefixing these with with __btd instead of
> > btd. What's the criteria used to decide what to use and when and why is
> > __btd the correct choice for these new functions? My first guess would
> > have been that __btd is for things only accessible by the core-daemon
> > whereas btd is for functions exported to plugins, but that doesn't seem
> > to be the case with your patches since many of these __btd functions get
> > called from plugins.
>
> I just followed oFono and ConnMan on this. That is the reason and I
> didn't asked myself why have a __ in this case.. But I see your point.
> Do you think that change that to btd_error_* will fit better inside
> BlueZ? I can change that then.
so within ConnMan and oFono we make a difference between public symbols
that are reachable from within plugins and other which are not.
In general btd_ should be public symbols available to plugins and __btd_
for internal symbols that are no available to plugins.
For builtin plugins that makes no difference of course, but this is not
about internal builtin plugins. It is for protecting against external
plugins to not allow access to internal details.
That said, bluetoothd is not linked with the case to hide certain
symbols anyway so that right now there is no real difference here.
Regards
Marcel
^ permalink raw reply
* Re: I think we should add GoepL2capPsm attribute in sdp.h
From: Marcel Holtmann @ 2010-11-10 5:19 UTC (permalink / raw)
To: hui li; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimThap03_iYvxYyw39HOkMRAzzkscKjGMwmY7dX@mail.gmail.com>
Hi Hui,
> Currently there is no GoepL2capPsm attribute value in sdp.h, I think
> we should add this to supprot GOEP v2.0.
> Maybe we can add
> #define SDP_ATTR_GOEP_L2CAP_PSM 0x0200
> in sdp.h.
please send a proper patch for this and provide a clear reference to it
since this is not part of the assigned numbers document.
Regards
Marcel
^ permalink raw reply
* [PATCH] [RFC] Fix HDP-related segfault upon device recreation
From: Elvis Pfützenreuter @ 2010-11-10 3:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: epx
When device is removed and paired again, hdp_device is destroyed
but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
This patch ensures that such MCL is destroyed.
There is probably a better solution to this e.g. changing cache
strategy for accepted connections. A loop can be removed if 1:1
relationship between hdp_device and MCL is guaranteed.
---
health/hdp.c | 1 +
health/mcap.c | 23 ++++++++++++++++++++++-
health/mcap_lib.h | 2 ++
3 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index b141fe7..44ebe75 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
}
device_unref_mcl(device);
+ mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
g_free(device);
}
diff --git a/health/mcap.c b/health/mcap.c
index cb7b74c..34ccdaa 100644
--- a/health/mcap.c
+++ b/health/mcap.c
@@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
return TRUE;
}
+static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
+{
+ const struct mcap_mcl *mcl = a;
+ gconstpointer user_data = b;
+
+ if (mcl->cb)
+ if (mcl->cb->user_data == user_data)
+ return 0;
+
+ return 1;
+}
+
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
+{
+ GSList *l;
+
+ while ((l = g_slist_find_custom(mi->cached, user_data,
+ cmp_mcl_user_data)))
+ mcap_close_mcl(l->data, FALSE);
+}
+
void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
{
bacpy(addr, &mcl->addr);
@@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
BT_IO_OPT_INVALID);
-}
\ No newline at end of file
+}
diff --git a/health/mcap_lib.h b/health/mcap_lib.h
index 7740623..cc10c17 100644
--- a/health/mcap_lib.h
+++ b/health/mcap_lib.h
@@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
void mcap_mcl_unref(struct mcap_mcl *mcl);
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
+
/* CSP operations */
void mcap_enable_csp(struct mcap_instance *ms);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Check HealthApplication path before trying to destroy it
From: Elvis Pfützenreuter @ 2010-11-10 2:11 UTC (permalink / raw)
To: linux-bluetooth; +Cc: epx
---
health/hdp.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index 1eba8e1..b141fe7 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -376,6 +376,12 @@ static DBusMessage *manager_destroy_application(DBusConnection *conn,
l = g_slist_find_custom(applications, path, cmp_app);
+ if (!l)
+ return g_dbus_create_error(msg,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call, "
+ "no such application");
+
app = l->data;
applications = g_slist_remove(applications, app);
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 1/9] Create __btd_error_invalid_args()
From: Gustavo F. Padovan @ 2010-11-10 0:51 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimnfiUYD0b=bMLN=A218=zwUAWkZ7aWM9GBxoKr@mail.gmail.com>
Hi Jose,
* Jose Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-11-09 09:20:33 +0100]:
> Hi Gustavo,
>
> 2010/11/8 Gustavo F. Padovan <padovan@profusion.mobi>:
> > DBus error handling in BlueZ is a mess. This is the first patch to unify
> > all DBus error handling like in ConnMan and oFono. This unifies all
> > .InvalidArguments errors.
> > ---
> > attrib/client.c | 20 +++++----------
> > audio/gateway.c | 8 +----
> > audio/headset.c | 18 ++++---------
> > audio/media.c | 9 ++----
> > audio/telephony-dummy.c | 25 ++++++++------------
> > audio/telephony-maemo5.c | 11 ++------
> > audio/telephony-maemo6.c | 11 ++------
> > audio/transport.c | 14 +++--------
> > health/hdp.c | 58 ++++++++++++----------------------------------
> > network/server.c | 7 -----
> > plugins/service.c | 8 +-----
> > serial/port.c | 8 ------
> > serial/proxy.c | 19 ++++----------
> > src/adapter.c | 52 ++++++++++++++++++-----------------------
> > src/device.c | 22 ++++++-----------
> > src/error.c | 7 +++++
> > src/error.h | 2 +
> > src/manager.c | 7 -----
> > 18 files changed, 100 insertions(+), 206 deletions(-)
> >
> > diff --git a/attrib/client.c b/attrib/client.c
> > index 1f2c217..cd6e401 100644
> > --- a/attrib/client.c
> > +++ b/attrib/client.c
> > @@ -190,12 +190,6 @@ static int watcher_cmp(gconstpointer a, gconstpointer b)
> > return g_strcmp0(watcher->path, match->path);
> > }
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > static inline DBusMessage *not_authorized(DBusMessage *msg)
> > {
> > return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAuthorized",
> > @@ -465,7 +459,7 @@ static DBusMessage *register_watcher(DBusConnection *conn,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (l2cap_connect(prim->gatt, &gerr, TRUE) < 0) {
> > DBusMessage *reply;
> > @@ -499,7 +493,7 @@ static DBusMessage *unregister_watcher(DBusConnection *conn,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > match = g_new0(struct watcher, 1);
> > match->name = g_strdup(sender);
> > @@ -537,7 +531,7 @@ static DBusMessage *set_value(DBusConnection *conn, DBusMessage *msg,
> >
> > if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY ||
> > dbus_message_iter_get_element_type(iter) != DBUS_TYPE_BYTE)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_recurse(iter, &sub);
> >
> > @@ -586,23 +580,23 @@ static DBusMessage *set_property(DBusConnection *conn,
> > const char *property;
> >
> > if (!dbus_message_iter_init(msg, &iter))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_get_basic(&iter, &property);
> > dbus_message_iter_next(&iter);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_recurse(&iter, &sub);
> >
> > if (g_str_equal("Value", property))
> > return set_value(conn, msg, &sub, chr);
> >
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > }
> >
> > static GDBusMethodTable char_methods[] = {
> > diff --git a/audio/gateway.c b/audio/gateway.c
> > index 07ebdd4..ab7d310 100644
> > --- a/audio/gateway.c
> > +++ b/audio/gateway.c
> > @@ -494,9 +494,7 @@ static DBusMessage *register_agent(DBusConnection *conn,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> > DBUS_TYPE_INVALID))
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid argument");
> > + return __btd_error_invalid_args(msg);
> >
> > name = dbus_message_get_sender(msg);
> > agent = g_new0(struct hf_agent, 1);
> > @@ -529,9 +527,7 @@ static DBusMessage *unregister_agent(DBusConnection *conn,
> > if (!dbus_message_get_args(msg, NULL,
> > DBUS_TYPE_OBJECT_PATH, &path,
> > DBUS_TYPE_INVALID))
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid argument");
> > + return __btd_error_invalid_args(msg);
> >
> > if (strcmp(gw->agent->path, path) != 0)
> > return g_dbus_create_error(msg,
> > diff --git a/audio/headset.c b/audio/headset.c
> > index 0763585..2cca5ca 100644
> > --- a/audio/headset.c
> > +++ b/audio/headset.c
> > @@ -180,12 +180,6 @@ struct event {
> >
> > static GSList *headset_callbacks = NULL;
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > static DBusHandlerResult error_not_supported(DBusConnection *conn,
> > DBusMessage *msg)
> > {
> > @@ -2026,35 +2020,35 @@ static DBusMessage *hs_set_property(DBusConnection *conn,
> > uint16_t gain;
> >
> > if (!dbus_message_iter_init(msg, &iter))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_get_basic(&iter, &property);
> > dbus_message_iter_next(&iter);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > dbus_message_iter_recurse(&iter, &sub);
> >
> > if (g_str_equal("SpeakerGain", property)) {
> > if (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_UINT16)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_get_basic(&sub, &gain);
> > return hs_set_gain(conn, msg, data, gain,
> > HEADSET_GAIN_SPEAKER);
> > } else if (g_str_equal("MicrophoneGain", property)) {
> > if (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_UINT16)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_get_basic(&sub, &gain);
> > return hs_set_gain(conn, msg, data, gain,
> > HEADSET_GAIN_MICROPHONE);
> > }
> >
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > }
> > static GDBusMethodTable headset_methods[] = {
> > { "Connect", "", "", hs_connect,
> > diff --git a/audio/media.c b/audio/media.c
> > index b6c90f9..862cee6 100644
> > --- a/audio/media.c
> > +++ b/audio/media.c
> > @@ -323,18 +323,15 @@ static DBusMessage *register_endpoint(DBusConnection *conn, DBusMessage *msg,
> >
> > dbus_message_iter_recurse(&args, &props);
> > if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
> > - return g_dbus_create_error(msg, ERROR_INTERFACE
> > - ".Failed", "Invalid argument");
> > + return __btd_error_invalid_args(msg);
> >
> > if (parse_properties(&props, &uuid, &delay_reporting, &codec,
> > &capabilities, &size) || uuid == NULL)
> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
> > - "Invalid argument");
> > + return __btd_error_invalid_args(msg);
> >
> > if (media_endpoint_create(adapter, sender, path, uuid, delay_reporting,
> > codec, capabilities, size) == FALSE)
> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
> > - "Invalid argument");
> > + return __btd_error_invalid_args(msg);
> >
> > return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
> > }
> > diff --git a/audio/telephony-dummy.c b/audio/telephony-dummy.c
> > index 06cb798..b56b6e7 100644
> > --- a/audio/telephony-dummy.c
> > +++ b/audio/telephony-dummy.c
> > @@ -35,6 +35,7 @@
> >
> > #include "log.h"
> > #include "telephony.h"
> > +#include "error.h"
> >
> > #define TELEPHONY_DUMMY_IFACE "org.bluez.TelephonyTest"
> > #define TELEPHONY_DUMMY_PATH "/org/bluez/test"
> > @@ -69,12 +70,6 @@ static struct indicator dummy_indicators[] =
> > { NULL }
> > };
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg, "org.bluez.Error.InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > void telephony_device_connected(void *telephony_device)
> > {
> > DBG("telephony-dummy: device %p connected", telephony_device);
> > @@ -236,7 +231,7 @@ static DBusMessage *outgoing_call(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > DBG("telephony-dummy: outgoing call to %s", number);
> >
> > @@ -261,7 +256,7 @@ static DBusMessage *incoming_call(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > DBG("telephony-dummy: incoming call to %s", number);
> >
> > @@ -307,10 +302,10 @@ static DBusMessage *signal_strength(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_UINT32, &strength,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (strength > 5)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > telephony_update_indicator(dummy_indicators, "signal", strength);
> >
> > @@ -326,10 +321,10 @@ static DBusMessage *battery_level(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_UINT32, &level,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (level > 5)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > telephony_update_indicator(dummy_indicators, "battchg", level);
> >
> > @@ -346,7 +341,7 @@ static DBusMessage *roaming_status(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_BOOLEAN, &roaming,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > val = roaming ? EV_ROAM_ACTIVE : EV_ROAM_INACTIVE;
> >
> > @@ -365,7 +360,7 @@ static DBusMessage *registration_status(DBusConnection *conn, DBusMessage *msg,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_BOOLEAN, ®istration,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > val = registration ? EV_SERVICE_PRESENT : EV_SERVICE_NONE;
> >
> > @@ -384,7 +379,7 @@ static DBusMessage *set_subscriber_number(DBusConnection *conn,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> > DBUS_TYPE_INVALID))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > g_free(subscriber_number);
> > subscriber_number = g_strdup(number);
> > diff --git a/audio/telephony-maemo5.c b/audio/telephony-maemo5.c
> > index 4d0134c..0f9819c 100644
> > --- a/audio/telephony-maemo5.c
> > +++ b/audio/telephony-maemo5.c
> > @@ -38,6 +38,7 @@
> >
> > #include "log.h"
> > #include "telephony.h"
> > +#include "error.h"
> >
> > /* SSC D-Bus definitions */
> > #define SSC_DBUS_NAME "com.nokia.phone.SSC"
> > @@ -1880,12 +1881,6 @@ static void csd_init(void)
> > }
> > }
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg,"org.bluez.Error.InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > static uint32_t get_callflag(const char *callerid_setting)
> > {
> > if (callerid_setting != NULL) {
> > @@ -1950,7 +1945,7 @@ static DBusMessage *set_callerid(DBusConnection *conn, DBusMessage *msg,
> > if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING,
> > &callerid_setting,
> > DBUS_TYPE_INVALID) == FALSE)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (g_str_equal(callerid_setting, "allowed") ||
> > g_str_equal(callerid_setting, "restricted") ||
> > @@ -1964,7 +1959,7 @@ static DBusMessage *set_callerid(DBusConnection *conn, DBusMessage *msg,
> >
> > error("telephony-maemo: invalid argument %s for method call"
> > " SetCallerId", callerid_setting);
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > }
> >
> > static GDBusMethodTable telephony_maemo_methods[] = {
> > diff --git a/audio/telephony-maemo6.c b/audio/telephony-maemo6.c
> > index 72c8e36..4663b4d 100644
> > --- a/audio/telephony-maemo6.c
> > +++ b/audio/telephony-maemo6.c
> > @@ -38,6 +38,7 @@
> >
> > #include "log.h"
> > #include "telephony.h"
> > +#include "error.h"
> >
> > /* SSC D-Bus definitions */
> > #define SSC_DBUS_NAME "com.nokia.phone.SSC"
> > @@ -1760,12 +1761,6 @@ static void csd_init(void)
> > }
> > }
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg,"org.bluez.Error.InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > static uint32_t get_callflag(const char *callerid_setting)
> > {
> > if (callerid_setting != NULL) {
> > @@ -1830,7 +1825,7 @@ static DBusMessage *set_callerid(DBusConnection *conn, DBusMessage *msg,
> > if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING,
> > &callerid_setting,
> > DBUS_TYPE_INVALID) == FALSE)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (g_str_equal(callerid_setting, "allowed") ||
> > g_str_equal(callerid_setting, "restricted") ||
> > @@ -1844,7 +1839,7 @@ static DBusMessage *set_callerid(DBusConnection *conn, DBusMessage *msg,
> >
> > error("telephony-maemo6: invalid argument %s for method call"
> > " SetCallerId", callerid_setting);
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > }
> >
> > static DBusMessage *clear_lastnumber(DBusConnection *conn, DBusMessage *msg,
> > diff --git a/audio/transport.c b/audio/transport.c
> > index eda46e1..0c865f7 100644
> > --- a/audio/transport.c
> > +++ b/audio/transport.c
> > @@ -93,12 +93,6 @@ struct media_transport {
> > DBusMessageIter *value);
> > };
> >
> > -static inline DBusMessage *invalid_args(DBusMessage *msg)
> > -{
> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > -}
> > -
> > static inline DBusMessage *error_failed(DBusMessage *msg, const char *desc)
> > {
> > return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed", "%s", desc);
> > @@ -549,16 +543,16 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg,
> > int err;
> >
> > if (!dbus_message_iter_init(msg, &iter))
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> >
> > dbus_message_iter_get_basic(&iter, &property);
> > dbus_message_iter_next(&iter);
> >
> > if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > dbus_message_iter_recurse(&iter, &value);
> >
> > sender = dbus_message_get_sender(msg);
> > @@ -577,7 +571,7 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg,
> >
> > if (err < 0) {
> > if (err == -EINVAL)
> > - return invalid_args(msg);
> > + return __btd_error_invalid_args(msg);
> > return error_failed(msg, strerror(-err));
> > }
> >
> > diff --git a/health/hdp.c b/health/hdp.c
> > index 1eba8e1..6c1fd98 100644
> > --- a/health/hdp.c
> > +++ b/health/hdp.c
> > @@ -321,15 +321,8 @@ static DBusMessage *manager_create_application(DBusConnection *conn,
> >
> > dbus_message_iter_init(msg, &iter);
> > app = hdp_get_app_config(&iter, &err);
> > - if (err) {
> > - DBusMessage *reply;
> > -
> > - reply = g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments: %s", err->message);
> > - g_error_free(err);
> > - return reply;
> > - }
> > + if (err)
> > + return __btd_error_invalid_args(msg);
>
>
> You are leaking memory here, err should be freed before return. Also
> the message that we add to the error is for clarify the user the kind
> of error, it will be great to keep it.
Sure.
>
> >
> > name = dbus_message_get_sender(msg);
> > if (!name) {
> > @@ -368,11 +361,8 @@ static DBusMessage *manager_destroy_application(DBusConnection *conn,
> > GSList *l;
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> > - DBUS_TYPE_INVALID)){
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > - }
> > + DBUS_TYPE_INVALID))
> > + return __btd_error_invalid_args(msg);
> >
> > l = g_slist_find_custom(applications, path, cmp_app);
> >
> > @@ -1801,18 +1791,13 @@ static DBusMessage *device_create_channel(DBusConnection *conn,
> >
> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &app_path,
> > DBUS_TYPE_STRING, &conf,
> > - DBUS_TYPE_INVALID)) {
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > - }
> > + DBUS_TYPE_INVALID))
> > + return __btd_error_invalid_args(msg);
> >
> > l = g_slist_find_custom(applications, app_path, cmp_app);
> > if (!l)
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call, "
> > - "no such application");
> > + return __btd_error_invalid_args(msg);
> > +
> > app = l->data;
> >
> > if (g_ascii_strcasecmp("Reliable", conf) == 0)
> > @@ -1822,25 +1807,16 @@ static DBusMessage *device_create_channel(DBusConnection *conn,
> > else if (g_ascii_strcasecmp("Any", conf) == 0)
> > config = HDP_NO_PREFERENCE_DC;
> > else
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Invalid arguments in method call");
> > + return __btd_error_invalid_args(msg);
> >
> > if (app->role == HDP_SINK && config != HDP_NO_PREFERENCE_DC)
> > - return g_dbus_create_error(msg,
> > - ERROR_INTERFACE ".InvalidArguments",
> > - "Configuration not valid for sinks");
> > + return __btd_error_invalid_args(msg);
>
> Also here the message we tried to clarify the kind of "Invalid
> Arguments" error with a different message.
I'm not really caring about those messages, I'm fine on removing all of
them. The idea is to just have a __btd_error_failed_str() to report
string messages in some more generic errors.
^ permalink raw reply
* Re: [PATCH 2/9] Add __btd_error_already_exists()
From: Jose Antonio Santos Cadenas @ 2010-11-09 20:35 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101109203058.GB29174@vigoh>
Hi,
2010/11/9 Gustavo F. Padovan <padovan@profusion.mobi>:
> HI Jose,
>
> * Jose Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-11-09 21:13:57 +0100]:
>
>> Hi Gustavo,
>>
>> 2010/11/9 Gustavo F. Padovan <padovan@profusion.mobi>:
>> > Hi Jose,
>> >
>> > * Jose Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-11-09 09:22:56 +0100]:
>> >
>> >> Hi,
>> >>
>> >> 2010/11/8 Gustavo F. Padovan <padovan@profusion.mobi>:
>> >> > ---
>> >> > audio/gateway.c | 4 +---
>> >> > audio/media.c | 3 +--
>> >> > network/server.c | 2 +-
>> >> > serial/proxy.c | 3 +--
>> >> > src/adapter.c | 8 ++------
>> >> > src/device.c | 4 +---
>> >> > src/error.c | 7 +++++++
>> >> > src/error.h | 1 +
>> >> > 8 files changed, 15 insertions(+), 17 deletions(-)
>> >> >
>> >> > diff --git a/audio/gateway.c b/audio/gateway.c
>> >> > index ab7d310..ae0ee75 100644
>> >> > --- a/audio/gateway.c
>> >> > +++ b/audio/gateway.c
>> >> > @@ -488,9 +488,7 @@ static DBusMessage *register_agent(DBusConnection *conn,
>> >> > const char *path, *name;
>> >> >
>> >> > if (gw->agent)
>> >> > - return g_dbus_create_error(msg,
>> >> > - ERROR_INTERFACE ".AlreadyExists",
>> >> > - "Agent already exists");
>> >> > + return __btd_error_already_exists(msg);
>> >> >
>> >> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
>> >> > DBUS_TYPE_INVALID))
>> >> > diff --git a/audio/media.c b/audio/media.c
>> >> > index 862cee6..8821ee1 100644
>> >> > --- a/audio/media.c
>> >> > +++ b/audio/media.c
>> >> > @@ -318,8 +318,7 @@ static DBusMessage *register_endpoint(DBusConnection *conn, DBusMessage *msg,
>> >> > dbus_message_iter_next(&args);
>> >> >
>> >> > if (media_adapter_find_endpoint(adapter, sender, path, NULL) != NULL)
>> >> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
>> >> > - "Endpoint already registered");
>> >> > + return __btd_error_already_exists(msg);
>> >> >
>> >> > dbus_message_iter_recurse(&args, &props);
>> >> > if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
>> >> > diff --git a/network/server.c b/network/server.c
>> >> > index ce1fe5e..41c9ec3 100644
>> >> > --- a/network/server.c
>> >> > +++ b/network/server.c
>> >> > @@ -605,7 +605,7 @@ static DBusMessage *register_server(DBusConnection *conn,
>> >> > return failed(msg, "Invalid UUID");
>> >> >
>> >> > if (ns->record_id)
>> >> > - return failed(msg, "Already registered");
>> >> > + return __btd_error_already_exists(msg);
>> >> >
>> >> > reply = dbus_message_new_method_return(msg);
>> >> > if (!reply)
>> >> > diff --git a/serial/proxy.c b/serial/proxy.c
>> >> > index 8e182b6..de82f9a 100644
>> >> > --- a/serial/proxy.c
>> >> > +++ b/serial/proxy.c
>> >> > @@ -1058,8 +1058,7 @@ static DBusMessage *create_proxy(DBusConnection *conn,
>> >> > if (err == -EINVAL)
>> >> > return __btd_error_invalid_args(msg);
>> >> > else if (err == -EALREADY)
>> >> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".AlreadyExist",
>> >> > - "Proxy already exists");
>> >> > + return __btd_error_already_exists(msg);
>> >> > else if (err < 0)
>> >> > return g_dbus_create_error(msg, ERROR_INTERFACE "Failed",
>> >> > "Proxy creation failed (%s)", strerror(-err));
>> >> > diff --git a/src/adapter.c b/src/adapter.c
>> >> > index cc51816..ffbc943 100644
>> >> > --- a/src/adapter.c
>> >> > +++ b/src/adapter.c
>> >> > @@ -1742,9 +1742,7 @@ static DBusMessage *create_device(DBusConnection *conn,
>> >> > return __btd_error_invalid_args(msg);
>> >> >
>> >> > if (adapter_find_device(adapter, address))
>> >> > - return g_dbus_create_error(msg,
>> >> > - ERROR_INTERFACE ".AlreadyExists",
>> >> > - "Device already exists");
>> >> > + return __btd_error_already_exists(msg);
>> >> >
>> >> > DBG("%s", address);
>> >> >
>> >> > @@ -1906,9 +1904,7 @@ static DBusMessage *register_agent(DBusConnection *conn, DBusMessage *msg,
>> >> > return NULL;
>> >> >
>> >> > if (adapter->agent)
>> >> > - return g_dbus_create_error(msg,
>> >> > - ERROR_INTERFACE ".AlreadyExists",
>> >> > - "Agent already exists");
>> >> > + return __btd_error_already_exists(msg);
>> >> >
>> >> > cap = parse_io_capability(capability);
>> >> > if (cap == IO_CAPABILITY_INVALID)
>> >> > diff --git a/src/device.c b/src/device.c
>> >> > index ef1a910..6d110dc 100644
>> >> > --- a/src/device.c
>> >> > +++ b/src/device.c
>> >> > @@ -1944,9 +1944,7 @@ DBusMessage *device_create_bonding(struct btd_device *device,
>> >> > str = textfile_caseget(filename, dstaddr);
>> >> > if (str) {
>> >> > free(str);
>> >> > - return g_dbus_create_error(msg,
>> >> > - ERROR_INTERFACE ".AlreadyExists",
>> >> > - "Bonding already exists");
>> >> > + return __btd_error_already_exists(msg);
>> >> > }
>> >> >
>> >> > /* If our IO capability is NoInputNoOutput use medium security
>> >> > diff --git a/src/error.c b/src/error.c
>> >> > index a30c050..e268163 100644
>> >> > --- a/src/error.c
>> >> > +++ b/src/error.c
>> >> > @@ -55,3 +55,10 @@ DBusMessage *__btd_error_invalid_args(DBusMessage *msg)
>> >> > ".InvalidArguments",
>> >> > "Invalid arguments in method call");
>> >> > }
>> >> > +
>> >> > +DBusMessage *__btd_error_already_exists(DBusMessage *msg)
>> >>
>> >> I also think that an additional message will be great for a better description.
>> >
>> > I'm not seeing any case when we need that for already_exists(). All
>> > messages were saying the same thing. We don't need them.
>>
>>
>> The messages are different for each case: (ie: "Bonding already
>> exists", "Agent already exists", etc), they are nearly the same but
>> they are a little bit more expressive. Also if we keep the additional
>> message, we are following the same behaviour than in the other error
>> funtions, this way this API is easy to use.
>
> Yes, they are, but you always know the DBus funcion you called. If you
> call RegisterAgent() and it replies .AlreadyExists you will know that is
> a Agent that already exists, and not a Bonding, Device, etc. So no need
> to explain the error in theses cases.
You are right, in most of the cases the message won't be needed or
won't give extra information. I just see the uniformity simpler (just
my opinion).
>
>
^ permalink raw reply
* Re: [PATCH 2/9] Add __btd_error_already_exists()
From: Gustavo F. Padovan @ 2010-11-09 20:30 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTi=7Ou9+j1rEt=d6b9Fc2eHx4L=4aMvGD2OPYooY@mail.gmail.com>
HI Jose,
* Jose Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-11-09 21:13:57 +0100]:
> Hi Gustavo,
>
> 2010/11/9 Gustavo F. Padovan <padovan@profusion.mobi>:
> > Hi Jose,
> >
> > * Jose Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-11-09 09:22:56 +0100]:
> >
> >> Hi,
> >>
> >> 2010/11/8 Gustavo F. Padovan <padovan@profusion.mobi>:
> >> > ---
> >> > audio/gateway.c | 4 +---
> >> > audio/media.c | 3 +--
> >> > network/server.c | 2 +-
> >> > serial/proxy.c | 3 +--
> >> > src/adapter.c | 8 ++------
> >> > src/device.c | 4 +---
> >> > src/error.c | 7 +++++++
> >> > src/error.h | 1 +
> >> > 8 files changed, 15 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/audio/gateway.c b/audio/gateway.c
> >> > index ab7d310..ae0ee75 100644
> >> > --- a/audio/gateway.c
> >> > +++ b/audio/gateway.c
> >> > @@ -488,9 +488,7 @@ static DBusMessage *register_agent(DBusConnection *conn,
> >> > const char *path, *name;
> >> >
> >> > if (gw->agent)
> >> > - return g_dbus_create_error(msg,
> >> > - ERROR_INTERFACE ".AlreadyExists",
> >> > - "Agent already exists");
> >> > + return __btd_error_already_exists(msg);
> >> >
> >> > if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> >> > DBUS_TYPE_INVALID))
> >> > diff --git a/audio/media.c b/audio/media.c
> >> > index 862cee6..8821ee1 100644
> >> > --- a/audio/media.c
> >> > +++ b/audio/media.c
> >> > @@ -318,8 +318,7 @@ static DBusMessage *register_endpoint(DBusConnection *conn, DBusMessage *msg,
> >> > dbus_message_iter_next(&args);
> >> >
> >> > if (media_adapter_find_endpoint(adapter, sender, path, NULL) != NULL)
> >> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
> >> > - "Endpoint already registered");
> >> > + return __btd_error_already_exists(msg);
> >> >
> >> > dbus_message_iter_recurse(&args, &props);
> >> > if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
> >> > diff --git a/network/server.c b/network/server.c
> >> > index ce1fe5e..41c9ec3 100644
> >> > --- a/network/server.c
> >> > +++ b/network/server.c
> >> > @@ -605,7 +605,7 @@ static DBusMessage *register_server(DBusConnection *conn,
> >> > return failed(msg, "Invalid UUID");
> >> >
> >> > if (ns->record_id)
> >> > - return failed(msg, "Already registered");
> >> > + return __btd_error_already_exists(msg);
> >> >
> >> > reply = dbus_message_new_method_return(msg);
> >> > if (!reply)
> >> > diff --git a/serial/proxy.c b/serial/proxy.c
> >> > index 8e182b6..de82f9a 100644
> >> > --- a/serial/proxy.c
> >> > +++ b/serial/proxy.c
> >> > @@ -1058,8 +1058,7 @@ static DBusMessage *create_proxy(DBusConnection *conn,
> >> > if (err == -EINVAL)
> >> > return __btd_error_invalid_args(msg);
> >> > else if (err == -EALREADY)
> >> > - return g_dbus_create_error(msg, ERROR_INTERFACE ".AlreadyExist",
> >> > - "Proxy already exists");
> >> > + return __btd_error_already_exists(msg);
> >> > else if (err < 0)
> >> > return g_dbus_create_error(msg, ERROR_INTERFACE "Failed",
> >> > "Proxy creation failed (%s)", strerror(-err));
> >> > diff --git a/src/adapter.c b/src/adapter.c
> >> > index cc51816..ffbc943 100644
> >> > --- a/src/adapter.c
> >> > +++ b/src/adapter.c
> >> > @@ -1742,9 +1742,7 @@ static DBusMessage *create_device(DBusConnection *conn,
> >> > return __btd_error_invalid_args(msg);
> >> >
> >> > if (adapter_find_device(adapter, address))
> >> > - return g_dbus_create_error(msg,
> >> > - ERROR_INTERFACE ".AlreadyExists",
> >> > - "Device already exists");
> >> > + return __btd_error_already_exists(msg);
> >> >
> >> > DBG("%s", address);
> >> >
> >> > @@ -1906,9 +1904,7 @@ static DBusMessage *register_agent(DBusConnection *conn, DBusMessage *msg,
> >> > return NULL;
> >> >
> >> > if (adapter->agent)
> >> > - return g_dbus_create_error(msg,
> >> > - ERROR_INTERFACE ".AlreadyExists",
> >> > - "Agent already exists");
> >> > + return __btd_error_already_exists(msg);
> >> >
> >> > cap = parse_io_capability(capability);
> >> > if (cap == IO_CAPABILITY_INVALID)
> >> > diff --git a/src/device.c b/src/device.c
> >> > index ef1a910..6d110dc 100644
> >> > --- a/src/device.c
> >> > +++ b/src/device.c
> >> > @@ -1944,9 +1944,7 @@ DBusMessage *device_create_bonding(struct btd_device *device,
> >> > str = textfile_caseget(filename, dstaddr);
> >> > if (str) {
> >> > free(str);
> >> > - return g_dbus_create_error(msg,
> >> > - ERROR_INTERFACE ".AlreadyExists",
> >> > - "Bonding already exists");
> >> > + return __btd_error_already_exists(msg);
> >> > }
> >> >
> >> > /* If our IO capability is NoInputNoOutput use medium security
> >> > diff --git a/src/error.c b/src/error.c
> >> > index a30c050..e268163 100644
> >> > --- a/src/error.c
> >> > +++ b/src/error.c
> >> > @@ -55,3 +55,10 @@ DBusMessage *__btd_error_invalid_args(DBusMessage *msg)
> >> > ".InvalidArguments",
> >> > "Invalid arguments in method call");
> >> > }
> >> > +
> >> > +DBusMessage *__btd_error_already_exists(DBusMessage *msg)
> >>
> >> I also think that an additional message will be great for a better description.
> >
> > I'm not seeing any case when we need that for already_exists(). All
> > messages were saying the same thing. We don't need them.
>
>
> The messages are different for each case: (ie: "Bonding already
> exists", "Agent already exists", etc), they are nearly the same but
> they are a little bit more expressive. Also if we keep the additional
> message, we are following the same behaviour than in the other error
> funtions, this way this API is easy to use.
Yes, they are, but you always know the DBus funcion you called. If you
call RegisterAgent() and it replies .AlreadyExists you will know that is
a Agent that already exists, and not a Bonding, Device, etc. So no need
to explain the error in theses cases.
^ permalink raw reply
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