[PATCH 1/2] core: Remove unneeded assignment

[PATCH 1/2] core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Variable rfcomm is never used after assignment
---
 src/profile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/profile.c b/src/profile.c
index 528525a..7aca3be 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1272,7 +1272,6 @@ static uint32_t ext_start_servers(struct ext_profile *ext,
 			error("RFCOMM server failed for %s: %s",
 						ext->name, err->message);
 			g_free(rfcomm);
-			rfcomm = NULL;
 			g_clear_error(&err);
 			goto failed;
 		} else {
-- 
1.9.1

[PATCH 2/2] android/core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 android/bluetooth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index a2ab445..5b72a36 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -291,7 +291,6 @@ static void load_adapter_config(void)
 	if (gerr) {
 		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
 		g_error_free(gerr);
-		gerr = NULL;
 	}
 
 	g_key_file_free(key_file);
-- 
1.9.1

[PATCHv2 1/6] core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Variable rfcomm is never used after assignment
---
 src/profile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/profile.c b/src/profile.c
index 528525a..7aca3be 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1272,7 +1272,6 @@ static uint32_t ext_start_servers(struct ext_profile *ext,
 			error("RFCOMM server failed for %s: %s",
 						ext->name, err->message);
 			g_free(rfcomm);
-			rfcomm = NULL;
 			g_clear_error(&err);
 			goto failed;
 		} else {
-- 
1.9.1

[PATCHv2 2/6] android/core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 android/bluetooth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index a2ab445..5b72a36 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -291,7 +291,6 @@ static void load_adapter_config(void)
 	if (gerr) {
 		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
 		g_error_free(gerr);
-		gerr = NULL;
 	}
 
 	g_key_file_free(key_file);
-- 
1.9.1

[PATCHv2 3/6] tools/rctest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Code is unreachable since we use exit() before
---
 tools/rctest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index 2c7e45b..a09a973 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -437,8 +437,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv2 4/6] tools/rctest: Remove dead code

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Since do_listen() never returns remove hanging code
---
 tools/rctest.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index a09a973..e61f7e2 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -629,10 +629,8 @@ static void automated_send_recv()
 	char device[18];
 
 	if (fork()) {
-		if (!savefile) {
+		if (!savefile)
 			do_listen(recv_mode);
-			return;
-		}
 
 		save_fd = open(savefile, O_CREAT | O_WRONLY,
 						S_IRUSR | S_IWUSR);
@@ -640,8 +638,6 @@ static void automated_send_recv()
 			syslog(LOG_ERR, "Failed to open file to save data");
 
 		do_listen(save_mode);
-
-		close(save_fd);
 	} else {
 		ba2str(&bdaddr, device);
 
-- 
1.9.1

[PATCHv2 5/6] tools/scotest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/scotest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/scotest.c b/tools/scotest.c
index e5530d9..bd65034 100644
--- a/tools/scotest.c
+++ b/tools/scotest.c
@@ -237,8 +237,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv2 6/6] tools/l2test: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/l2test.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/l2test.c b/tools/l2test.c
index ffad7c4..67ca70a 100644
--- a/tools/l2test.c
+++ b/tools/l2test.c
@@ -768,8 +768,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv3 01/11] core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Variable rfcomm is never used after assignment
---
 src/profile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/profile.c b/src/profile.c
index 528525a..7aca3be 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1272,7 +1272,6 @@ static uint32_t ext_start_servers(struct ext_profile *ext,
 			error("RFCOMM server failed for %s: %s",
 						ext->name, err->message);
 			g_free(rfcomm);
-			rfcomm = NULL;
 			g_clear_error(&err);
 			goto failed;
 		} else {
-- 
1.9.1

[PATCHv3 02/11] android/core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 android/bluetooth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index a2ab445..5b72a36 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -291,7 +291,6 @@ static void load_adapter_config(void)
 	if (gerr) {
 		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
 		g_error_free(gerr);
-		gerr = NULL;
 	}
 
 	g_key_file_free(key_file);
-- 
1.9.1

[PATCHv3 03/11] tools/rctest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Code is unreachable since we use exit() before
---
 tools/rctest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index 2c7e45b..a09a973 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -437,8 +437,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv3 04/11] tools/rctest: Remove dead code

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Since do_listen() never returns remove hanging code
---
 tools/rctest.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index a09a973..e61f7e2 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -629,10 +629,8 @@ static void automated_send_recv()
 	char device[18];
 
 	if (fork()) {
-		if (!savefile) {
+		if (!savefile)
 			do_listen(recv_mode);
-			return;
-		}
 
 		save_fd = open(savefile, O_CREAT | O_WRONLY,
 						S_IRUSR | S_IWUSR);
@@ -640,8 +638,6 @@ static void automated_send_recv()
 			syslog(LOG_ERR, "Failed to open file to save data");
 
 		do_listen(save_mode);
-
-		close(save_fd);
 	} else {
 		ba2str(&bdaddr, device);
 
-- 
1.9.1

[PATCHv3 05/11] tools/scotest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/scotest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/scotest.c b/tools/scotest.c
index e5530d9..bd65034 100644
--- a/tools/scotest.c
+++ b/tools/scotest.c
@@ -237,8 +237,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv3 06/11] tools/l2test: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/l2test.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/l2test.c b/tools/l2test.c
index ffad7c4..67ca70a 100644
--- a/tools/l2test.c
+++ b/tools/l2test.c
@@ -768,8 +768,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv3 07/11] tools/scotest: Fix double close socket

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When executing in a child we close(sk) in the beginning. error label
handles parent process error conditions.
---
 tools/scotest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/scotest.c b/tools/scotest.c
