* Re: [PATCH] Query locked pins after reset pin
2013-09-11 19:13 [PATCH] Query locked pins after reset pin caiwen.zhang
@ 2013-09-11 18:22 ` Denis Kenzior
2013-09-12 7:31 ` Zhang, Caiwen
0 siblings, 1 reply; 4+ messages in thread
From: Denis Kenzior @ 2013-09-11 18:22 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Query locked pins after reset pin
@ 2013-09-11 19:13 caiwen.zhang
2013-09-11 18:22 ` Denis Kenzior
0 siblings, 1 reply; 4+ messages in thread
From: caiwen.zhang @ 2013-09-11 19:13 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] Query locked pins after reset pin
2013-09-11 18:22 ` Denis Kenzior
@ 2013-09-12 7:31 ` Zhang, Caiwen
2013-09-12 13:51 ` Denis Kenzior
0 siblings, 1 reply; 4+ messages in thread
From: Zhang, Caiwen @ 2013-09-12 7:31 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Query locked pins after reset pin
2013-09-12 7:31 ` Zhang, Caiwen
@ 2013-09-12 13:51 ` Denis Kenzior
0 siblings, 0 replies; 4+ messages in thread
From: Denis Kenzior @ 2013-09-12 13:51 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-12 13:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-11 19:13 [PATCH] Query locked pins after reset pin caiwen.zhang
2013-09-11 18:22 ` Denis Kenzior
2013-09-12 7:31 ` Zhang, Caiwen
2013-09-12 13:51 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.