RE: [PATCH] sim: check if lock is locked after code changing attempt

RE: [PATCH] sim: check if lock is locked after code changing attempt

[Jussi.Kangas]

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

Hi Lucas,

On Tue, 2011-01-11 at 16:08 +0200, Lucas De Marchi wrote:
> Hi Jussi
> 
> On Tue, Jan 11, 2011 at 10:17 AM,  <Jussi.Kangas@tieto.com> wrote:
> > Hi,
> >
> > This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well.
> >
> 
> I failed to understand why you are hard-coding this and why you'd like
> to use retry counters. Can't you just check the pin state by calling
> sim_pin_check()?
> 
> Moreover, this is not really implementing what the TODO item says.
> 
> 
> regards,
> Lucas De Marchi

Marit already proposed sim_pin_check approach 2010/11/2. Denis Kenzior
thought that initializing sim interface would not be a good idea. See
mail "Re: [Patch] sim: Check SIM pin status after changing pin" from
Friday 5th in November 2010. 

True, fix proposal does not do exactly what TODO item says. It trusts to
returned error value instead of going to check the sim state once more.
In my testings this value has been trustable so far and there has not
been reason to go check the state. Of course I've tested only with one
modem so this might not be a universal truth.

I don't particularly wish to use the retry counters. I was just thinking that same
information ( PUK required ) could be deduced by counting the retries. 

Br,
-Jussi

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Lucas De Marchi]

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

Hi Jussi,

On Wed, Jan 12, 2011 at 5:39 AM,  <Jussi.Kangas@tieto.com> wrote:
> Hi Lucas,
>
> On Tue, 2011-01-11 at 16:08 +0200, Lucas De Marchi wrote:
>> Hi Jussi
>>
>> On Tue, Jan 11, 2011 at 10:17 AM,  <Jussi.Kangas@tieto.com> wrote:
>> > Hi,
>> >
>> > This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well.
>> >
>>
>> I failed to understand why you are hard-coding this and why you'd like
>> to use retry counters. Can't you just check the pin state by calling
>> sim_pin_check()?
>>
>> Moreover, this is not really implementing what the TODO item says.
>>
>>
>> regards,
>> Lucas De Marchi
>
> Marit already proposed sim_pin_check approach 2010/11/2. Denis Kenzior
> thought that initializing sim interface would not be a good idea. See
> mail "Re: [Patch] sim: Check SIM pin status after changing pin" from
> Friday 5th in November 2010.
>
> True, fix proposal does not do exactly what TODO item says. It trusts to
> returned error value instead of going to check the sim state once more.
> In my testings this value has been trustable so far and there has not
> been reason to go check the state. Of course I've tested only with one
> modem so this might not be a universal truth.
>
> I don't particularly wish to use the retry counters. I was just thinking that same
> information ( PUK required ) could be deduced by counting the retries.

IMO you should try querying the pin state on failure as Denis and I
suggested rather than using retry counters and/or hard-coding this
here. You might do this without re-initializing the sim interface.
Then Denis' worries in
http://www.mail-archive.com/ofono(a)ofono.org/msg05294.html are gone.



Lucas De Marchi

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Denis Kenzior]

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

Hi Jussi,

> I don't particularly wish to use the retry counters. I was just thinking that same
> information ( PUK required ) could be deduced by counting the retries. 

To my knowledge you cannot.  Correct me if I'm wrong, but the retry
counters are stored on the SIM and the SIM can be removed, and later
re-inserted.  You would not know how the counters have been affected.

Regards,
-Denis

[PATCH] sim: check if lock is locked after code changing attempt

[Jussi.Kangas]

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

Hi,

This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well. 

Br,
-Jussi