index bd65034..d033ae0 100644
--- a/tools/scotest.c
+++ b/tools/scotest.c
@@ -210,7 +210,7 @@ static void do_listen(void (*handler)(int sk))
 							strerror(errno), errno);
 			if (!defer_setup) {
 				close(nsk);
-				goto error;
+				exit(1);
 			}
 		}
 
@@ -227,7 +227,7 @@ static void do_listen(void (*handler)(int sk))
 
 			if (defer_setup < 0) {
 				close(nsk);
-				goto error;
+				exit(1);
 			}
 		}
 
-- 
1.9.1

[PATCHv3 08/11] android/ipc: Remove unneeded check

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

rsp_len cannot be NULL since it is checked above and dereferenced.
Remove static analyzers warnings.
---
 android/hal-ipc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/android/hal-ipc.c b/android/hal-ipc.c
index 494ba85..363072c 100644
--- a/android/hal-ipc.c
+++ b/android/hal-ipc.c
@@ -462,8 +462,7 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len, void *param,
 		}
 	}
 
-	if (rsp_len)
-		*rsp_len = cmd.len;
+	*rsp_len = cmd.len;
 
 	return BT_STATUS_SUCCESS;
 
-- 
1.9.1

[PATCHv3 09/11] tools: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

lmpver and hciver are allocated through malloc and need to be freed.
---
 tools/parser/hci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/parser/hci.c b/tools/parser/hci.c
index 351f843..1a639af 100644
--- a/tools/parser/hci.c
+++ b/tools/parser/hci.c
@@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(rp->status));
 	} else {
+		char *lmpver = lmp_vertostr(rp->lmp_ver);
+		char *hciver = hci_vertostr(rp->hci_ver);
+
 		p_indent(level, frm);
 		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
-					hci_vertostr(rp->hci_ver),
+					hciver ? hciver : "n/a",
 					rp->hci_ver, btohs(rp->hci_rev));
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-					lmp_vertostr(rp->lmp_ver),
+					lmpver ? lmpver : "n/a",
 					rp->lmp_ver, btohs(rp->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 				bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			bt_free(lmpver);
+		if (hciver)
+			bt_free(hciver);
 	}
 }
 
@@ -3178,13 +3186,18 @@ static inline void read_remote_version_complete_dump(int level, struct frame *fr
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(evt->status));
 	} else {
+		char *lmpver = lmp_vertostr(evt->lmp_ver);
+
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-			lmp_vertostr(evt->lmp_ver), evt->lmp_ver,
+			lmpver ? lmpver : "n/a", evt->lmp_ver,
 			btohs(evt->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 			bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			bt_free(lmpver);
 	}
 }
 
-- 
1.9.1

[PATCHv3 10/11] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for folder_listing_cb().
---
 obexd/client/map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index d2d3d81..331aebc 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -271,8 +271,10 @@ static void folder_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
