[PATCH] Don't pass down NULL path to dbus if no modem.

[PATCH] Don't pass down NULL path to dbus if no modem.

[Zhu Yanhai]

We shouldn't pass down NULL object path to dbus if there's no modems,
which is a common case on many netbooks.

Signed-off-by: Zhu Yanhai <yanhai.zhu@linux.intel.com>
---
 audio/telephony-ofono.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
index 2778cfc..4f4bf88 100644
--- a/audio/telephony-ofono.c
+++ b/audio/telephony-ofono.c
@@ -574,12 +574,20 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
 	while (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_INVALID) {
 
 		dbus_message_iter_get_basic(&sub, &modem_obj_path_local);
+		if (modem_obj_path_local == NULL)
+			continue;
+
 		modem_obj_path = g_strdup(modem_obj_path_local);
-		debug("modem_obj_path is %p, %s\n", modem_obj_path,
+		debug("modem_obj_path is %p, %s", modem_obj_path,
 						modem_obj_path);
 		dbus_message_iter_next(&sub);
 	}
 
+	if (modem_obj_path == NULL) {
+		debug("no modem found.");
+		goto done;
+	}
+
 	ret = get_registration_and_signal_status();
 	if (ret < 0)
 		error("get_registration_and_signal_status() failed(%d)", ret);
-- 
1.6.2.2

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Zhu Yanhai]

On 01/12/2010 02:15 PM, Zhu Yanhai wrote:
> We shouldn't pass down NULL object path to dbus if there's no modems,
> which is a common case on many netbooks.
> 
> Signed-off-by: Zhu Yanhai <yanhai.zhu@linux.intel.com>
> ---
>  audio/telephony-ofono.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
> index 2778cfc..4f4bf88 100644
> --- a/audio/telephony-ofono.c
> +++ b/audio/telephony-ofono.c
> @@ -574,12 +574,20 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
>  	while (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_INVALID) {
>  
>  		dbus_message_iter_get_basic(&sub, &modem_obj_path_local);
> +		if (modem_obj_path_local == NULL)
> +			continue;
> +
>  		modem_obj_path = g_strdup(modem_obj_path_local);
> -		debug("modem_obj_path is %p, %s\n", modem_obj_path,
> +		debug("modem_obj_path is %p, %s", modem_obj_path,
>  						modem_obj_path);
>  		dbus_message_iter_next(&sub);
>  	}
>  
> +	if (modem_obj_path == NULL) {
> +		debug("no modem found.");
> +		goto done;
> +	}
> +
>  	ret = get_registration_and_signal_status();
>  	if (ret < 0)
>  		error("get_registration_and_signal_status() failed(%d)", ret);

Johan,
Please correct me if there's some coding style issues.

Thanks
Zhu Yanhai

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Johan Hedberg]

Hi,

