**** BEGIN LOGGING AT Sun Apr 03 02:59:58 2011
**** BEGIN LOGGING AT Sun Apr 03 23:55:21 2011
**** BEGIN LOGGING AT Mon Apr 04 15:14:14 2011
Apr 04 16:57:08 <pessi> aruravi: pong?
Apr 04 19:40:57 <Jeevaka_> denkenz: ping
Apr 04 19:41:15 <denkenz> Jeevaka: pong
Apr 04 19:42:36 <Jeevaka> any comments on the emergency number core modification patch?
Apr 04 19:43:58 <denkenz> which one?
Apr 04 19:44:34 <Jeevaka> http://lists.ofono.org/pipermail/ofono/2011-April/009922.html
Apr 04 19:47:41 <denkenz> My only concern there was the un-needed g_strduping
Apr 04 19:50:29 <denkenz> you already g_strdup when inserting into the hash table, there's no need to do so when building a string array for get properties
Apr 04 19:52:42 <Jeevaka> correct me if I'm wrong. String array(char **list) is allocated memory. if we use the pointers directly from hash table, then when we are freeing up the memory allocated for list, there will be an issue right?
Apr 04 19:54:15 <denkenz> only if we use g_strfreev
Apr 04 19:54:34 <denkenz> However, there's nothing stopping you from allocating a char ** array and only using g_free on it
Apr 04 19:55:33 <Jeevaka> ok
Apr 04 19:56:08 <Jeevaka> will do that. any other review comments?
Apr 04 19:57:25 <denkenz> Rest looks fine
Apr 04 19:58:27 <denkenz> However, you might want to put the variable declaration for the nw list into patch 3
Apr 04 19:58:32 <denkenz> And I don't like patch 3 at all
Apr 04 19:59:03 <denkenz> Breaks the driver / core memory ownership contract
Apr 04 19:59:03 <Jeevaka> hmm, reason?
Apr 04 20:00:06 <denkenz> g_strdupv might be better there
Apr 04 20:00:15 <Jeevaka> it doesn't as core is still not going to own the memory
Apr 04 20:00:40 <denkenz> yes, but the driver has to keep it indefinitely
Apr 04 20:00:49 <denkenz> Which is a bad idea
Apr 04 20:01:06 <denkenz> (e.g. what if a Refresh with EFecc is sent)
Apr 04 20:02:08 <Jeevaka> sim_en_list is maintained separately.
Apr 04 20:02:44 <Jeevaka> may be i'm not getting what you meant. little more briefing would do
Apr 04 20:03:41 <denkenz> You're relying on the fact that new_en_list points to driver memory
Apr 04 20:04:22 <denkenz> So any time we need to set the new_ecc (e.g. when an EFecc refresh happens, or reading EFecc simply takes a while), you rely that the driver keeps that memory around
Apr 04 20:04:42 <denkenz> This might work for the IFX implementation, but not really something you want the driver writers to know about
Apr 04 20:06:09 <Jeevaka> in that case as you said will duplicate the mempory
Apr 04 20:07:06 <denkenz> There it is OK, what I don't want is strduping it on every PropertyChanged and GetProperties needlessly
Apr 04 20:08:57 <Jeevaka> ok
Apr 04 20:13:51 <Jeevaka> denkenz: new_en_list is used in patch 2 for no sim list. So, It has to be in patch 2
Apr 04 20:14:19 <denkenz> ok fair enough
Apr 04 20:15:00 <Jeevaka> may be i need to use g_strdupv there also. so that it can be freed in set_new_ecc
Apr 04 20:16:05 <denkenz> you have to think a bit whether you want these numbers in new_en_list
Apr 04 20:16:18 <denkenz> e.g. what if there's no SIM but the modem still sends additional numbers
Apr 04 20:20:30 <Jeevaka> hmm. may be i'll call set_new_ecc first and call add_to_en_list with default_en_list_no_sim
Apr 04 20:21:52 <Jeevaka> this way, new_en_list can be moved to patch 3 as well ;)
Apr 04 20:23:16 <denkenz> heh
Apr 04 20:23:44 <denkenz> just gotta think through all the cases
Apr 04 20:24:02 <denkenz> when we had sim and default list it was easy, with 3 sources it gets trickier
Apr 04 20:24:54 <Jeevaka> there is one more issue
Apr 04 20:25:07 <Jeevaka> what if there is no numbers in SIM
Apr 04 20:25:18 <denkenz> Then the default list is it
Apr 04 20:25:23 <denkenz> We already cover this today
Apr 04 20:25:30 <Jeevaka> nope
Apr 04 20:26:01 <Jeevaka> as per the current one, we will have both default_en_list and default_en_list_no_sim
Apr 04 20:26:18 <denkenz> Not last I checked
Apr 04 20:27:14 <denkenz> We still call set_new_ecc even if the list is empty
Apr 04 20:27:53 <Jeevaka> may be with SIM case but not with USIM case
Apr 04 20:28:54 <denkenz> check:
Apr 04 20:28:54 <denkenz>         if (!ok && vc->new_en_list == NULL)
Apr 04 20:28:55 <denkenz>                 return;
Apr 04 20:28:55 <denkenz>         set_new_ecc(vc);
Apr 04 20:29:02 <denkenz> are you sure?
Apr 04 20:29:15 <denkenz> looks just fine to me
Apr 04 20:31:38 <Jeevaka> lets take the case where the EFecc read fails
Apr 04 20:31:56 <denkenz> That is different
Apr 04 20:33:10 <denkenz> The spec says nothing about EFecc failing, and it is a mandatory field in 31.102
Apr 04 20:33:22 <denkenz> So failure to read EFecc is actually a bad thing (TM)
Apr 04 20:34:55 <Jeevaka> agree. so the plan is to do nothing in core if the read fails
Apr 04 20:35:15 <denkenz> Yes, the plan is to keep the no-sim list if that happens
Apr 04 20:36:16 <denkenz> You also need to be careful and keep the change from no-sim -> sim atomic
Apr 04 20:36:23 <denkenz> e.g. only do so when the entire sim list has been read
Apr 04 20:55:24 <Jeevaka> you mean the change from no-sim -> sim atomic? why?
Apr 04 20:57:35 <denkenz> I don't want the nw list to trigger a change from no-sim -> sim numbers
Apr 04 20:57:41 <denkenz> if the sim numbers weren't read fully yet
Apr 04 21:02:35 <Jeevaka> may be i'll just think about all the cases tomorrow. too late :)
Apr 04 21:03:09 <denkenz> nod, no hurry, quality over quantity ;)
Apr 04 21:08:51 <Jeevaka> agreed
**** ENDING LOGGING AT Tue Apr 05 02:59:58 2011