+	if (reply == NULL) {
+		g_free(contents);
 		return;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-- 
1.9.1

[PATCHv3 11/11] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for message_listing_cb()
---
 obexd/client/map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 331aebc..5d7f0dc 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1182,8 +1182,10 @@ static void message_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
+	if (reply == NULL) {
+		g_free(contents);
 		return;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-- 
1.9.1

Re: [PATCHv3 02/11] android/core: Remove unneeded assignment

[Johan Hedberg]

Hi Andrei,

On Mon, Jul 28, 2014, Andrei Emeltchenko wrote:
> ---
>  android/bluetooth.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index a2ab445..5b72a36 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -291,7 +291,6 @@ static void load_adapter_config(void)
>  	if (gerr) {
>  		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
>  		g_error_free(gerr);
> -		gerr = NULL;
>  	}
>  
>  	g_key_file_free(key_file);

I don't think patches like this are a good idea since they make the code
more prone to bugs in the future. In this case if the function was ever
extended and gerr reused one would need to remember to "fix" the old
code so that it's re-initialized to NULL.

Johan

Re: [PATCHv3 04/11] tools/rctest: Remove dead code

[Johan Hedberg]

Hi Andrei,

On Mon, Jul 28, 2014, Andrei Emeltchenko wrote:
> Since do_listen() never returns remove hanging code
> ---
>  tools/rctest.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tools/rctest.c b/tools/rctest.c
> index a09a973..e61f7e2 100644
> --- a/tools/rctest.c
> +++ b/tools/rctest.c
> @@ -629,10 +629,8 @@ static void automated_send_recv()
>  	char device[18];
>  
>  	if (fork()) {
> -		if (!savefile) {
> +		if (!savefile)
>  			do_listen(recv_mode);
> -			return;
> -		}
>  
>  		save_fd = open(savefile, O_CREAT | O_WRONLY,
>  						S_IRUSR | S_IWUSR);
> @@ -640,8 +638,6 @@ static void automated_send_recv()
>  			syslog(LOG_ERR, "Failed to open file to save data");
>  
>  		do_listen(save_mode);
> -
> -		close(save_fd);
>  	} else {
>  		ba2str(&bdaddr, device);

I'd either forget about this patch and leave the code as-is, or then at
least add a comment before the do_listen() call to explain that it will
never return.

Johan

Re: [PATCHv3 02/11] android/core: Remove unneeded assignment

[Andrei Emeltchenko]

Hi Johan,

On Mon, Jul 28, 2014 at 04:57:50PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Mon, Jul 28, 2014, Andrei Emeltchenko wrote:
> > ---
> >  android/bluetooth.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/android/bluetooth.c b/android/bluetooth.c
> > index a2ab445..5b72a36 100644
> > --- a/android/bluetooth.c
> > +++ b/android/bluetooth.c
> > @@ -291,7 +291,6 @@ static void load_adapter_config(void)
> >  	if (gerr) {
> >  		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
> >  		g_error_free(gerr);
> > -		gerr = NULL;
> >  	}
> >  
> >  	g_key_file_free(key_file);
> 
> I don't think patches like this are a good idea since they make the code
> more prone to bugs in the future. In this case if the function was ever
> extended and gerr reused one would need to remember to "fix" the old
> code so that it's re-initialized to NULL.

This is common practice, to initialize if needed value before use
(especially if value is reused). Makes code more clean.

This also fixes static analyzer warnings.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv3 02/11] android/core: Remove unneeded assignment

[Johan Hedberg]

Hi Andrei,

On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> On Mon, Jul 28, 2014 at 04:57:50PM +0300, Johan Hedberg wrote:
> > Hi Andrei,
> > 
> > On Mon, Jul 28, 2014, Andrei Emeltchenko wrote:
> > > ---
> > >  android/bluetooth.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/android/bluetooth.c b/android/bluetooth.c
> > > index a2ab445..5b72a36 100644
> > > --- a/android/bluetooth.c
> > > +++ b/android/bluetooth.c
> > > @@ -291,7 +291,6 @@ static void load_adapter_config(void)
> > >  	if (gerr) {
> > >  		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
> > >  		g_error_free(gerr);
> > > -		gerr = NULL;
> > >  	}
> > >  
> > >  	g_key_file_free(key_file);
> > 
> > I don't think patches like this are a good idea since they make the code
> > more prone to bugs in the future. In this case if the function was ever
> > extended and gerr reused one would need to remember to "fix" the old
> > code so that it's re-initialized to NULL.
> 
> This is common practice, to initialize if needed value before use
> (especially if value is reused). Makes code more clean.

Then use at least g_clear_error instead of g_error_free.

> This also fixes static analyzer warnings.

To me it looks more like this is not just a nice side effect (which you
imply by your "also ...") but the main reason you created this patch.

Johan

[PATCHv4 01/12] core: Remove unneeded assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Variable rfcomm is never used after assignment
---
 src/profile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/profile.c b/src/profile.c
index 528525a..7aca3be 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1272,7 +1272,6 @@ static uint32_t ext_start_servers(struct ext_profile *ext,
 			error("RFCOMM server failed for %s: %s",
 						ext->name, err->message);
 			g_free(rfcomm);
-			rfcomm = NULL;
 			g_clear_error(&err);
 			goto failed;
 		} else {
-- 
1.9.1

[PATCHv4 02/12] android/core: Use g_clear_error instead of free and assignment

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

g_clear_error() frees err and set it to NULL. This fixes dead code
warnings from analyzers.
---
 android/bluetooth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index a2ab445..52d816f 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -290,8 +290,7 @@ static void load_adapter_config(void)
 				"General", "DiscoverableTimeout", &gerr);
 	if (gerr) {
 		adapter.discoverable_timeout = DEFAULT_DISCOVERABLE_TIMEOUT;
-		g_error_free(gerr);
-		gerr = NULL;
+		g_clear_error(&gerr);
 	}
 
 	g_key_file_free(key_file);
-- 
1.9.1

[PATCHv4 03/12] tools/rctest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Code is unreachable since we use exit() before
---
 tools/rctest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index 2c7e45b..a09a973 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -437,8 +437,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv4 04/12] tools/rctest: Remove dead code

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Since do_listen() never returns remove hanging code
---
 tools/rctest.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index a09a973..583b50c 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -629,19 +629,17 @@ static void automated_send_recv()
 	char device[18];
 
 	if (fork()) {
-		if (!savefile) {
+		if (!savefile)
+			/* do_listen() never returns */
 			do_listen(recv_mode);
-			return;
-		}
 
 		save_fd = open(savefile, O_CREAT | O_WRONLY,
 						S_IRUSR | S_IWUSR);
 		if (save_fd < 0)
 			syslog(LOG_ERR, "Failed to open file to save data");
 
+		/* do_listen() never returns */
 		do_listen(save_mode);
-
-		close(save_fd);
 	} else {
 		ba2str(&bdaddr, device);
 
-- 
1.9.1

[PATCHv4 05/12] tools/scotest: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/scotest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/scotest.c b/tools/scotest.c
index e5530d9..bd65034 100644
--- a/tools/scotest.c
+++ b/tools/scotest.c
@@ -237,8 +237,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv4 06/12] tools/l2test: Remove unneeded return

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove dead code after exit()
---
 tools/l2test.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/l2test.c b/tools/l2test.c
index ffad7c4..67ca70a 100644
--- a/tools/l2test.c
+++ b/tools/l2test.c
@@ -768,8 +768,6 @@ static void do_listen(void (*handler)(int sk))
 		exit(0);
 	}
 