On Tue, Jan 12, 2010, Zhu Yanhai wrote:
> --- a/audio/telephony-ofono.c
> +++ b/audio/telephony-ofono.c
> @@ -574,12 +574,20 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
>  	while (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_INVALID) {
>  
>  		dbus_message_iter_get_basic(&sub, &modem_obj_path_local);
> +		if (modem_obj_path_local == NULL)
> +			continue;

dbus_message_iter_get_basic doesn't set the pointer to NULL in the case
of failure, so this check is useless. The D-Bus documentation says the
following about dbus_message_iter_get_basic:

"Be sure you have somehow checked that dbus_message_iter_get_arg_type()
matches the type you are expecting, or you'll crash when you try to use
an integer as a string or something."

You could either change the while loop condition from
"!= DBUS_TYPE_INVALID" to "== DBUS_TYPE_STRING", or if you just want to
pick the first modem in the list (which probably makes more sense than
the current method of picking the last one and leaking the path to every
modem before it) remove the loop completely and then bail out (goto
done) if the first parameter isn't a string.

In the future, please check the reference documentation instead of
guessing the usage of the API. You can find it e.g. here:
http://dbus.freedesktop.org/doc/api/html/group__DBus.html

>  		modem_obj_path = g_strdup(modem_obj_path_local);
> -		debug("modem_obj_path is %p, %s\n", modem_obj_path,
> +		debug("modem_obj_path is %p, %s", modem_obj_path,
>  						modem_obj_path);

The removal of the unnecessary newline character is good, but there are
a few other issues that are strange with this:

1. What's the value of printing the memory address of the string here?
2. Leaking modem_obj_path if it was already set

Since there are a few different issues to fix here, feel free to split
them up into separate patches (e.g. the summary of the commit message
isn't accurate anymore).

Johan

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Zhu Yanhai]

On 01/13/2010 02:28 PM, Johan Hedberg wrote:
> Hi,
> 
> On Tue, Jan 12, 2010, Zhu Yanhai wrote:
>> --- a/audio/telephony-ofono.c
>> +++ b/audio/telephony-ofono.c
>> @@ -574,12 +574,20 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
>>  	while (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_INVALID) {
>>  
>>  		dbus_message_iter_get_basic(&sub, &modem_obj_path_local);
>> +		if (modem_obj_path_local == NULL)
>> +			continue;
> 
> dbus_message_iter_get_basic doesn't set the pointer to NULL in the case
> of failure, so this check is useless. The D-Bus documentation says the
> following about dbus_message_iter_get_basic:
> 
> "Be sure you have somehow checked that dbus_message_iter_get_arg_type()
> matches the type you are expecting, or you'll crash when you try to use
> an integer as a string or something."
> 
> You could either change the while loop condition from
> "!= DBUS_TYPE_INVALID" to "== DBUS_TYPE_STRING", or if you just want to
> pick the first modem in the list (which probably makes more sense than
> the current method of picking the last one and leaking the path to every
> modem before it) remove the loop completely and then bail out (goto
> done) if the first parameter isn't a string.

Thanks for your review, I will write a new patch and send it out again.
> 
> In the future, please check the reference documentation instead of
> guessing the usage of the API. You can find it e.g. here:
> http://dbus.freedesktop.org/doc/api/html/group__DBus.html
> 
>>  		modem_obj_path = g_strdup(modem_obj_path_local);
>> -		debug("modem_obj_path is %p, %s\n", modem_obj_path,
>> +		debug("modem_obj_path is %p, %s", modem_obj_path,
>>  						modem_obj_path);
> 
> The removal of the unnecessary newline character is good, but there are
> a few other issues that are strange with this:
> 
> 1. What's the value of printing the memory address of the string here?

I don't know...seems useless. I will remove it in next patch
> 2. Leaking modem_obj_path if it was already set

I think it will be freed in telephony_exit().
> 
> Since there are a few different issues to fix here, feel free to split
> them up into separate patches (e.g. the summary of the commit message
> isn't accurate anymore).
> 
> Johan
> 

Regards,
Zhu Yanhai

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Johan Hedberg]

On Wed, Jan 13, 2010, Zhu Yanhai wrote:
> > 2. Leaking modem_obj_path if it was already set
> 
> I think it will be freed in telephony_exit().

The point was that if you fetch the modems list more than once or if
there's more than one modem the previous value of modem_obj_path will be
overwritten with a new one without freeing the original string first.

Johan

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Zhu Yanhai]

On 01/13/2010 03:35 PM, Johan Hedberg wrote:
> On Wed, Jan 13, 2010, Zhu Yanhai wrote:
>>> 2. Leaking modem_obj_path if it was already set
>> I think it will be freed in telephony_exit().
> 
> The point was that if you fetch the modems list more than once or if
> there's more than one modem the previous value of modem_obj_path will be
> overwritten with a new one without freeing the original string first.
> 
> Johan
> --
> 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
> 
I see...it seems that we don't have multiple modems plan yet...ok let's
just use the first modem we saw in the system. I will patch this part and
send a separate patch later today.

Thanks,
Zhu Yanhai

[PATCH] Don't pass down NULL path to dbus if no modem.

[Zhu Yanhai]

We shouldn't pass down NULL object path to dbus if there's no modem,
which is a common case on many netbooks.

Signed-off-by: Zhu Yanhai <yanhai.zhu@linux.intel.com>
---
 audio/telephony-ofono.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
index 2778cfc..2970e43 100644
--- a/audio/telephony-ofono.c
+++ b/audio/telephony-ofono.c
@@ -579,6 +579,11 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
 						modem_obj_path);
 		dbus_message_iter_next(&sub);
 	}
+	if (modem_obj_path == NULL)
+	{
+		debug("no modem found.\n");	
+		goto done;
+	}
 
 	ret = get_registration_and_signal_status();
 	if (ret < 0)
-- 
1.6.2.2

Re: [PATCH] Don't pass down NULL path to dbus if no modem.

[Johan Hedberg]

Hi,

On Mon, Jan 11, 2010, Zhu Yanhai wrote:
> index 2778cfc..2970e43 100644
> --- a/audio/telephony-ofono.c
> +++ b/audio/telephony-ofono.c
> @@ -579,6 +579,11 @@ static void list_modem_reply(DBusPendingCall *call, void *user_data)
>  						modem_obj_path);
>  		dbus_message_iter_next(&sub);
>  	}
> +	if (modem_obj_path == NULL)
> +	{
> +		debug("no modem found.\n");	
> +		goto done;
> +	}

Functionally the patch seems fine, but could you fix a few coding style
issues please:

 * empty line before the if-statement
 * opening { on the same line as the if statement
 * no \n in the debug statement (that gets added automatically).
 * Remove whitespace from the end of the debug line (right now you have
   a tab after the semicolon)

Johan