[PATCH 1/3] Fix dbus reply memory leak

[PATCH 1/3] Fix dbus reply memory leak

[Syam Sidhardhan]

---
 audio/telephony-maemo5.c |    8 ++++----
 cups/main.c              |   30 +++++++++++++++++++++++++-----
 test/agent.c             |    2 ++
 test/mpris-player.c      |    6 +++++-
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/audio/telephony-maemo5.c b/audio/telephony-maemo5.c
index 23801c0..2832062 100644
--- a/audio/telephony-maemo5.c
+++ b/audio/telephony-maemo5.c
@@ -1599,12 +1599,12 @@ static void signal_strength_reply(DBusPendingCall *call, void *user_data)
 		error("Unable to parse signal_strength reply: %s, %s",
 							err.name, err.message);
 		dbus_error_free(&err);
-		return;
+		goto done;
 	}
 
 	if (net_err != 0) {
 		error("get_signal_strength failed with code %d", net_err);
-		return;
+		goto done;
 	}
 
 	update_signal_strength(signals_bar);
@@ -1654,12 +1654,12 @@ static void registration_status_reply(DBusPendingCall *call, void *user_data)
 		error("Unable to parse registration_status_change reply:"
 					" %s, %s", err.name, err.message);
 		dbus_error_free(&err);
-		return;
+		goto done;
 	}
 
 	if (net_err != 0) {
 		error("get_registration_status failed with code %d", net_err);
-		return;
+		goto done;
 	}
 
 	update_registration_status(status, lac, cell_id, operator_code,
diff --git a/cups/main.c b/cups/main.c
index 7f3f4b0..a884c6e 100644
--- a/cups/main.c
+++ b/cups/main.c
@@ -348,10 +348,15 @@ static void remote_device_found(const char *adapter, const char *bdaddr,
 
 		dbus_message_unref(message);
 
+		if (!adapter_reply)
+			return;
+
 		if (dbus_message_get_args(adapter_reply, NULL,
 					DBUS_TYPE_OBJECT_PATH, &adapter,
-					DBUS_TYPE_INVALID) == FALSE)
+					DBUS_TYPE_INVALID) == FALSE) {
+			dbus_message_unref(adapter_reply);
 			return;
+		}
 	}
 
 	message = dbus_message_new_method_call("org.bluez", adapter,
@@ -386,12 +391,16 @@ static void remote_device_found(const char *adapter, const char *bdaddr,
 
 	if (dbus_message_get_args(reply, NULL,
 					DBUS_TYPE_OBJECT_PATH, &object_path,
-					DBUS_TYPE_INVALID) == FALSE)
+					DBUS_TYPE_INVALID) == FALSE) {
+		dbus_message_unref(reply);
 		return;
+	}
 
 	id = device_get_ieee1284_id(adapter, object_path);
 	add_device_to_list(name, bdaddr, id);
 	g_free(id);
+
+	dbus_message_unref(reply);
 }
 
 static void discovery_completed(void)
@@ -642,10 +651,15 @@ static gboolean print_ieee1284(const char *bdaddr)
 
 	dbus_message_unref(message);
 
+	if (!adapter_reply)
+		return FALSE;
+
 	if (dbus_message_get_args(adapter_reply, NULL,
 			DBUS_TYPE_OBJECT_PATH, &adapter,
-			DBUS_TYPE_INVALID) == FALSE)
+			DBUS_TYPE_INVALID) == FALSE) {
+		dbus_message_unref(adapter_reply);
 		return FALSE;
+	}
 
 	message = dbus_message_new_method_call("org.bluez", adapter,
 			"org.bluez.Adapter",
@@ -680,15 +694,21 @@ static gboolean print_ieee1284(const char *bdaddr)
 
 	if (dbus_message_get_args(reply, NULL,
 					DBUS_TYPE_OBJECT_PATH, &object_path,
-					DBUS_TYPE_INVALID) == FALSE)
+					DBUS_TYPE_INVALID) == FALSE) {
+		dbus_message_unref(reply);
 		return FALSE;
+	}
 
 	id = device_get_ieee1284_id(adapter, object_path);
-	if (id == NULL)
+	if (id == NULL) {
+		dbus_message_unref(reply);
 		return FALSE;
+	}
 	printf("%s", id);
 	g_free(id);
 
+	dbus_message_unref(reply);
+
 	return TRUE;
 }
 