-	return;
-
 error:
 	close(sk);
 	exit(1);
-- 
1.9.1

[PATCHv4 07/12] tools/scotest: Fix double close socket

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When executing in a child we close(sk) in the beginning. error label
handles parent process error conditions.
---
 tools/scotest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/scotest.c b/tools/scotest.c
index bd65034..d033ae0 100644
--- a/tools/scotest.c
+++ b/tools/scotest.c
@@ -210,7 +210,7 @@ static void do_listen(void (*handler)(int sk))
 							strerror(errno), errno);
 			if (!defer_setup) {
 				close(nsk);
-				goto error;
+				exit(1);
 			}
 		}
 
@@ -227,7 +227,7 @@ static void do_listen(void (*handler)(int sk))
 
 			if (defer_setup < 0) {
 				close(nsk);
-				goto error;
+				exit(1);
 			}
 		}
 
-- 
1.9.1

[PATCHv4 08/12] android/ipc: Remove unneeded check

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

rsp_len cannot be NULL since it is checked above and dereferenced.
Remove static analyzers warnings.
---
 android/hal-ipc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/android/hal-ipc.c b/android/hal-ipc.c
index 494ba85..363072c 100644
--- a/android/hal-ipc.c
+++ b/android/hal-ipc.c
@@ -462,8 +462,7 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len, void *param,
 		}
 	}
 
-	if (rsp_len)
-		*rsp_len = cmd.len;
+	*rsp_len = cmd.len;
 
 	return BT_STATUS_SUCCESS;
 
-- 
1.9.1

[PATCHv4 09/12] tools: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

lmpver and hciver are allocated through malloc and need to be freed.
---
 tools/parser/hci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/parser/hci.c b/tools/parser/hci.c
index 351f843..1a639af 100644
--- a/tools/parser/hci.c
+++ b/tools/parser/hci.c
@@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(rp->status));
 	} else {
+		char *lmpver = lmp_vertostr(rp->lmp_ver);
+		char *hciver = hci_vertostr(rp->hci_ver);
+
 		p_indent(level, frm);
 		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
-					hci_vertostr(rp->hci_ver),
+					hciver ? hciver : "n/a",
 					rp->hci_ver, btohs(rp->hci_rev));
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-					lmp_vertostr(rp->lmp_ver),
+					lmpver ? lmpver : "n/a",
 					rp->lmp_ver, btohs(rp->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 				bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			bt_free(lmpver);
+		if (hciver)
+			bt_free(hciver);
 	}
 }
 
@@ -3178,13 +3186,18 @@ static inline void read_remote_version_complete_dump(int level, struct frame *fr
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(evt->status));
 	} else {
+		char *lmpver = lmp_vertostr(evt->lmp_ver);
+
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-			lmp_vertostr(evt->lmp_ver), evt->lmp_ver,
+			lmpver ? lmpver : "n/a", evt->lmp_ver,
 			btohs(evt->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 			bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			bt_free(lmpver);
 	}
 }
 
-- 
1.9.1

[PATCHv4 10/12] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for folder_listing_cb().
---
 obexd/client/map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index d2d3d81..331aebc 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -271,8 +271,10 @@ static void folder_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
