From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 8 May 2012 17:34:35 +0300 From: Andrei Emeltchenko To: David Herrmann Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org, gustavo@padovan.org Subject: Re: [PATCH 2/3] Bluetooth: Replace unsafe batostr() calls with ba2str() Message-ID: <20120508143434.GE29352@aemeltch-MOBL1> References: <1336483706-1533-1-git-send-email-dh.herrmann@googlemail.com> <1336483706-1533-2-git-send-email-dh.herrmann@googlemail.com> <20120508134859.GC29352@aemeltch-MOBL1> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 In-Reply-To: Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi David, On Tue, May 08, 2012 at 04:02:39PM +0200, David Herrmann wrote: > Hi Andrei > > On Tue, May 8, 2012 at 3:49 PM, Andrei Emeltchenko > wrote: > > Hi David, > > > > On Tue, May 08, 2012 at 03:28:25PM +0200, David Herrmann wrote: > >> batostr() is not thread-safe so we _must_ use ba2str() in all un-protected > >> paths (which is pretty everywhere in our source). > >> > >> All places where BT_DBG() is used we call ba2str() as parameter so the > >> static buffer is unused if debug is disabled during compilation and gcc > >> should be clever enough to remove the buffers. > >> > >> Reported-by: Johannes Berg > >> Signed-off-by: David Herrmann > >> --- > >>  net/bluetooth/bnep/core.c   |    4 +++- > >>  net/bluetooth/cmtp/core.c   |    3 ++- > >>  net/bluetooth/hci_conn.c    |    9 ++++++--- > >>  net/bluetooth/hci_core.c    |   32 ++++++++++++++++++++++---------- > >>  net/bluetooth/hci_event.c   |   17 +++++++++++------ > >>  net/bluetooth/hci_sysfs.c   |   14 ++++++++++---- > >>  net/bluetooth/hidp/core.c   |    4 ++-- > >>  net/bluetooth/l2cap_core.c  |   23 ++++++++++++++--------- > >>  net/bluetooth/rfcomm/core.c |   15 +++++++++------ > >>  net/bluetooth/rfcomm/sock.c |   10 ++++++---- > >>  net/bluetooth/rfcomm/tty.c  |   10 +++++++--- > >>  net/bluetooth/sco.c         |   20 ++++++++++++++------ > >>  12 files changed, 106 insertions(+), 55 deletions(-) > >> > >> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c > >> index a779ec7..03a3cb3 100644 > >> --- a/net/bluetooth/bnep/core.c > >> +++ b/net/bluetooth/bnep/core.c > >> @@ -166,6 +166,7 @@ static int bnep_ctrl_set_netfilter(struct bnep_session *s, __be16 *data, int len > >>  static int bnep_ctrl_set_mcfilter(struct bnep_session *s, u8 *data, int len) > >>  { > >>       int n; > >> +     char babuf1[BDADDR_STRLEN], babuf2[BDADDR_STRLEN]; > > > > NAK in this form. You are always allocating unneeded memory. > > That's on the stack. I think gcc should be smart enough to _not_ > allocate this on code paths not using it. Furthermore, there is only > one other way to do this and this is introducing a new format > specifier for snprintf() that takes as argument a bdaddr_t but I tried > to avoid this. > > I also don't know what the problem is with using the stack for both > buffers? That's definitely no speed problem so you must be concerned > about stack size. But again, both are not even available when > disabling debug and thats just 18 bytes per buffer... > > Please be more specific about what's bothering you here. Those extra variables does not look necessary. What about the method suggested by Johannes using %pM Best regards Andrei Emeltchenko