diff --git a/test/agent.c b/test/agent.c
index ae74074..5cdeeb4 100644
--- a/test/agent.c
+++ b/test/agent.c
@@ -504,6 +504,7 @@ static char *get_default_adapter_path(DBusConnection *conn)
 			fprintf(stderr, "%s\n", err.message);
 			dbus_error_free(&err);
 		}
+		dbus_message_unref(reply);
 		return NULL;
 	}
 
@@ -562,6 +563,7 @@ static char *get_adapter_path(DBusConnection *conn, const char *adapter)
 			fprintf(stderr, "%s\n", err.message);
 			dbus_error_free(&err);
 		}
+		dbus_message_unref(reply);
 		return NULL;
 	}
 
diff --git a/test/mpris-player.c b/test/mpris-player.c
index a1632f3..a2c4cc6 100644
--- a/test/mpris-player.c
+++ b/test/mpris-player.c
@@ -700,6 +700,7 @@ static char *get_default_adapter(DBusConnection *conn)
 			fprintf(stderr, "%s\n", err.message);
 			dbus_error_free(&err);
 		}
+		dbus_message_unref(reply);
 		return NULL;
 	}
 
@@ -756,6 +757,7 @@ static char *get_adapter(DBusConnection *conn, const char *adapter)
 			fprintf(stderr, "%s\n", err.message);
 			dbus_error_free(&err);
 		}
+		dbus_message_unref(reply);
 		return NULL;
 	}
 
@@ -802,8 +804,10 @@ static char *get_name_owner(DBusConnection *conn, const char *name)
 
 	if (!dbus_message_get_args(reply, NULL,
 					DBUS_TYPE_STRING, &owner,
-					DBUS_TYPE_INVALID))
+					DBUS_TYPE_INVALID)) {
+		dbus_message_unref(reply);
 		return NULL;
+	}
 
 	owner = g_strdup(owner);
 
-- 
1.7.4.1

[PATCH 2/3] Remove unwanted GError* assignment to NULL

[Syam Sidhardhan]

---
 health/hdp.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/health/hdp.c b/health/hdp.c
index d167ab0..403d4c8 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -551,7 +551,6 @@ static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)
 	hdp_tmp_dc_data_unref(hdp_conn);
 	hdp_conn->cb(hdp_chann->mdl, err, hdp_conn);
 	g_error_free(gerr);