+	if (reply == NULL) {
+		g_free(contents);
 		return;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-- 
1.9.1

[PATCHv4 11/12] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for message_listing_cb()
---
 obexd/client/map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 331aebc..5d7f0dc 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1182,8 +1182,10 @@ static void message_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
+	if (reply == NULL) {
+		g_free(contents);
 		return;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-- 
1.9.1

[PATCHv4 12/12] attrib: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 attrib/gatttool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index bca7f69..db5da62 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -340,6 +340,7 @@ static gboolean characteristics_write(gpointer user_data)
 
 	gatt_write_cmd(attrib, opt_handle, value, len, mainloop_quit, value);
 
+	g_free(value);
 	return FALSE;
 
 error:
@@ -393,6 +394,7 @@ static gboolean characteristics_write_req(gpointer user_data)
 	gatt_write_char(attrib, opt_handle, value, len, char_write_req_cb,
 									NULL);
 
+	g_free(value);
 	return FALSE;
 
 error:
-- 
1.9.1

Re: [PATCHv4 01/12] core: Remove unneeded assignment

[Andrei Emeltchenko]

ping

On Tue, Jul 29, 2014 at 05:20:50PM +0300, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Variable rfcomm is never used after assignment
> ---
>  src/profile.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/profile.c b/src/profile.c
> index 528525a..7aca3be 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -1272,7 +1272,6 @@ static uint32_t ext_start_servers(struct ext_profile *ext,
>  			error("RFCOMM server failed for %s: %s",
>  						ext->name, err->message);
>  			g_free(rfcomm);
> -			rfcomm = NULL;
>  			g_clear_error(&err);
>  			goto failed;
>  		} else {
> -- 
> 1.9.1
> 
> --
> 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

Re: [PATCHv4 09/12] tools: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> lmpver and hciver are allocated through malloc and need to be freed.
> ---
>  tools/parser/hci.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

I've applied all 8 patches before this one.

> diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> index 351f843..1a639af 100644
> --- a/tools/parser/hci.c
> +++ b/tools/parser/hci.c
> @@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
>  		p_indent(level, frm);
>  		printf("Error: %s\n", status2str(rp->status));
>  	} else {
> +		char *lmpver = lmp_vertostr(rp->lmp_ver);
> +		char *hciver = hci_vertostr(rp->hci_ver);
> +
>  		p_indent(level, frm);
>  		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
> -					hci_vertostr(rp->hci_ver),
> +					hciver ? hciver : "n/a",
>  					rp->hci_ver, btohs(rp->hci_rev));
>  		p_indent(level, frm);
>  		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
> -					lmp_vertostr(rp->lmp_ver),
> +					lmpver ? lmpver : "n/a",
>  					rp->lmp_ver, btohs(rp->lmp_subver));
>  		p_indent(level, frm);
>  		printf("Manufacturer: %s (%d)\n",
>  				bt_compidtostr(manufacturer), manufacturer);
> +
> +		if (lmpver)
> +			bt_free(lmpver);
> +		if (hciver)
> +			bt_free(hciver);

These trace back to using malloc (in hci_uint2str) so I suppose free is
more appropriate than bt_free (which should be used for bt_malloc).

Johan

Re: [PATCHv4 10/12] obexd: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> Fixes memory leak for folder_listing_cb().
> ---
>  obexd/client/map.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index d2d3d81..331aebc 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -271,8 +271,10 @@ static void folder_listing_cb(struct obc_session *session,
>  	}
>  
>  	reply = dbus_message_new_method_return(request->msg);
> -	if (reply == NULL)
> +	if (reply == NULL) {
> +		g_free(contents);
>  		return;
> +	}

Shouldn't you call pending_requesT_free(request) before returning?

Johan

Re: [PATCHv4 11/12] obexd: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> Fixes memory leak for message_listing_cb()
> ---
>  obexd/client/map.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 331aebc..5d7f0dc 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -1182,8 +1182,10 @@ static void message_listing_cb(struct obc_session *session,
>  	}
>  
>  	reply = dbus_message_new_method_return(request->msg);
> -	if (reply == NULL)
> +	if (reply == NULL) {
> +		g_free(contents);
>  		return;
> +	}
>  

Here also there seems to be a missing call to pending_request_free().
Might be simplest if you simply replace the return with goto done and
then put the g_dbus_send_message call behind a NULL check (same applies
to the previous patch).

Johan

Re: [PATCHv4 12/12] attrib: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> ---
>  attrib/gatttool.c | 2 ++
>  1 file changed, 2 insertions(+)

This patch has also been applied. Thanks.

Johan

Re: [PATCHv4 09/12] tools: Fix memory leak

[Andrei Emeltchenko]

Hi Johan,

On Fri, Aug 01, 2014 at 01:35:17PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> > lmpver and hciver are allocated through malloc and need to be freed.
> > ---
> >  tools/parser/hci.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> I've applied all 8 patches before this one.
> 
> > diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> > index 351f843..1a639af 100644
> > --- a/tools/parser/hci.c
> > +++ b/tools/parser/hci.c
> > @@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
> >  		p_indent(level, frm);
> >  		printf("Error: %s\n", status2str(rp->status));
> >  	} else {
> > +		char *lmpver = lmp_vertostr(rp->lmp_ver);
> > +		char *hciver = hci_vertostr(rp->hci_ver);
> > +
> >  		p_indent(level, frm);
> >  		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
> > -					hci_vertostr(rp->hci_ver),
> > +					hciver ? hciver : "n/a",
> >  					rp->hci_ver, btohs(rp->hci_rev));
> >  		p_indent(level, frm);
> >  		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
> > -					lmp_vertostr(rp->lmp_ver),
> > +					lmpver ? lmpver : "n/a",
> >  					rp->lmp_ver, btohs(rp->lmp_subver));
> >  		p_indent(level, frm);
> >  		printf("Manufacturer: %s (%d)\n",
> >  				bt_compidtostr(manufacturer), manufacturer);
> > +
> > +		if (lmpver)
> > +			bt_free(lmpver);
> > +		if (hciver)
> > +			bt_free(hciver);
> 
> These trace back to using malloc (in hci_uint2str) so I suppose free is
> more appropriate than bt_free (which should be used for bt_malloc).

