[PATCH] sim: Handle multiple invocations of sim_imsi_obtained

[PATCH] sim: Handle multiple invocations of sim_imsi_obtained

[Slava Monich]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

To avoid leaking memory and issuing unnecessary D-Bus signals
---
 src/sim.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index edae5eb..1b0cb80 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1425,11 +1425,18 @@ static void sim_set_ready(struct ofono_sim *sim)
 
 static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
 {
-	DBusConnection *conn = ofono_dbus_get_connection();
-	const char *path = __ofono_atom_get_path(sim->atom);
+	DBusConnection *conn;
+	const char *path;
+
+	if (sim->imsi && !strcmp(sim->imsi, imsi))
+		return;
 
+	DBG("%s", imsi);
+	g_free(sim->imsi);
 	sim->imsi = g_strdup(imsi);
 
+	conn = ofono_dbus_get_connection();
+	path = __ofono_atom_get_path(sim->atom);
 	ofono_dbus_signal_property_changed(conn, path,
 						OFONO_SIM_MANAGER_INTERFACE,
 						"SubscriberIdentity",
-- 
1.8.3.2

Re: [PATCH] sim: Handle multiple invocations of sim_imsi_obtained

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

Hi Slava,

On 05/06/2014 06:54 AM, Slava Monich wrote:
> To avoid leaking memory and issuing unnecessary D-Bus signals
> ---
>  src/sim.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/sim.c b/src/sim.c
> index edae5eb..1b0cb80 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -1425,11 +1425,18 @@ static void sim_set_ready(struct ofono_sim *sim)
>  
>  static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>  {
> -	DBusConnection *conn = ofono_dbus_get_connection();
> -	const char *path = __ofono_atom_get_path(sim->atom);
> +	DBusConnection *conn;
> +	const char *path;
> +
> +	if (sim->imsi && !strcmp(sim->imsi, imsi))
> +		return;

Calling this function a second time is an error.  So this condition
looks highly suspect.  What issue are you trying to address?

>  
> +	DBG("%s", imsi);
> +	g_free(sim->imsi);
>  	sim->imsi = g_strdup(imsi);
>  
> +	conn = ofono_dbus_get_connection();
> +	path = __ofono_atom_get_path(sim->atom);
>  	ofono_dbus_signal_property_changed(conn, path,
>  						OFONO_SIM_MANAGER_INTERFACE,
>  						"SubscriberIdentity",
> 

Regards,
-Denis

Re: [PATCH] sim: Handle multiple invocations of sim_imsi_obtained

[Slava Monich]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi Denis,

>> To avoid leaking memory and issuing unnecessary D-Bus signals
>> ---
>>   src/sim.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/sim.c b/src/sim.c
>> index edae5eb..1b0cb80 100644
>> --- a/src/sim.c
>> +++ b/src/sim.c
>> @@ -1425,11 +1425,18 @@ static void sim_set_ready(struct ofono_sim *sim)
>>   
>>   static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>>   {
>> -	DBusConnection *conn = ofono_dbus_get_connection();
>> -	const char *path = __ofono_atom_get_path(sim->atom);
>> +	DBusConnection *conn;
>> +	const char *path;
>> +
>> +	if (sim->imsi && !strcmp(sim->imsi, imsi))
>> +		return;
> Calling this function a second time is an error.  So this condition
> looks highly suspect.  What issue are you trying to address?

sim_imsi_obtained() is invoked from two places: sim_imsi_cb() and 
sim_efimsi_cb(). sim_imsi_cb() in turn is a completion for 
sim_retrieve_imsi() which is invoked from 5 other asynchronous 
callbacks, mostly triggered by a number of ofono_sim_read() calls. I 
don't claim that I fully understand what's going on and I haven't 
analyzed all the call sequences but it surely is invoked twice in our 
setup. Any suggestions on where the problem might be?

Thanks,
-Slava

Re: [PATCH] sim: Handle multiple invocations of sim_imsi_obtained

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

Hi Slava,

>>> +    if (sim->imsi && !strcmp(sim->imsi, imsi))
>>> +        return;
>> Calling this function a second time is an error.  So this condition
>> looks highly suspect.  What issue are you trying to address?
> 
> sim_imsi_obtained() is invoked from two places: sim_imsi_cb() and
> sim_efimsi_cb(). sim_imsi_cb() in turn is a completion for

Only one of those functions is called, depending on how the driver
implements obtaining of the IMSI.  Some drivers do not have the ability
to read IMSI using the READ BINARY / READ RECORD SIM commands.

> sim_retrieve_imsi() which is invoked from 5 other asynchronous
> callbacks, mostly triggered by a number of ofono_sim_read() calls. I
> don't claim that I fully understand what's going on and I haven't
> analyzed all the call sequences but it surely is invoked twice in our
> setup. Any suggestions on where the problem might be?

The code flow of the functions deals with Barred / Fixed Dialing Numbers
and the variations between 2G and 3G sims.  The logic is set up in such
a way that sim_retrieve_imsi is called only once.  Looking at the code,
there doesn't seem to be any way for sim_retrieve_imsi to be called twice.

If this is indeed happening, then please let me know under what
circumstances this happens and I can help you track this issue down.

Regards,
-Denis