-	gerr = NULL;
 }
 
 static void device_reconnect_mdl_cb(struct mcap_mdl *mdl, GError *err,
-- 
1.7.4.1

[PATCH 3/3] Send the Extended Error result code, if requested in the failure cases

[Syam Sidhardhan]

If HF has already requested for the Extended Error result code reporting,
then send the same in certain failure cases. Earlier in this case we were
sending normal Error.
---
 audio/headset.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/audio/headset.c b/audio/headset.c
index 6aef6a8..2bddedc 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1331,7 +1331,13 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
 		if (err == -EINVAL) {
 			error("Badly formated or unrecognized command: %s",
 					&slc->buf[slc->data_start]);
-			err = headset_send(hs, "\r\nERROR\r\n");
+
+			if (slc->enabled)
+				err = headset_send(hs, "\r\n+CME ERROR: %d\r\n",
+						CME_ERROR_NOT_SUPPORTED);
+			else
+				err = headset_send(hs, "\r\nERROR\r\n");
+
 			if (err < 0)
 				goto failed;
 		} else if (err < 0)
-- 
1.7.4.1

Re: [PATCH 3/3] Send the Extended Error result code, if requested in the failure cases

[Syam Sidhardhan]

Hi,

----- Original Message ----- 
From: "Syam Sidhardhan" <s.syam@samsung.com>
To: <linux-bluetooth@vger.kernel.org>
Cc: "Syam Sidhardhan" <s.syam@samsung.com>
Sent: Thursday, November 24, 2011 7:44 PM
Subject: [PATCH 3/3] Send the Extended Error result code, if requested in 
the failure cases


> If HF has already requested for the Extended Error result code reporting,
> then send the same in certain failure cases. Earlier in this case we were
> sending normal Error.
> ---
> audio/headset.c |    8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/audio/headset.c b/audio/headset.c
> index 6aef6a8..2bddedc 100644
> --- a/audio/headset.c
> +++ b/audio/headset.c
> @@ -1331,7 +1331,13 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, 
> GIOCondition cond,
>  if (err == -EINVAL) {
>  error("Badly formated or unrecognized command: %s",
>  &slc->buf[slc->data_start]);
> - err = headset_send(hs, "\r\nERROR\r\n");
> +
> + if (slc->enabled)
> + err = headset_send(hs, "\r\n+CME ERROR: %d\r\n",
> + CME_ERROR_NOT_SUPPORTED);
> + else
> + err = headset_send(hs, "\r\nERROR\r\n");
> +
>  if (err < 0)
>  goto failed;
>  } else if (err < 0)
> -- 
> 1.7.4.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

Kindly ignore this patch. Its should be if (slc->cme_enabled). Sorry, By 
mistake got posted before correcting.
Also I want to distinguish CME error message in the case of invalid AT 
commands and parse error(eg AT+CMER ).
Both case handle_event() returns -EINVAL.


Regards,
Syam.

Re: [PATCH 1/3] Fix dbus reply memory leak

[Johan Hedberg]

Hi Syam,

On Thu, Nov 24, 2011, Syam Sidhardhan wrote:
> ---
>  audio/telephony-maemo5.c |    8 ++++----
>  cups/main.c              |   30 +++++++++++++++++++++++++-----
>  test/agent.c             |    2 ++
>  test/mpris-player.c      |    6 +++++-
>  4 files changed, 36 insertions(+), 10 deletions(-)

Your commit message uses the word leak in singular form but there are
multiple fixes in this patch, i.e. the commit message is misleading. In
this case I'd split the patch into four separate ones:

telephony-maemo5: Fix D-Bus reply memory leaks
cups: Fix D-Bus reply memory leaks
agent: Fix D-Bus reply memory leaks
mpris-player: Fix D-Bus reply memory leaks

Johan

Re: [PATCH 2/3] Remove unwanted GError* assignment to NULL

[Johan Hedberg]

Hi Syam,

On Thu, Nov 24, 2011, Syam Sidhardhan wrote:
> ---
>  health/hdp.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/health/hdp.c b/health/hdp.c
> index d167ab0..403d4c8 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -551,7 +551,6 @@ static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)
>  	hdp_tmp_dc_data_unref(hdp_conn);
>  	hdp_conn->cb(hdp_chann->mdl, err, hdp_conn);
>  	g_error_free(gerr);
> -	gerr = NULL;
>  }

Applied, however a bigger question is whether gerr is even needed in
this function at all since it's never used after potentially being set.

Johan

Re: [PATCH 2/3] Remove unwanted GError* assignment to NULL

[Santiago Carot]

Hi,

2011/12/2 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Syam,
>
> On Thu, Nov 24, 2011, Syam Sidhardhan wrote:
>> ---
>>  health/hdp.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/health/hdp.c b/health/hdp.c
>> index d167ab0..403d4c8 100644
>> --- a/health/hdp.c
>> +++ b/health/hdp.c
>> @@ -551,7 +551,6 @@ static void hdp_get_dcpsm_cb(uint16_t dcpsm, gpointer user_data, GError *err)
>>       hdp_tmp_dc_data_unref(hdp_conn);
>>       hdp_conn->cb(hdp_chann->mdl, err, hdp_conn);
>>       g_error_free(gerr);
>> -     gerr = NULL;
>>  }
>
> Applied, however a bigger question is whether gerr is even needed in
> this function at all since it's never used after potentially being set.
>

I think it would be better get ride of it at least we can add a debug
message here if MCAP fails trying to connect the data channel.

Regards.

Re: [PATCH 1/3] Fix dbus reply memory leak

[Syam Sidhardhan]

Hi Johan,
On 12/2/2011 4:45 PM, Johan Hedberg wrote:
> Hi Syam,
>
> On Thu, Nov 24, 2011, Syam Sidhardhan wrote:
>    
>> ---
>>   audio/telephony-maemo5.c |    8 ++++----
>>   cups/main.c              |   30 +++++++++++++++++++++++++-----
>>   test/agent.c             |    2 ++
>>   test/mpris-player.c      |    6 +++++-
>>   4 files changed, 36 insertions(+), 10 deletions(-)
>>      
> Your commit message uses the word leak in singular form but there are
> multiple fixes in this patch, i.e. the commit message is misleading. In
> this case I'd split the patch into four separate ones:
>
> telephony-maemo5: Fix D-Bus reply memory leaks
> cups: Fix D-Bus reply memory leaks
> agent: Fix D-Bus reply memory leaks
> mpris-player: Fix D-Bus reply memory leaks
>
> Johan
>    