---
 src/sim.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index d627647..789ddde 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -712,8 +712,60 @@ static DBusMessage *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,
 static void sim_change_pin_cb(const struct ofono_error *error, void *data)
 {
     struct ofono_sim *sim = data;
+    DBusConnection *conn = ofono_dbus_get_connection();
+    const char *path = __ofono_atom_get_path(sim->atom);
+    struct ofono_modem *modem = __ofono_atom_get_modem(sim->atom);
+    const char *pin_name;
 
     if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+        if (error->error == 12) {
+            sim->locked_pins[sim->pin_type] = TRUE;
+            switch (sim->pin_type) {
+            case OFONO_SIM_PASSWORD_SIM_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_SIM_PUK);
+                break;
+            case OFONO_SIM_PASSWORD_PHFSIM_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_PHFSIM_PUK;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_PHFSIM_PUK);
+                break;
+            case OFONO_SIM_PASSWORD_PHCORP_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_PHCORP_PUK;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_PHCORP_PUK);
+                break;
+            case OFONO_SIM_PASSWORD_PHNET_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_PHNET_PUK;
+                pin_name = sim_passwd_name(
+                        OFONO_SIM_PASSWORD_PHNET_PUK);
+            case OFONO_SIM_PASSWORD_PHNETSUB_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_PHNETSUB_PUK;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_PHNETSUB_PUK);
+                break;
+            case OFONO_SIM_PASSWORD_PHSP_PIN:
+                sim->pin_type = OFONO_SIM_PASSWORD_PHSP_PUK;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_PHSP_PUK);
+                break;
+            case OFONO_SIM_PASSWORD_SIM_PIN2:
+                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK2;
+                pin_name = sim_passwd_name(
+                    OFONO_SIM_PASSWORD_SIM_PUK2);
+                break;
+            default:
+                break;
+            }
+            ofono_dbus_signal_property_changed(conn, path,
+                OFONO_SIM_MANAGER_INTERFACE,
+                "PinRequired", DBUS_TYPE_STRING,
+                &pin_name);
+
+            if (sim->pin_type != OFONO_SIM_PASSWORD_SIM_PUK2)
+                ofono_modem_reset(modem);
+        }
         __ofono_dbus_pending_reply(&sim->pending,
                 __ofono_error_failed(sim->pending));
 
@@ -722,6 +774,7 @@ static void sim_change_pin_cb(const struct ofono_error *error, void *data)
         return;
     }
 
+    sim->pin_type = OFONO_SIM_PASSWORD_NONE;
     __ofono_dbus_pending_reply(&sim->pending,
                 dbus_message_new_method_return(sim->pending));
 
@@ -764,6 +817,7 @@ static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,
         return dbus_message_new_method_return(msg);
 
     sim->pending = dbus_message_ref(msg);
+    sim->pin_type = type;
     sim->driver->change_passwd(sim, type, old, new,
                     sim_change_pin_cb, sim);
 
-- 
1.7.1

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Lucas De Marchi]

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

Hi Jussi

On Tue, Jan 11, 2011 at 10:17 AM,  <Jussi.Kangas@tieto.com> wrote:
> Hi,
>
> This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well.
>

I failed to understand why you are hard-coding this and why you'd like
to use retry counters. Can't you just check the pin state by calling
sim_pin_check()?

Moreover, this is not really implementing what the TODO item says.


regards,
Lucas De Marchi

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Denis Kenzior]

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

Hi Jussi,

On 01/11/2011 06:17 AM, Jussi.Kangas(a)tieto.com wrote:
> Hi,
> 
> This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well. 
> 
> Br,
> -Jussi
> 
> ---
>  src/sim.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)
> 

First, please fix your authorship information.  See the AUTHORS file to
see what we expect.  Also, please make sure that you're using tabs for
indentation and following the relevant coding style.  See the Submitting
Patches section in the HACKING document in oFono git as well as
doc/coding-style.txt.

> diff --git a/src/sim.c b/src/sim.c
> index d627647..789ddde 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -712,8 +712,60 @@ static DBusMessage *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,
>  static void sim_change_pin_cb(const struct ofono_error *error, void *data)
>  {

So my first concern is that whatever you do here also has to work for
LockPin and UnlockPin.

>      struct ofono_sim *sim = data;
> +    DBusConnection *conn = ofono_dbus_get_connection();
> +    const char *path = __ofono_atom_get_path(sim->atom);
> +    struct ofono_modem *modem = __ofono_atom_get_modem(sim->atom);
> +    const char *pin_name;
>  
>      if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +        if (error->error == 12) {
> +            sim->locked_pins[sim->pin_type] = TRUE;
> +            switch (sim->pin_type) {
> +            case OFONO_SIM_PASSWORD_SIM_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_SIM_PUK);
> +                break;
> +            case OFONO_SIM_PASSWORD_PHFSIM_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_PHFSIM_PUK;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_PHFSIM_PUK);
> +                break;
> +            case OFONO_SIM_PASSWORD_PHCORP_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_PHCORP_PUK;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_PHCORP_PUK);
> +                break;
> +            case OFONO_SIM_PASSWORD_PHNET_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_PHNET_PUK;
> +                pin_name = sim_passwd_name(
> +                        OFONO_SIM_PASSWORD_PHNET_PUK);
> +            case OFONO_SIM_PASSWORD_PHNETSUB_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_PHNETSUB_PUK;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_PHNETSUB_PUK);
> +                break;
> +            case OFONO_SIM_PASSWORD_PHSP_PIN:
> +                sim->pin_type = OFONO_SIM_PASSWORD_PHSP_PUK;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_PHSP_PUK);
> +                break;
> +            case OFONO_SIM_PASSWORD_SIM_PIN2:
> +                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK2;
> +                pin_name = sim_passwd_name(
> +                    OFONO_SIM_PASSWORD_SIM_PUK2);
> +                break;
> +            default:
> +                break;
> +            }
> +            ofono_dbus_signal_property_changed(conn, path,
> +                OFONO_SIM_MANAGER_INTERFACE,
> +                "PinRequired", DBUS_TYPE_STRING,
> +                &pin_name);

