[PATCH] Query locked pins after reset pin

[PATCH] Query locked pins after reset pin

[caiwen.zhang]

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

From: Caiwen Zhang <caiwen.zhang@intel.com>

---
 src/sim.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/src/sim.c b/src/sim.c
index edae5eb..ef55b1c 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -123,6 +123,11 @@ struct msisdn_set_request {
 	DBusMessage *msg;
 };
 
+struct sim_locked_query_request {
+	struct ofono_sim *sim;
+	enum ofono_sim_password_type type;
+};
+
 struct service_number {
 	char *id;
 	struct ofono_phone_number ph;
@@ -610,6 +615,57 @@ error:
 	return __ofono_error_invalid_args(msg);
 }
 
+void sim_query_locked_cb(const struct ofono_error *error,
+			int locked, void *data)
+{
+	struct sim_locked_query_request *req = data;
+	struct ofono_sim *sim = req->sim;
+	char **locked_pins;
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(sim->atom);
+
+	DBG("");
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		g_free(req);
+		return;
+	}
+
+	if (req->type >= OFONO_SIM_PASSWORD_SIM_PUK) {
+		g_free(req);
+		return;
+	}
+
+	sim->locked_pins[req->type] = locked;
+	DBG("sim_query_locked_cb pin_type: %d %d", req->type, locked);
+
+	locked_pins = get_locked_pins(sim);
+	ofono_dbus_signal_array_property_changed(conn, path,
+						OFONO_SIM_MANAGER_INTERFACE,
+						"LockedPins", DBUS_TYPE_STRING,
+						&locked_pins);
+	g_strfreev(locked_pins);
+	g_free(req);
+}
+
+static void __sim_query_locked(struct ofono_sim *sim,
+			enum ofono_sim_password_type type)
+{
+	struct sim_locked_query_request *req;
+
+	if (type == OFONO_SIM_PASSWORD_NONE 
+		|| type > OFONO_SIM_PASSWORD_PHCORP_PIN)
+		return;
+
+	req = g_new0(struct sim_locked_query_request, 1);
+	req->sim = sim;
+	req->type = type;
+		
+	if (sim->driver->query_locked)
+		sim->driver->query_locked(sim, type, sim_query_locked_cb, req);
+}
+
+
 static void sim_locked_cb(struct ofono_sim *sim, gboolean locked)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -1039,6 +1095,25 @@ static DBusMessage *sim_get_icon(DBusConnection *conn,
 	return NULL;
 }
 
+static void sim_reset_pin_cb(const struct ofono_error *error, void *data)
+{
+	struct ofono_sim *sim = data;
+	DBusMessage *reply;
+	enum ofono_sim_password_type type;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+		reply = __ofono_error_failed(sim->pending);
+	else
+		reply = dbus_message_new_method_return(sim->pending);
+
+	__ofono_dbus_pending_reply(&sim->pending, reply);
+
+	__ofono_sim_recheck_pin(sim);
+
+	for (type = OFONO_SIM_PASSWORD_SIM_PIN; type < OFONO_SIM_PASSWORD_SIM_PUK; type++)
+		__sim_query_locked(sim, type);
+}
+
 static DBusMessage *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
 					void *data)
 {
@@ -1074,7 +1149,7 @@ static DBusMessage *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
 		return __ofono_error_invalid_format(msg);
 
 	sim->pending = dbus_message_ref(msg);
-	sim->driver->reset_passwd(sim, puk, pin, sim_enter_pin_cb, sim);
+	sim->driver->reset_passwd(sim, puk, pin, sim_reset_pin_cb, sim);
 
 	return NULL;
 }
-- 
1.7.9.5

Re: [PATCH] Query locked pins after reset pin

[Denis Kenzior]

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

Hi Caiwen,

