Re: [PATCH v3 2/4] sim: query remaining pin retries

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Lucas,

On 01/05/2011 11:20 AM, Lucas De Marchi wrote:
> Check the remaining pin retries after each operation that might have
> changed it, i.e. locking, unlocking, reseting or changing pin.
> ---
>  src/sim.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 97 insertions(+), 0 deletions(-)
> 
> diff --git a/src/sim.c b/src/sim.c
> index 335f611..5374da1 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -62,6 +62,8 @@ struct ofono_sim {
>  	enum ofono_sim_password_type pin_type;
>  	gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
>  
> +	int pin_retries[OFONO_SIM_PASSWORD_INVALID];
> +
>  	enum ofono_sim_phase phase;
>  	unsigned char mnc_length;
>  	enum ofono_sim_cphs_phase cphs_phase;
> @@ -248,6 +250,33 @@ static char **get_locked_pins(struct ofono_sim *sim)
>  	return ret;
>  }
>  
> +static void **get_pin_retries(struct ofono_sim *sim)
> +{
> +	int i, nelem;
> +	void **ret;
> +
> +	for (i = 1, nelem = 0; i < OFONO_SIM_PASSWORD_INVALID; i++) {
> +		if (sim->pin_retries[i] == -1)
> +			continue;
> +
> +		nelem+=1;
> +	}
> +
> +	ret = g_new0(void *, nelem * 2 + 1);
> +
> +	nelem = 0;
> +
> +	for (i = 1; i < OFONO_SIM_PASSWORD_INVALID; i++) {
> +		if (sim->pin_retries[i] == -1)
> +			continue;
> +
> +		ret[nelem++] = (void *)(sim_passwd_name(i));

Do you really need parentheses around the sim_passwd_name call?  Also,
you need a space after the cast.  e.g. (void *) sim_passwd_name(i);

> +		ret[nelem++] = &sim->pin_retries[i];
> +	}
> +
> +	return ret;
> +}
> +
>  static char **get_service_numbers(GSList *service_numbers)
>  {
>  	int nelem;
> @@ -287,6 +316,7 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
>  	char **service_numbers;
>  	char **locked_pins;
>  	const char *pin_name;
> +	void **pin_retries;
>  	dbus_bool_t present = sim->state != OFONO_SIM_STATE_NOT_PRESENT;
>  	dbus_bool_t fdn;
>  	dbus_bool_t bdn;
> @@ -369,12 +399,63 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
>  				DBUS_TYPE_STRING,
>  				(void *) &pin_name);
>  
> +	pin_retries = get_pin_retries(sim);
> +

We're not always consistent, but I prefer to logically group operations
together.  So there should be no empty lines between pin_retries,
ofono_dbus_dict_append_dict and g_free.

> +	ofono_dbus_dict_append_dict(&dict, "Retries", DBUS_TYPE_BYTE,
> +								&pin_retries);
> +
> +	g_free(pin_retries);
> +
>  done:
>  	dbus_message_iter_close_container(&iter, &dict);
>  
>  	return reply;
>  }
>  
> +static void sim_pin_retries_query_cb(const struct ofono_error *error,
> +					int retries[OFONO_SIM_PASSWORD_INVALID],
> +					void *data)
> +{
> +	struct ofono_sim *sim = data;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sim->atom);
> +	int i;
> +	void **pin_retries;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		ofono_error("Querying remaining pin retries failed");
> +
> +		return;
> +	}
> +
> +	for (i = 1; i < OFONO_SIM_PASSWORD_INVALID; i++) {
> +		if (retries[i] != sim->pin_retries[i])
> +			break;
> +	}
> +
> +	if (i == OFONO_SIM_PASSWORD_INVALID)
> +		return;
> +

I think the above for and if statements can be easily handled by using
memcmp.

> +	memcpy(sim->pin_retries, retries, sizeof(sim->pin_retries));
> +
> +	pin_retries = get_pin_retries(sim);
> +
> +	ofono_dbus_signal_dict_property_changed(conn, path,
> +					OFONO_SIM_MANAGER_INTERFACE, "Retries",
> +					DBUS_TYPE_BYTE,	&pin_retries);
> +
> +	g_free(pin_retries);

Same comment as above about grouping.

> +}
> +
> +static void sim_pin_retries_check(struct ofono_sim *sim)
> +{
> +	if (sim->driver->query_pin_retries == NULL)
> +		return;
> +
> +	sim->driver->query_pin_retries(sim, sim_pin_retries_query_cb, sim);
> +}
> +
> +
>  static void msisdn_set_done(struct msisdn_set_request *req)
>  {
>  	DBusMessage *reply;
> @@ -549,6 +630,8 @@ static void sim_locked_cb(struct ofono_sim *sim, gboolean locked)
>  						"LockedPins", DBUS_TYPE_STRING,
>  						&locked_pins);
>  	g_strfreev(locked_pins);
> +
> +	sim_pin_retries_check(sim);
>  }
>  
>  static void sim_unlock_cb(const struct ofono_error *error, void *data)
> @@ -557,7 +640,10 @@ static void sim_unlock_cb(const struct ofono_error *error, void *data)
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBusMessage *reply = __ofono_error_failed(sim->pending);
> +
>  		__ofono_dbus_pending_reply(&sim->pending, reply);
> +		sim_pin_retries_check(sim);
> +
>  		return;
>  	}
>  
> @@ -570,7 +656,10 @@ static void sim_lock_cb(const struct ofono_error *error, void *data)
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBusMessage *reply = __ofono_error_failed(sim->pending);
> +
>  		__ofono_dbus_pending_reply(&sim->pending, reply);
> +		sim_pin_retries_check(sim);
> +
>  		return;
>  	}
>  
> @@ -639,11 +728,16 @@ static void sim_change_pin_cb(const struct ofono_error *error, void *data)
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		__ofono_dbus_pending_reply(&sim->pending,
>  				__ofono_error_failed(sim->pending));
> +
> +		sim_pin_retries_check(sim);
> +
>  		return;
>  	}
>  
>  	__ofono_dbus_pending_reply(&sim->pending,
>  				dbus_message_new_method_return(sim->pending));
> +
> +	sim_pin_retries_check(sim);
>  }
>  
>  static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,
> @@ -1594,6 +1688,8 @@ static void sim_pin_query_cb(const struct ofono_error *error,
>  						&pin_name);
>  	}
>  
> +	sim_pin_retries_check(sim);
> +
>  checkdone:
>  	if (pin_type == OFONO_SIM_PASSWORD_NONE)
>  		sim_initialize_after_pin(sim);
> @@ -2196,6 +2292,7 @@ struct ofono_sim *ofono_sim_create(struct ofono_modem *modem,
>  	sim->phase = OFONO_SIM_PHASE_UNKNOWN;
>  	sim->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_SIM,
>  						sim_remove, sim);
> +	memset(sim->pin_retries, -1, sizeof(sim->pin_retries));
>  
>  	for (l = g_drivers; l; l = l->next) {
>  		const struct ofono_sim_driver *drv = l->data;

Regards,
-Denis