Have you considered querying the PIN state on a failure instead?  If the
lock/unlock/change pin operation fails, then re-query the current PIN.
If the CPIN is no longer returning READY and we're in a state
OFONO_SIM_STATE_READY, then tear us back down to an earlier state.

> +
> +            if (sim->pin_type != OFONO_SIM_PASSWORD_SIM_PUK2)

What is the reason for excepting PUK2 here?  Should we be also
proceeding to the SIM_READY state on SIM initialization if PUK2 is
required as well?

> +                ofono_modem_reset(modem);
> +        }

This seems a bit drastic, but fair enough...

>          __ofono_dbus_pending_reply(&sim->pending,
>                  __ofono_error_failed(sim->pending));
>  
> @@ -722,6 +774,7 @@ static void sim_change_pin_cb(const struct ofono_error *error, void *data)
>          return;
>      }
>  
> +    sim->pin_type = OFONO_SIM_PASSWORD_NONE;
>      __ofono_dbus_pending_reply(&sim->pending,
>                  dbus_message_new_method_return(sim->pending));
>  
> @@ -764,6 +817,7 @@ static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,
>          return dbus_message_new_method_return(msg);
>  
>      sim->pending = dbus_message_ref(msg);
> +    sim->pin_type = type;
>      sim->driver->change_passwd(sim, type, old, new,
>                      sim_change_pin_cb, sim);
>  

Regards,
-Denis

RE: [PATCH] sim: check if lock is locked after code changing attempt

[Jussi.Kangas]

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

Hi Denis, 

On Wed, 2011-01-12 at 07:07 +0200, Denis Kenzior wrote:
> Hi Jussi,
> 
> On 01/11/2011 06:17 AM, Jussi.Kangas(a)tieto.com wrote:
> > Hi,
> > 
> > This is fix to Marit Henriksen's TODO item "Check SIM pin status if sim_change_pin fails". I've discussed with Marit and it's ok for her if I fix the issue. Problem here is that issue could perhaps also be fixed with retry counter solution introduced by Lucas De Marchi couple of tasks ago. That would seem however require some extra implementation in ste modem ( at least I was not able get the ofono show correct values without extra modifications ) and I think that isimodems don't have that sort of retry counter at all. Because of that and since I had this solution already implemented I propose it to be added as well. 
> > 
> > Br,
> > -Jussi
> > 
> > ---
> >  src/sim.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 54 insertions(+), 0 deletions(-)
> > 
> 
> First, please fix your authorship information.  See the AUTHORS file 
> to see what we expect.  Also, please make sure that you're using tabs 
> for indentation and following the relevant coding style.  See the 
> Submitting Patches section in the HACKING document in oFono git as 
> well as doc/coding-style.txt.

I think whole this chapter should be translated as "Do not use Outlook".
I asked around little bit and best guess seems to be that you are using something that takes authorship information from mail address. Also since the patch passes the checkpatch here with no problems whatsoever I guess Outlook ruined the patch. Fine, I'll try Evolution next. If possible, I would like to avoid using the git send-email. I prefer tools with GUI. 