On 09/11/2013 02:13 PM, caiwen.zhang(a)intel.com wrote:
> From: Caiwen Zhang <caiwen.zhang@intel.com>
>
> ---
>   src/sim.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/src/sim.c b/src/sim.c
> index edae5eb..ef55b1c 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -123,6 +123,11 @@ struct msisdn_set_request {
>   	DBusMessage *msg;
>   };
>
> +struct sim_locked_query_request {
> +	struct ofono_sim *sim;
> +	enum ofono_sim_password_type type;
> +};
> +
>   struct service_number {
>   	char *id;
>   	struct ofono_phone_number ph;
> @@ -610,6 +615,57 @@ error:
>   	return __ofono_error_invalid_args(msg);
>   }
>
> +void sim_query_locked_cb(const struct ofono_error *error,
> +			int locked, void *data)
> +{
> +	struct sim_locked_query_request *req = data;
> +	struct ofono_sim *sim = req->sim;
> +	char **locked_pins;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sim->atom);
> +
> +	DBG("");
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		g_free(req);
> +		return;
> +	}
> +
> +	if (req->type >= OFONO_SIM_PASSWORD_SIM_PUK) {
> +		g_free(req);
> +		return;

This check seems to be pointless

> +	}
> +
> +	sim->locked_pins[req->type] = locked;
> +	DBG("sim_query_locked_cb pin_type: %d %d", req->type, locked);
> +
> +	locked_pins = get_locked_pins(sim);
> +	ofono_dbus_signal_array_property_changed(conn, path,
> +						OFONO_SIM_MANAGER_INTERFACE,
> +						"LockedPins", DBUS_TYPE_STRING,
> +						&locked_pins);
> +	g_strfreev(locked_pins);
> +	g_free(req);
> +}
> +
> +static void __sim_query_locked(struct ofono_sim *sim,
> +			enum ofono_sim_password_type type)