OK, I will change it to free(). Shall I also change other similar
bt_free() calls which I took as example?

Best regards 
Andrei Emeltchenko 

[PATCHv4 1/3] tools: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

lmpver and hciver are allocated through malloc and need to be freed.
---
 tools/parser/hci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/parser/hci.c b/tools/parser/hci.c
index 351f843..cd52cb5 100644
--- a/tools/parser/hci.c
+++ b/tools/parser/hci.c
@@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(rp->status));
 	} else {
+		char *lmpver = lmp_vertostr(rp->lmp_ver);
+		char *hciver = hci_vertostr(rp->hci_ver);
+
 		p_indent(level, frm);
 		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
-					hci_vertostr(rp->hci_ver),
+					hciver ? hciver : "n/a",
 					rp->hci_ver, btohs(rp->hci_rev));
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-					lmp_vertostr(rp->lmp_ver),
+					lmpver ? lmpver : "n/a",
 					rp->lmp_ver, btohs(rp->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 				bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			free(lmpver);
+		if (hciver)
+			free(hciver);
 	}
 }
 
@@ -3178,13 +3186,18 @@ static inline void read_remote_version_complete_dump(int level, struct frame *fr
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(evt->status));
 	} else {
+		char *lmpver = lmp_vertostr(evt->lmp_ver);
+
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-			lmp_vertostr(evt->lmp_ver), evt->lmp_ver,
+			lmpver ? lmpver : "n/a", evt->lmp_ver,
 			btohs(evt->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 			bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			free(lmpver);
 	}
 }
 
-- 
1.9.1

[PATCHv4 2/3] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for folder_listing_cb().
---
 obexd/client/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index d2d3d81..0ef5e0f 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -271,8 +271,10 @@ static void folder_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
-		return;
+	if (reply == NULL) {
+		g_free(contents);
+		goto clean;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
@@ -288,6 +290,7 @@ static void folder_listing_cb(struct obc_session *session,
 
 done:
 	g_dbus_send_message(conn, reply);
+clean:
 	pending_request_free(request);
 }
 
-- 
1.9.1

[PATCHv4 3/3] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for message_listing_cb()
---
 obexd/client/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 0ef5e0f..47afc31 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1183,8 +1183,10 @@ static void message_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
-		return;
+	if (reply == NULL) {
+		g_free(contents);
+		goto clean;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
@@ -1211,6 +1213,7 @@ static void message_listing_cb(struct obc_session *session,
 
 done:
 	g_dbus_send_message(conn, reply);
+clean:
 	pending_request_free(request);
 }
 
-- 
1.9.1

Re: [PATCHv4 09/12] tools: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Fri, Aug 01, 2014, Andrei Emeltchenko wrote:
> Hi Johan,
> 
> On Fri, Aug 01, 2014 at 01:35:17PM +0300, Johan Hedberg wrote:
> > Hi Andrei,
> > 
> > On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> > > lmpver and hciver are allocated through malloc and need to be freed.
> > > ---
> > >  tools/parser/hci.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > I've applied all 8 patches before this one.
> > 
> > > diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> > > index 351f843..1a639af 100644
> > > --- a/tools/parser/hci.c
> > > +++ b/tools/parser/hci.c
> > > @@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
> > >  		p_indent(level, frm);
> > >  		printf("Error: %s\n", status2str(rp->status));
> > >  	} else {
> > > +		char *lmpver = lmp_vertostr(rp->lmp_ver);
> > > +		char *hciver = hci_vertostr(rp->hci_ver);
> > > +
> > >  		p_indent(level, frm);
> > >  		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
> > > -					hci_vertostr(rp->hci_ver),
> > > +					hciver ? hciver : "n/a",
> > >  					rp->hci_ver, btohs(rp->hci_rev));
> > >  		p_indent(level, frm);
> > >  		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
> > > -					lmp_vertostr(rp->lmp_ver),
> > > +					lmpver ? lmpver : "n/a",
> > >  					rp->lmp_ver, btohs(rp->lmp_subver));
> > >  		p_indent(level, frm);
> > >  		printf("Manufacturer: %s (%d)\n",
> > >  				bt_compidtostr(manufacturer), manufacturer);
> > > +
> > > +		if (lmpver)
> > > +			bt_free(lmpver);
> > > +		if (hciver)
> > > +			bt_free(hciver);
> > 
> > These trace back to using malloc (in hci_uint2str) so I suppose free is
> > more appropriate than bt_free (which should be used for bt_malloc).
> 
> OK, I will change it to free(). Shall I also change other similar
> bt_free() calls which I took as example?