> 
> > diff --git a/src/sim.c b/src/sim.c
> > index d627647..789ddde 100644
> > --- a/src/sim.c
> > +++ b/src/sim.c
> > @@ -712,8 +712,60 @@ static DBusMessage 
> > *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,  static void 
> > sim_change_pin_cb(const struct ofono_error *error, void *data)  {
> 
> So my first concern is that whatever you do here also has to work for 
> LockPin and UnlockPin.

I'm not after total solution here. I would like to implement this so that first the ChangePin starts working, i.e fullfill the existing TODO task first. Reasoning here is that I thought it would be easier to get the fixes in if I keep them small. If u mean that stuff inside (error->error = 12) condition should be as separate function to be usable for LockPin and UnlockPin, I agree. I was just thinking that separation could be done when fix is extended to LockPin and UnlockPin. 

> 
> >      struct ofono_sim *sim = data;
> > +    DBusConnection *conn = ofono_dbus_get_connection();
> > +    const char *path = __ofono_atom_get_path(sim->atom);
> > +    struct ofono_modem *modem = __ofono_atom_get_modem(sim->atom);
> > +    const char *pin_name;
> >  
> >      if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > +        if (error->error == 12) {
> > +            sim->locked_pins[sim->pin_type] = TRUE;
> > +            switch (sim->pin_type) {
> > +            case OFONO_SIM_PASSWORD_SIM_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_SIM_PUK);
> > +                break;
> > +            case OFONO_SIM_PASSWORD_PHFSIM_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_PHFSIM_PUK;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_PHFSIM_PUK);
> > +                break;
> > +            case OFONO_SIM_PASSWORD_PHCORP_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_PHCORP_PUK;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_PHCORP_PUK);
> > +                break;
> > +            case OFONO_SIM_PASSWORD_PHNET_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_PHNET_PUK;
> > +                pin_name = sim_passwd_name(
> > +                        OFONO_SIM_PASSWORD_PHNET_PUK);
> > +            case OFONO_SIM_PASSWORD_PHNETSUB_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_PHNETSUB_PUK;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_PHNETSUB_PUK);
> > +                break;
> > +            case OFONO_SIM_PASSWORD_PHSP_PIN:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_PHSP_PUK;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_PHSP_PUK);
> > +                break;
> > +            case OFONO_SIM_PASSWORD_SIM_PIN2:
> > +                sim->pin_type = OFONO_SIM_PASSWORD_SIM_PUK2;
> > +                pin_name = sim_passwd_name(
> > +                    OFONO_SIM_PASSWORD_SIM_PUK2);
> > +                break;
> > +            default:
> > +                break;
> > +            }
> > +            ofono_dbus_signal_property_changed(conn, path,
> > +                OFONO_SIM_MANAGER_INTERFACE,
> > +                "PinRequired", DBUS_TYPE_STRING,
> > +                &pin_name);
> 
> Have you considered querying the PIN state on a failure instead?  If 
> the lock/unlock/change pin operation fails, then re-query the current PIN.
> If the CPIN is no longer returning READY and we're in a state 
> OFONO_SIM_STATE_READY, then tear us back down to an earlier state.

Yes, I have. As I answered to Lucas already at least with the modem I'm using the return value I get seems to be trustable and there is no point to go querying the information I already know. Also there could be other reasons why sim state is not ready ( small chance of course ) so to satisfy my pedant nature I would have to either check the return value anyways or use some other means to get the reason why the sim is not ready in order to be 100 % sure that PUK is required. At least Lucas seemed to favor query approach. I guess that has to mean that return value is not trustable with all modems. I can change this if required. 

> 
> > +
> > +            if (sim->pin_type != OFONO_SIM_PASSWORD_SIM_PUK2)
> 
> What is the reason for excepting PUK2 here?  Should we be also 
> proceeding to the SIM_READY state on SIM initialization if PUK2 is 
> required as well?

PUK2 locking does not force modem to drop from network. There is no reason to go pre_sim state. In matter of fact I'm using SIM with PIN2 blocked here all the time since I don't know how to get the code. 

> 
> > +                ofono_modem_reset(modem);
> > +        }
> 
> This seems a bit drastic, but fair enough...
> 
> >          __ofono_dbus_pending_reply(&sim->pending,
> >                  __ofono_error_failed(sim->pending));
> >  
> > @@ -722,6 +774,7 @@ static void sim_change_pin_cb(const struct ofono_error *error, void *data)
> >          return;
> >      }
> >  
> > +    sim->pin_type = OFONO_SIM_PASSWORD_NONE;
> >      __ofono_dbus_pending_reply(&sim->pending,
> >                  dbus_message_new_method_return(sim->pending));
> >  
> > @@ -764,6 +817,7 @@ static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,
> >          return dbus_message_new_method_return(msg);
> >  
> >      sim->pending = dbus_message_ref(msg);
> > +    sim->pin_type = type;
> >      sim->driver->change_passwd(sim, type, old, new,
> >                      sim_change_pin_cb, sim);
> >  
> 
> Regards,
> -Denis

