Re: [PATCH] sim: showing lock state with call meter

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Jussi,

>  include/sim.h    |    2 ++
>  src/call-meter.c |   27 +++++++++++++++++++++++++++
>  src/sim.c        |    8 ++++----
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/include/sim.h b/include/sim.h
> index 412ae44..a56056e 100644
> --- a/include/sim.h
> +++ b/include/sim.h
> @@ -227,6 +227,8 @@ unsigned int ofono_sim_add_file_watch(struct ofono_sim_context *context,
>  void ofono_sim_remove_file_watch(struct ofono_sim_context *context,
>  					unsigned int id);
>  
> +void sim_pin_check(struct ofono_sim *sim);
> +

Please name this __ofono_sim_recheck_pin(struct ofono_sim *sim);

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/src/call-meter.c b/src/call-meter.c
> index 0789935..61678d8 100644
> --- a/src/call-meter.c
> +++ b/src/call-meter.c
> @@ -335,11 +335,20 @@ static void set_acm_max_query_callback(const struct ofono_error *error,
>  static void set_acm_max_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_call_meter *cm = data;
> +	struct ofono_atom *sim_atom;
> +	struct ofono_sim *sim = NULL;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBG("Setting acm_max failed");
>  		__ofono_dbus_pending_reply(&cm->pending,
>  					__ofono_error_failed(cm->pending));
> +		sim_atom = __ofono_modem_find_atom(
> +			__ofono_atom_get_modem(cm->atom),
> +				OFONO_ATOM_TYPE_SIM);
> +		if (!sim_atom)

doc/coding-style.txt M1, and M13

> +			return;
> +		sim = __ofono_atom_get_data(sim_atom);

doc/coding-style.txt M1

> +		sim_pin_check(sim);
>  		return;
>  	}
>

And please factor out this code into a separate function, you repeat it
three times.

> @@ -396,11 +405,20 @@ static void set_puct_query_callback(const struct ofono_error *error,
>  static void set_puct_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_call_meter *cm = data;
> +	struct ofono_atom *sim_atom;
> +	struct ofono_sim *sim = NULL;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBG("setting puct failed");
>  		__ofono_dbus_pending_reply(&cm->pending,
>  					__ofono_error_failed(cm->pending));
> +		sim_atom = __ofono_modem_find_atom(
> +			__ofono_atom_get_modem(cm->atom),
> +				OFONO_ATOM_TYPE_SIM);
> +		if (!sim_atom)
> +			return;
> +		sim = __ofono_atom_get_data(sim_atom);
> +		sim_pin_check(sim);
>  		return;
>  	}
>  
> @@ -593,11 +611,20 @@ static void reset_acm_query_callback(const struct ofono_error *error, int value,
>  static void acm_reset_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_call_meter *cm = data;
> +	struct ofono_atom *sim_atom;
> +	struct ofono_sim *sim = NULL;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBG("reseting acm failed");
>  		__ofono_dbus_pending_reply(&cm->pending,
>  					__ofono_error_failed(cm->pending));
> +		sim_atom = __ofono_modem_find_atom(
> +			__ofono_atom_get_modem(cm->atom),
> +				OFONO_ATOM_TYPE_SIM);
> +		if (!sim_atom)
> +			return;
> +		sim = __ofono_atom_get_data(sim_atom);
> +		sim_pin_check(sim);
>  		return;
>  	}
>  
> diff --git a/src/sim.c b/src/sim.c
> index c39269d..08236f2 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -49,7 +49,6 @@
>  static GSList *g_drivers = NULL;
>  
>  static void sim_own_numbers_update(struct ofono_sim *sim);
> -static void sim_pin_check(struct ofono_sim *sim);
>  
>  struct ofono_sim {
>  	/* Contents of the SIM file system, in rough initialization order */
> @@ -2231,8 +2230,9 @@ static void sim_pin_query_cb(const struct ofono_error *error,
>  						&pin_name);
>  	}
>  
> -	if (pin_type != OFONO_SIM_PASSWORD_NONE &&
> -			sim->state == OFONO_SIM_STATE_READY) {
> +	if ((pin_type != OFONO_SIM_PASSWORD_NONE &&
> +			sim->state == OFONO_SIM_STATE_READY) &&
> +				(pin_type != OFONO_SIM_PASSWORD_SIM_PIN2)) {

I don't see how this can work.  You need to check for pin_type != NONE,
PIN2 and PUK2 AND state == READY here.  This also only covers the case
of the PIN2 or PUK2 being triggered when call-meter is active.  You do
not take care of the case where PIN2 or PUK2 are already required during
sim initialization.

>  		/* Force the sim state out of READY */
>  		sim_free_main_state(sim);
>  
> @@ -2247,7 +2247,7 @@ checkdone:
>  		sim_initialize_after_pin(sim);
>  }
>  
> -static void sim_pin_check(struct ofono_sim *sim)
> +void sim_pin_check(struct ofono_sim *sim)
>  {
>  	if (sim->driver->query_passwd_state == NULL) {
>  		sim_initialize_after_pin(sim);

Regards,
-Denis