I'm not sure it's worth bothering with legacy code like this which will
eventually disappear from the tree.

Johan

[PATCHv5 1/3] tools: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

lmpver and hciver are allocated through malloc and need to be freed.
---
 tools/parser/hci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/parser/hci.c b/tools/parser/hci.c
index 351f843..cd52cb5 100644
--- a/tools/parser/hci.c
+++ b/tools/parser/hci.c
@@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(rp->status));
 	} else {
+		char *lmpver = lmp_vertostr(rp->lmp_ver);
+		char *hciver = hci_vertostr(rp->hci_ver);
+
 		p_indent(level, frm);
 		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
-					hci_vertostr(rp->hci_ver),
+					hciver ? hciver : "n/a",
 					rp->hci_ver, btohs(rp->hci_rev));
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-					lmp_vertostr(rp->lmp_ver),
+					lmpver ? lmpver : "n/a",
 					rp->lmp_ver, btohs(rp->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 				bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			free(lmpver);
+		if (hciver)
+			free(hciver);
 	}
 }
 
@@ -3178,13 +3186,18 @@ static inline void read_remote_version_complete_dump(int level, struct frame *fr
 		p_indent(level, frm);
 		printf("Error: %s\n", status2str(evt->status));
 	} else {
+		char *lmpver = lmp_vertostr(evt->lmp_ver);
+
 		p_indent(level, frm);
 		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
-			lmp_vertostr(evt->lmp_ver), evt->lmp_ver,
+			lmpver ? lmpver : "n/a", evt->lmp_ver,
 			btohs(evt->lmp_subver));
 		p_indent(level, frm);
 		printf("Manufacturer: %s (%d)\n",
 			bt_compidtostr(manufacturer), manufacturer);
+
+		if (lmpver)
+			free(lmpver);
 	}
 }
 
-- 
1.9.1

[PATCHv5 2/3] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for folder_listing_cb().
---
 obexd/client/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index d2d3d81..0ef5e0f 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -271,8 +271,10 @@ static void folder_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
-		return;
+	if (reply == NULL) {
+		g_free(contents);
+		goto clean;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
@@ -288,6 +290,7 @@ static void folder_listing_cb(struct obc_session *session,
 
 done:
 	g_dbus_send_message(conn, reply);
+clean:
 	pending_request_free(request);
 }
 
-- 
1.9.1

[PATCHv5 3/3] obexd: Fix memory leak

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes memory leak for message_listing_cb()
---
 obexd/client/map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 0ef5e0f..47afc31 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1183,8 +1183,10 @@ static void message_listing_cb(struct obc_session *session,
 	}
 
 	reply = dbus_message_new_method_return(request->msg);
-	if (reply == NULL)
-		return;
+	if (reply == NULL) {
+		g_free(contents);
+		goto clean;
+	}
 
 	dbus_message_iter_init_append(reply, &iter);
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
@@ -1211,6 +1213,7 @@ static void message_listing_cb(struct obc_session *session,
 
 done:
 	g_dbus_send_message(conn, reply);
+clean:
 	pending_request_free(request);
 }
 
-- 
1.9.1

Re: [PATCHv4 09/12] tools: Fix memory leak

[Andrei Emeltchenko]

Hi Johan,

On Fri, Aug 01, 2014 at 02:21:24PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Fri, Aug 01, 2014, Andrei Emeltchenko wrote:
> > Hi Johan,
> > 
> > On Fri, Aug 01, 2014 at 01:35:17PM +0300, Johan Hedberg wrote:
> > > Hi Andrei,
> > > 
> > > On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> > > > lmpver and hciver are allocated through malloc and need to be freed.
> > > > ---
> > > >  tools/parser/hci.c | 19 ++++++++++++++++---
> > > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > I've applied all 8 patches before this one.
> > > 
> > > > diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> > > > index 351f843..1a639af 100644
> > > > --- a/tools/parser/hci.c
> > > > +++ b/tools/parser/hci.c
> > > > @@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
> > > >  		p_indent(level, frm);
> > > >  		printf("Error: %s\n", status2str(rp->status));
> > > >  	} else {
> > > > +		char *lmpver = lmp_vertostr(rp->lmp_ver);
> > > > +		char *hciver = hci_vertostr(rp->hci_ver);
> > > > +
> > > >  		p_indent(level, frm);
> > > >  		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
> > > > -					hci_vertostr(rp->hci_ver),
> > > > +					hciver ? hciver : "n/a",
> > > >  					rp->hci_ver, btohs(rp->hci_rev));
> > > >  		p_indent(level, frm);
> > > >  		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
> > > > -					lmp_vertostr(rp->lmp_ver),
> > > > +					lmpver ? lmpver : "n/a",
> > > >  					rp->lmp_ver, btohs(rp->lmp_subver));
> > > >  		p_indent(level, frm);
> > > >  		printf("Manufacturer: %s (%d)\n",
> > > >  				bt_compidtostr(manufacturer), manufacturer);
> > > > +
> > > > +		if (lmpver)
> > > > +			bt_free(lmpver);
> > > > +		if (hciver)
> > > > +			bt_free(hciver);
> > > 
> > > These trace back to using malloc (in hci_uint2str) so I suppose free is
> > > more appropriate than bt_free (which should be used for bt_malloc).
> > 
> > OK, I will change it to free(). Shall I also change other similar
> > bt_free() calls which I took as example?
> 
> I'm not sure it's worth bothering with legacy code like this which will
> eventually disappear from the tree.

