2011-11-24 14:14:34

by Syam Sidhardhan

[permalink] [raw]
Subject: [PATCH 1/3] Fix dbus reply memory leak

---
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



2011-11-24 14:14:36

by Syam Sidhardhan

[permalink] [raw]
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


2011-11-24 14:14:35

by Syam Sidhardhan

[permalink] [raw]
Subject: [PATCH 2/3] Remove unwanted GError* assignment to NULL

---
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


2011-12-05 15:20:06

by Syam Sidhardhan

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

Hi Johan,

----- Original Message -----
From: "Johan Hedberg" <[email protected]>
To: "Syam Sidhardhan" <[email protected]>
Cc: "Syam Sidhardhan" <[email protected]>;
<[email protected]>
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


2011-12-04 20:55:41

by Johan Hedberg

[permalink] [raw]
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

2011-12-03 21:00:26

by gene heskett

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

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

2011-12-03 20:25:14

by Syam Sidhardhan

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

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

2011-12-02 11:25:37

by Santiago Carot

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

Hi,

2011/12/2 Johan Hedberg <[email protected]>:
> 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.

2011-12-02 11:18:27

by Johan Hedberg

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

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

2011-12-02 11:15:10

by Johan Hedberg

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

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