Yes, you are correct. You can split it into multiple patches.
Thanks in advance.

Syam

Re: [PATCH 1/3] Fix dbus reply memory leak

[gene heskett]

On Saturday, December 03, 2011 03:54:56 PM Syam Sidhardhan did opine:

> Hi Johan,
> 
> On 12/2/2011 4:45 PM, Johan Hedberg wrote:
> > Hi Syam,
> > 
> > On Thu, Nov 24, 2011, Syam Sidhardhan wrote:
> >> ---
> >> 
> >>   audio/telephony-maemo5.c |    8 ++++----
> >>   cups/main.c              |   30 +++++++++++++++++++++++++-----
> >>   test/agent.c             |    2 ++
> >>   test/mpris-player.c      |    6 +++++-
> >>   4 files changed, 36 insertions(+), 10 deletions(-)
> > 
> > Your commit message uses the word leak in singular form but there are
> > multiple fixes in this patch, i.e. the commit message is misleading.
> > In this case I'd split the patch into four separate ones:
> > 
> > telephony-maemo5: Fix D-Bus reply memory leaks
> > cups: Fix D-Bus reply memory leaks
> > agent: Fix D-Bus reply memory leaks
> > mpris-player: Fix D-Bus reply memory leaks
> > 
> > Johan
> 
> Yes, you are correct. You can split it into multiple patches.
> Thanks in advance.
> 
> Syam

Probably off topic to this patch discussion, but whats chances, while you 
folks are kicking dbus's tires, of fixing the thing so that if the target 
doesn't exist, the message sent gets thrown under the buss instead of 
blocking, which then requires a kill of both processes, and a proper 
sequentially done restart (target started first) to make it work?

Cheers, Gene
-- 
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
My web page: <http://coyoteden.dyndns-free.com:85/gene>
I am just a nice, clean-cut Mongolian boy.
	-- Yul Brynner, 1956

Re: [PATCH 1/3] Fix dbus reply memory leak

[Johan Hedberg]

Hi Syam,

On Sun, Dec 04, 2011, Syam Sidhardhan wrote:
> >Your commit message uses the word leak in singular form but there are
> >multiple fixes in this patch, i.e. the commit message is misleading. In
> >this case I'd split the patch into four separate ones:
> >
> >telephony-maemo5: Fix D-Bus reply memory leaks
> >cups: Fix D-Bus reply memory leaks
> >agent: Fix D-Bus reply memory leaks
> >mpris-player: Fix D-Bus reply memory leaks
> >
> >Johan
> 
> Yes, you are correct. You can split it into multiple patches.
> Thanks in advance.

Maybe I was a bit unclear: I'm expecting *you* to do this split and
resend the patches. Thanks :)

Johan

Re: [PATCH 1/3] Fix dbus reply memory leak

[Syam Sidhardhan]

Hi Johan,

----- Original Message ----- 
From: "Johan Hedberg" <johan.hedberg@gmail.com>
To: "Syam Sidhardhan" <syamsidhardh@gmail.com>
Cc: "Syam Sidhardhan" <s.syam@samsung.com>; 
<linux-bluetooth@vger.kernel.org>
Sent: Monday, December 05, 2011 2:25 AM
Subject: Re: [PATCH 1/3] Fix dbus reply memory leak


> Hi Syam,
>
> On Sun, Dec 04, 2011, Syam Sidhardhan wrote:
>> >Your commit message uses the word leak in singular form but there are
>> >multiple fixes in this patch, i.e. the commit message is misleading. In
>> >this case I'd split the patch into four separate ones:
>> >
>> >telephony-maemo5: Fix D-Bus reply memory leaks
>> >cups: Fix D-Bus reply memory leaks
>> >agent: Fix D-Bus reply memory leaks
>> >mpris-player: Fix D-Bus reply memory leaks
>> >
>> >Johan
>>
>> Yes, you are correct. You can split it into multiple patches.
>> Thanks in advance.
>
> Maybe I was a bit unclear: I'm expecting *you* to do this split and
> resend the patches. Thanks :)
>
> Johan

Ok :-),
I'll split it and send.

Regadrs,
Syam