Those constructions are in hcitool and hciconfig

Best regards 
Andrei Emeltchenko 

Re: [PATCHv4 09/12] tools: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Fri, Aug 01, 2014, Andrei Emeltchenko wrote:
> On Fri, Aug 01, 2014 at 02:21:24PM +0300, Johan Hedberg wrote:
> > Hi Andrei,
> > 
> > On Fri, Aug 01, 2014, Andrei Emeltchenko wrote:
> > > Hi Johan,
> > > 
> > > On Fri, Aug 01, 2014 at 01:35:17PM +0300, Johan Hedberg wrote:
> > > > Hi Andrei,
> > > > 
> > > > On Tue, Jul 29, 2014, Andrei Emeltchenko wrote:
> > > > > lmpver and hciver are allocated through malloc and need to be freed.
> > > > > ---
> > > > >  tools/parser/hci.c | 19 ++++++++++++++++---
> > > > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > > 
> > > > I've applied all 8 patches before this one.
> > > > 
> > > > > diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> > > > > index 351f843..1a639af 100644
> > > > > --- a/tools/parser/hci.c
> > > > > +++ b/tools/parser/hci.c
> > > > > @@ -2445,17 +2445,25 @@ static inline void read_local_version_dump(int level, struct frame *frm)
> > > > >  		p_indent(level, frm);
> > > > >  		printf("Error: %s\n", status2str(rp->status));
> > > > >  	} else {
> > > > > +		char *lmpver = lmp_vertostr(rp->lmp_ver);
> > > > > +		char *hciver = hci_vertostr(rp->hci_ver);
> > > > > +
> > > > >  		p_indent(level, frm);
> > > > >  		printf("HCI Version: %s (0x%x) HCI Revision: 0x%x\n",
> > > > > -					hci_vertostr(rp->hci_ver),
> > > > > +					hciver ? hciver : "n/a",
> > > > >  					rp->hci_ver, btohs(rp->hci_rev));
> > > > >  		p_indent(level, frm);
> > > > >  		printf("LMP Version: %s (0x%x) LMP Subversion: 0x%x\n",
> > > > > -					lmp_vertostr(rp->lmp_ver),
> > > > > +					lmpver ? lmpver : "n/a",
> > > > >  					rp->lmp_ver, btohs(rp->lmp_subver));
> > > > >  		p_indent(level, frm);
> > > > >  		printf("Manufacturer: %s (%d)\n",
> > > > >  				bt_compidtostr(manufacturer), manufacturer);
> > > > > +
> > > > > +		if (lmpver)
> > > > > +			bt_free(lmpver);
> > > > > +		if (hciver)
> > > > > +			bt_free(hciver);
> > > > 
> > > > These trace back to using malloc (in hci_uint2str) so I suppose free is
> > > > more appropriate than bt_free (which should be used for bt_malloc).
> > > 
> > > OK, I will change it to free(). Shall I also change other similar
> > > bt_free() calls which I took as example?
> > 
> > I'm not sure it's worth bothering with legacy code like this which will
> > eventually disappear from the tree.
> 
> Those constructions are in hcitool and hciconfig

Still legacy code. If you really feel like there's nothing else
important to work on feel free to send patches for that.

Johan

Re: [PATCHv4 09/12] tools: Fix memory leak

[Andrei Emeltchenko]

Hi Johan,

On Fri, Aug 01, 2014 at 02:33:10PM +0300, Johan Hedberg wrote:
> > > > > > +		if (lmpver)
> > > > > > +			bt_free(lmpver);
> > > > > > +		if (hciver)
> > > > > > +			bt_free(hciver);
> > > > > 
> > > > > These trace back to using malloc (in hci_uint2str) so I suppose free is
> > > > > more appropriate than bt_free (which should be used for bt_malloc).
> > > > 
> > > > OK, I will change it to free(). Shall I also change other similar
> > > > bt_free() calls which I took as example?
> > > 
> > > I'm not sure it's worth bothering with legacy code like this which will
> > > eventually disappear from the tree.
> > 
> > Those constructions are in hcitool and hciconfig
> 
> Still legacy code. If you really feel like there's nothing else
> important to work on feel free to send patches for that.

The question is if we change some programming practice shall we make other
code looks consistent.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv5 1/3] tools: Fix memory leak

[Johan Hedberg]

Hi Andrei,

On Fri, Aug 01, 2014, Andrei Emeltchenko wrote:
> lmpver and hciver are allocated through malloc and need to be freed.
> ---
>  tools/parser/hci.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

All three patches have been applied. Thanks.

Johan