Br,
-Jussi

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Lucas De Marchi]

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

Ho Jussi

On Thu, Jan 13, 2011 at 6:34 AM,  <Jussi.Kangas@tieto.com> wrote:
> On Wed, 2011-01-12 at 07:07 +0200, Denis Kenzior wrote:
>> First, please fix your authorship information.  See the AUTHORS file
>> to see what we expect.  Also, please make sure that you're using tabs
>> for indentation and following the relevant coding style.  See the
>> Submitting Patches section in the HACKING document in oFono git as
>> well as doc/coding-style.txt.
>
> I think whole this chapter should be translated as "Do not use Outlook".
> I asked around little bit and best guess seems to be that you are using something that takes authorship information from mail

This is `git am'

> address. Also since the patch passes the checkpatch here with no problems whatsoever I guess Outlook ruined the patch. Fine, > I'll try Evolution next. If possible, I would like to avoid using the git send-email. I prefer tools with GUI.

You can use whatever client you want to write email. But for sending
patches, git send-email is the best one since you won't need to copy
and paste, nor attach anything. Give it a try and you'll regret not
using it first.



Lucas De Marchi

Re: [PATCH] sim: check if lock is locked after code changing attempt

[Denis Kenzior]

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

Hi Jussi,

>>
>> First, please fix your authorship information.  See the AUTHORS file 
>> to see what we expect.  Also, please make sure that you're using tabs 
>> for indentation and following the relevant coding style.  See the 
>> Submitting Patches section in the HACKING document in oFono git as 
>> well as doc/coding-style.txt.
> 
> I think whole this chapter should be translated as "Do not use Outlook".
> I asked around little bit and best guess seems to be that you are using something that takes authorship information from mail address. Also since the patch passes the checkpatch here with no problems whatsoever I guess Outlook ruined the patch. Fine, I'll try Evolution next. If possible, I would like to avoid using the git send-email. I prefer tools with GUI. 

git-send-email is the only sane approach.  Save yourself some time and
just use it.  Or find a GUI tool to do it for you.

> 
>>
>>> diff --git a/src/sim.c b/src/sim.c
>>> index d627647..789ddde 100644
>>> --- a/src/sim.c
>>> +++ b/src/sim.c
>>> @@ -712,8 +712,60 @@ static DBusMessage 
>>> *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,  static void 
>>> sim_change_pin_cb(const struct ofono_error *error, void *data)  {
>>
>> So my first concern is that whatever you do here also has to work for 
>> LockPin and UnlockPin.
> 
> I'm not after total solution here. I would like to implement this so that first the ChangePin starts working, i.e fullfill the existing TODO task first. Reasoning here is that I thought it would be easier to get the fixes in if I keep them small. If u mean that stuff inside (error->error = 12) condition should be as separate function to be usable for LockPin and UnlockPin, I agree. I was just thinking that separation could be done when fix is extended to LockPin and UnlockPin. 
> 

Whatever you do here applies to LockPin and UnlockPin, so you might as
well handle them too.

>> Have you considered querying the PIN state on a failure instead?  If 
>> the lock/unlock/change pin operation fails, then re-query the current PIN.
>> If the CPIN is no longer returning READY and we're in a state 
>> OFONO_SIM_STATE_READY, then tear us back down to an earlier state.
> 
> Yes, I have. As I answered to Lucas already at least with the modem I'm using the return value I get seems to be trustable and there is no point to go querying the information I already know. Also there could be other reasons why sim state is not ready ( small chance of course ) so to satisfy my pedant nature I would have to either check the return value anyways or use some other means to get the reason why the sim is not ready in order to be 100 % sure that PUK is required. At least Lucas seemed to favor query approach. I guess that has to mean that return value is not trustable with all modems. I can change this if required. 
> 
>>

In general you cannot trust the return value.  Most modems simply get
this wrong.  On a well setup modem driver you do not need to worry about
sim readiness conditions.  The driver should take care of that.  If the
modem firmware is buggy, well there's no strategy that will save you anyway.

So I'd prefer if we try out the query approach first.

Regards,
-Denis