Why is this function prefixed by '__'?  The '__ofono' prefix is reserved 
for private APIs (e.g. functions that are not static, but not part of 
include/* public APIs).

> +{
> +	struct sim_locked_query_request *req;
> +
> +	if (type == OFONO_SIM_PASSWORD_NONE
> +		|| type > OFONO_SIM_PASSWORD_PHCORP_PIN)
> +		return;

This does not comply with our coding style guidelines.  Also, why do you 
bother checking type here since it is always guaranteed to be only 
OFONO_SIM_PASSWORD_SIM_PIN?

> +
> +	req = g_new0(struct sim_locked_query_request, 1);
> +	req->sim = sim;
> +	req->type = type;

This is wrong, see below:

> +		
> +	if (sim->driver->query_locked)
> +		sim->driver->query_locked(sim, type, sim_query_locked_cb, req);

The oFono driver calling semantics do not allow for a cleanup handler. 
So if the modem hardware crashes or is removed in the time between you 
submit the request to the driver and the driver returns with the result, 
you will incur a memory leak.

> +}
> +
> +
>   static void sim_locked_cb(struct ofono_sim *sim, gboolean locked)
>   {
>   	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -1039,6 +1095,25 @@ static DBusMessage *sim_get_icon(DBusConnection *conn,
>   	return NULL;
>   }
>
> +static void sim_reset_pin_cb(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_sim *sim = data;
> +	DBusMessage *reply;
> +	enum ofono_sim_password_type type;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> +		reply = __ofono_error_failed(sim->pending);
> +	else
> +		reply = dbus_message_new_method_return(sim->pending);
> +
> +	__ofono_dbus_pending_reply(&sim->pending, reply);
> +
> +	__ofono_sim_recheck_pin(sim);
> +
> +	for (type = OFONO_SIM_PASSWORD_SIM_PIN; type < OFONO_SIM_PASSWORD_SIM_PUK; type++)
> +		__sim_query_locked(sim, type);

Why do you need this in the first place.  Are you suggesting that by 
unblocking the SIM PIN it somehow has the side-effect of also resetting 
the SIM lock status?

> +}
> +
>   static DBusMessage *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
>   					void *data)
>   {
> @@ -1074,7 +1149,7 @@ static DBusMessage *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
>   		return __ofono_error_invalid_format(msg);
>
>   	sim->pending = dbus_message_ref(msg);
> -	sim->driver->reset_passwd(sim, puk, pin, sim_enter_pin_cb, sim);
> +	sim->driver->reset_passwd(sim, puk, pin, sim_reset_pin_cb, sim);
>
>   	return NULL;
>   }
>

Regards,
-Denis

RE: [PATCH] Query locked pins after reset pin

[Zhang, Caiwen]

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

Hi Denis,

> > ---
> >   src/sim.c |   77
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/sim.c b/src/sim.c
> > index edae5eb..ef55b1c 100644
> > --- a/src/sim.c
> > +++ b/src/sim.c
> > @@ -123,6 +123,11 @@ struct msisdn_set_request {
> >   	DBusMessage *msg;
> >   };
> >
> > +struct sim_locked_query_request {
> > +	struct ofono_sim *sim;
> > +	enum ofono_sim_password_type type;
> > +};
> > +
> >   struct service_number {
> >   	char *id;
> >   	struct ofono_phone_number ph;
> > @@ -610,6 +615,57 @@ error:
> >   	return __ofono_error_invalid_args(msg);
> >   }
> >
> > +void sim_query_locked_cb(const struct ofono_error *error,
> > +			int locked, void *data)
> > +{
> > +	struct sim_locked_query_request *req = data;
> > +	struct ofono_sim *sim = req->sim;
> > +	char **locked_pins;
> > +	DBusConnection *conn = ofono_dbus_get_connection();
> > +	const char *path = __ofono_atom_get_path(sim->atom);
> > +
> > +	DBG("");
> > +
> > +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > +		g_free(req);
> > +		return;
> > +	}
> > +
> > +	if (req->type >= OFONO_SIM_PASSWORD_SIM_PUK) {
> > +		g_free(req);
> > +		return;
> 
> This check seems to be pointless
> 
> > +	}
> > +
> > +	sim->locked_pins[req->type] = locked;
> > +	DBG("sim_query_locked_cb pin_type: %d %d", req->type, locked);
> > +
> > +	locked_pins = get_locked_pins(sim);
> > +	ofono_dbus_signal_array_property_changed(conn, path,
> > +						OFONO_SIM_MANAGER_INTERFACE,
> > +						"LockedPins", DBUS_TYPE_STRING,
> > +						&locked_pins);
> > +	g_strfreev(locked_pins);
> > +	g_free(req);
> > +}
> > +
> > +static void __sim_query_locked(struct ofono_sim *sim,
> > +			enum ofono_sim_password_type type)
> 
> Why is this function prefixed by '__'?  The '__ofono' prefix is reserved for
> private APIs (e.g. functions that are not static, but not part of
> include/* public APIs).

Got it. I will change it.
> 
> > +{
> > +	struct sim_locked_query_request *req;
> > +
> > +	if (type == OFONO_SIM_PASSWORD_NONE
> > +		|| type > OFONO_SIM_PASSWORD_PHCORP_PIN)
> > +		return;
> 
> This does not comply with our coding style guidelines.  Also, why do you
> bother checking type here since it is always guaranteed to be only
> OFONO_SIM_PASSWORD_SIM_PIN?
> 
Yes. only value between OFONO_SIM_PASSWORD_SIM_PIN and OFONO_SIM_PASSWORD_PHCORP_PIN is valid.
Other PUK relative value is invalid.

> > +
> > +	req = g_new0(struct sim_locked_query_request, 1);
> > +	req->sim = sim;
> > +	req->type = type;
> 
> This is wrong, see below:
> 
> > +
> > +	if (sim->driver->query_locked)
> > +		sim->driver->query_locked(sim, type, sim_query_locked_cb, req);
> 
> The oFono driver calling semantics do not allow for a cleanup handler.
> So if the modem hardware crashes or is removed in the time between you
> submit the request to the driver and the driver returns with the result, you will
> incur a memory leak.
> 
In any case, the callback (sim_query_locked_cb) will be invoked. If only the callback is called,
"req" will be freed. If I am wrong, please correct me.

> > +}
> > +
> > +
> >   static void sim_locked_cb(struct ofono_sim *sim, gboolean locked)
> >   {
> >   	DBusConnection *conn = ofono_dbus_get_connection(); @@ -1039,6
> > +1095,25 @@ static DBusMessage *sim_get_icon(DBusConnection *conn,
> >   	return NULL;
> >   }
> >
> > +static void sim_reset_pin_cb(const struct ofono_error *error, void
> > +*data) {
> > +	struct ofono_sim *sim = data;
> > +	DBusMessage *reply;
> > +	enum ofono_sim_password_type type;
> > +
> > +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> > +		reply = __ofono_error_failed(sim->pending);
> > +	else
> > +		reply = dbus_message_new_method_return(sim->pending);
> > +
> > +	__ofono_dbus_pending_reply(&sim->pending, reply);
> > +
> > +	__ofono_sim_recheck_pin(sim);
> > +
> > +	for (type = OFONO_SIM_PASSWORD_SIM_PIN; type <
> OFONO_SIM_PASSWORD_SIM_PUK; type++)
> > +		__sim_query_locked(sim, type);
> 
> Why do you need this in the first place.  Are you suggesting that by unblocking
> the SIM PIN it somehow has the side-effect of also resetting the SIM lock
> status?
> 
It is true for IMC modem. After reset SIM PIN, the SIM PIN locking is changed. I doubt this is an issue of
IMC modem. Following is the log:

> AT+CLCK="SC",2
< \r\n+CLCK: 0\r\n
< \r\nOK\r\n

> AT+CLCK="SC",1,"1111"
< \r\nERROR\r\n

> AT+CMEE=1
< \r\nOK\r\n

> AT+CLCK="SC",1,"1111"
< \r\n+CME ERROR: 16\r\n

> AT+CLCK="SC",1,"1111"
< \r\n+CME ERROR: 12\r\n

> AT+CPIN="13570794","1234"
< \r\nOK\r\n

> AT+CLCK="SC",2
< \r\n+CLCK: 1\r\n
< \r\nOK\r\n

To be safe, query SIM PIN locked information after reset sim pin.

> > +}
> > +
> >   static DBusMessage *sim_reset_pin(DBusConnection *conn,
> DBusMessage *msg,
> >   					void *data)
> >   {
> > @@ -1074,7 +1149,7 @@ static DBusMessage
> *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
> >   		return __ofono_error_invalid_format(msg);
> >
> >   	sim->pending = dbus_message_ref(msg);
> > -	sim->driver->reset_passwd(sim, puk, pin, sim_enter_pin_cb, sim);
> > +	sim->driver->reset_passwd(sim, puk, pin, sim_reset_pin_cb, sim);
> >
> >   	return NULL;
> >   }
> >
> 
Regards,
Caiwen

Re: [PATCH] Query locked pins after reset pin

[Denis Kenzior]

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

Hi Caiwen,

 >>> +
>>> +	if (sim->driver->query_locked)
>>> +		sim->driver->query_locked(sim, type, sim_query_locked_cb, req);
>>
>> The oFono driver calling semantics do not allow for a cleanup handler.
>> So if the modem hardware crashes or is removed in the time between you
>> submit the request to the driver and the driver returns with the result, you will
>> incur a memory leak.
>>
> In any case, the callback (sim_query_locked_cb) will be invoked. If only the callback is called,
> "req" will be freed. If I am wrong, please correct me.

Please re-read my comment above.  The callback is not guaranteed to be 
always called.  If the atom is removed for any reason between the time 
the driver request is submitted and the callback is called, you have a 
memory leak.  For example, if the modem resets itself or crashes or the 
SIM is removed.

>>
>> Why do you need this in the first place.  Are you suggesting that by unblocking
>> the SIM PIN it somehow has the side-effect of also resetting the SIM lock
>> status?
>>
> It is true for IMC modem. After reset SIM PIN, the SIM PIN locking is changed. I doubt this is an issue of
> IMC modem. Following is the log:
>
>> AT+CLCK="SC",2
> < \r\n+CLCK: 0\r\n
> < \r\nOK\r\n
>
>> AT+CLCK="SC",1,"1111"
> < \r\nERROR\r\n
>
>> AT+CMEE=1
> < \r\nOK\r\n
>
>> AT+CLCK="SC",1,"1111"
> < \r\n+CME ERROR: 16\r\n
>
>> AT+CLCK="SC",1,"1111"
> < \r\n+CME ERROR: 12\r\n
>
>> AT+CPIN="13570794","1234"
> < \r\nOK\r\n
>
>> AT+CLCK="SC",2
> < \r\n+CLCK: 1\r\n
> < \r\nOK\r\n
>
> To be safe, query SIM PIN locked information after reset sim pin.

Fair enough.  Please fix the other comments in my review.

Regards,
-Denis