Read messages left over in ME storage on startup.

Read messages left over in ME storage on startup.

[Andrzej Zaborowski]

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

On modems that don't support +CMT (or for class 2 SMSes) the messages are
stored in the modem and then read and deleted from there in two separate
steps with no warranty that deletion succeeds or (more likely) power is
cut before the deletion happens.  Over time the memory may become full
and if we don't want to deal with this condition we need to check on
startup if there are messages we haven't deleted.

We can't differentiate between those messages and those the user already
had on the SIM / modem before installing ofono or switching phones, so we
might want to deliver messages with REC READ status with some kind of
indication that these are potentially old so the UI doesn't emit spurious
alerts.  We don't do this now and just deliver as usual.
---
 drivers/atmodem/sms.c |  128 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 128 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 39cc718..ce78e9a 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -47,10 +47,12 @@ static const char *cmgf_prefix[] = { "+CMGF:", NULL };
 static const char *cpms_prefix[] = { "+CPMS:", NULL };
 static const char *cnmi_prefix[] = { "+CNMI:", NULL };
 static const char *cmgs_prefix[] = { "+CMGS:", NULL };
+static const char *cmgl_prefix[] = { "+CMGL:", NULL };
 static const char *none_prefix[] = { NULL };
 
 static gboolean set_cmgf(gpointer user_data);
 static gboolean set_cpms(gpointer user_data);
+static void at_cmgl_set_cpms(struct ofono_modem *modem, int store);
 
 #define MAX_CMGF_RETRIES 10
 #define MAX_CPMS_RETRIES 10
@@ -515,6 +517,126 @@ err:
 	ofono_error("Unable to parse CMTI notification");
 }
 
+static void at_cmgl_done(struct ofono_modem *modem)
+{
+	struct at_data *at = ofono_modem_get_userdata(modem);
+
+	if (at->sms->incoming == MT_STORE &&
+			at->sms->store == ME_STORE)
+		at_cmgl_set_cpms(modem, SM_STORE);
+}
+
+static void at_cmgl_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct at_data *at = ofono_modem_get_userdata(modem);
+	GAtResultIter iter;
+	const char *hexpdu;
+	unsigned char pdu[164];
+	long pdu_len;
+	int tpdu_len;
+	int index;
+	int status;
+	char buf[16];
+
+	dump_response("at_cmgl_cb", ok, result);
+
+	if (!ok) {
+		ofono_error("Initial listing SMS storage failed!");
+		goto err;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CMGL:")) {
+		if (!g_at_result_iter_next_number(&iter, &index)) {
+			ofono_error("Unable to parse CMGL response");
+			goto err;
+		}
+
+		if (!g_at_result_iter_next_number(&iter, &status)) {
+			ofono_error("Unable to parse CMGL response");
+			goto err;
+		}
+
+		if (!g_at_result_iter_skip_next(&iter)) {
+			ofono_error("Unable to parse CMGL response");
+			goto err;
+		}
+
+		if (!g_at_result_iter_next_number(&iter, &tpdu_len)) {
+			ofono_error("Unable to parse CMGL response");
+			goto err;
+		}
+
+		/* Only MT messages */
+		if (status != 0 && status != 1)
+			continue;
+
+		hexpdu = g_at_result_pdu(result);
+
+		ofono_debug("Found an old SMS PDU: %s, with len: %d",
+				hexpdu, tpdu_len);
+
+		decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
+		ofono_sms_deliver_notify(modem, pdu, pdu_len, tpdu_len);
+
+		/* We don't buffer SMS on the SIM/ME, send along a CMGD */
+		sprintf(buf, "AT+CMGD=%d", index);
+		g_at_chat_send(at->parser, buf, none_prefix,
+				at_cmgd_cb, NULL, NULL);
+	}
+
+err:
+	at_cmgl_done(modem);
+}
+
+static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cpms_request *req = user_data;
+	struct ofono_modem *modem = req->modem;
+	struct at_data *at = ofono_modem_get_userdata(modem);
+
+	if (!ok) {
+		ofono_error("Initial CPMS request failed");
+		at_cmgl_done(modem);
+		return;
+	}
+
+	at->sms->store = req->store;
+
+	g_at_chat_send(at->parser, "AT+CMGL=4", cmgl_prefix,
+			at_cmgl_cb, modem, NULL);
+}
+
+static void at_cmgl_set_cpms(struct ofono_modem *modem, int store)
+{
+	struct at_data *at = ofono_modem_get_userdata(modem);
+
+	if (store == at->sms->store) {
+		struct cpms_request req;
+
+		req.modem = modem;
+		req.store = store;
+
+		at_cmgl_cpms_cb(TRUE, NULL, &req);
+	} else {
+		char buf[128];
+		const char *readwrite = storages[store];
+		const char *incoming = storages[at->sms->incoming];
+		struct cpms_request *req = g_new(struct cpms_request, 1);
+
+		req->modem = modem;
+		req->store = store;
+
+		sprintf(buf, "AT+CPMS=\"%s\",\"%s\",\"%s\"",
+			readwrite, readwrite, incoming);
+
+		g_at_chat_send(at->parser, buf, cpms_prefix, at_cmgl_cpms_cb,
+				req, g_free);
+	}
+}
+
 static void at_sms_initialized(struct ofono_modem *modem)
 {
 	struct at_data *at = ofono_modem_get_userdata(modem);
@@ -533,6 +655,12 @@ static void at_sms_initialized(struct ofono_modem *modem)
 				modem, NULL);
 
 	ofono_sms_manager_register(modem, &ops);
+
+	/* Inspect and free the incoming SMS storage */
+	if (at->sms->incoming == MT_STORE)
+		at_cmgl_set_cpms(modem, ME_STORE);
+	else
+		at_cmgl_set_cpms(modem, at->sms->incoming);
 }
 
 static void at_sms_not_supported(struct ofono_modem *modem)
-- 
1.6.1

Re: Read messages left over in ME storage on startup.

[Denis Kenzior]

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

Hi Andrew,

On Fri, Aug 21, 2009 at 9:35 PM, Andrzej Zaborowski <
andrew.zaborowski@intel.com> wrote:

> On modems that don't support +CMT (or for class 2 SMSes) the messages are
> stored in the modem and then read and deleted from there in two separate
> steps with no warranty that deletion succeeds or (more likely) power is
> cut before the deletion happens.  Over time the memory may become full
> and if we don't want to deal with this condition we need to check on
> startup if there are messages we haven't deleted.
>
> We can't differentiate between those messages and those the user already
> had on the SIM / modem before installing ofono or switching phones, so we
> might want to deliver messages with REC READ status with some kind of
> indication that these are potentially old so the UI doesn't emit spurious
> alerts.  We don't do this now and just deliver as usual.


So the patch isn't going to work as intended.  See below for exact reasons:

+               hexpdu = g_at_result_pdu(result);


You can't use g_at_result_pdu on messages that haven't been flagged to
expect a PDU.  Right now this only works for g_at_chat_register style
unsolicited notifications for which the 'expect_pdu' has been set to TRUE.


> +
> +       g_at_chat_send(at->parser, "AT+CMGL=4", cmgl_prefix,
> +                       at_cmgl_cb, modem, NULL);


Again, this is actually filtering anything that doesn't start with a prefix
in the list given by cmgl_prefix.  Namely anything that doesn't start with
+CMGL: or is a known final response gets filtered out.

One way to go would be to use NULL for cmgl_prefix and update the parse loop
in the callback accordingly.  However, this means that the entire response
will be waited on and given once.

It might be worthwhile to support CMGL listings using something similar to
g_at_chat_send_listing.


>        ofono_sms_manager_register(modem, &ops);
> +
> +       /* Inspect and free the incoming SMS storage */
> +       if (at->sms->incoming == MT_STORE)
> +               at_cmgl_set_cpms(modem, ME_STORE);
> +       else
> +               at_cmgl_set_cpms(modem, at->sms->incoming);
>  }


It might be worth to scan both ME & SM stores.  Especially since MT
literally means combined SM + ME store.

Regards,
-Denis

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 3198 bytes --]

Re: Read messages left over in ME storage on startup.

[Andrzej Zaborowski]

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

Hi,

2009/8/25 Denis Kenzior <denkenz@gmail.com>:
> On Fri, Aug 21, 2009 at 9:35 PM, Andrzej Zaborowski
> <andrew.zaborowski@intel.com> wrote:
>> On modems that don't support +CMT (or for class 2 SMSes) the messages are
>> stored in the modem and then read and deleted from there in two separate
>> steps with no warranty that deletion succeeds or (more likely) power is
>> cut before the deletion happens.  Over time the memory may become full
>> and if we don't want to deal with this condition we need to check on
>> startup if there are messages we haven't deleted.
>>
>> We can't differentiate between those messages and those the user already
>> had on the SIM / modem before installing ofono or switching phones, so we
>> might want to deliver messages with REC READ status with some kind of
>> indication that these are potentially old so the UI doesn't emit spurious
>> alerts.  We don't do this now and just deliver as usual.
>
>> +               hexpdu = g_at_result_pdu(result);
>
> You can't use g_at_result_pdu on messages that haven't been flagged to
> expect a PDU.  Right now this only works for g_at_chat_register style
> unsolicited notifications for which the 'expect_pdu' has been set to TRUE.

Right, I now changed it to use the new g_at_chat_send_pdu_listing
function (I had an almost identical change in my tree) and tested with
non-empty SMS store on card.

>>        ofono_sms_manager_register(modem, &ops);
>> +
>> +       /* Inspect and free the incoming SMS storage */
>> +       if (at->sms->incoming == MT_STORE)
>> +               at_cmgl_set_cpms(modem, ME_STORE);
>> +       else
>> +               at_cmgl_set_cpms(modem, at->sms->incoming);
>>  }
>
> It might be worth to scan both ME & SM stores.  Especially since MT
> literally means combined SM + ME store.

We already do this, in at_cmgl_done after listing ME_STORE we repeat
the commands for SM_STORE.

Regards

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Read-messages-left-over-in-ME-storage-on-startup.patch --]
[-- Type: text/x-patch, Size: 5238 bytes --]

From 440d14f781e29125fc1dc8193b5645a11b4fa44d Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <andrew.zaborowski@intel.com>
Date: Fri, 4 Sep 2009 16:48:03 +0200
Subject: [PATCH] Read messages left over in ME storage on startup.

On modems that don't support +CMT (or for class 2 SMSes) the messages are
stored in the modem and then read and deleted from there in two separate
steps with no warranty that deletion succeeds or (more likely) power is
cut before the deletion happens.  Over time the memory may become full
and if we don't want to deal with this condition we need to check on
startup if there are messages we haven't deleted.

We can't differentiate between those messages and those the user already
had on the SIM / modem before installing ofono or switching phones, so we
might want to deliver messages with REC READ status with some kind of
indication that these are potentially old so the UI doesn't emit spurious
alerts.  We don't do this now and just deliver as usual.
---
 drivers/atmodem/sms.c |  125 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 125 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index fc53480..fa62774 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -48,10 +48,12 @@ static const char *cmgf_prefix[] = { "+CMGF:", NULL };
 static const char *cpms_prefix[] = { "+CPMS:", NULL };
 static const char *cnmi_prefix[] = { "+CNMI:", NULL };
 static const char *cmgs_prefix[] = { "+CMGS:", NULL };
+static const char *cmgl_prefix[] = { "+CMGL:", NULL };
 static const char *none_prefix[] = { NULL };
 
 static gboolean set_cmgf(gpointer user_data);
 static gboolean set_cpms(gpointer user_data);
+static void at_cmgl_set_cpms(struct ofono_sms *sms, int store);
 
 #define MAX_CMGF_RETRIES 10
 #define MAX_CPMS_RETRIES 10
@@ -499,6 +501,123 @@ err:
 	ofono_error("Unable to parse CMTI notification");
 }
 
+static void at_cmgl_done(struct ofono_sms *sms)
+{
+	struct sms_data *data = ofono_sms_get_data(sms);
+
+	if (data->incoming == MT_STORE && data->store == ME_STORE)
+		at_cmgl_set_cpms(sms, SM_STORE);
+}
+
+static void at_cmgl_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_sms *sms = user_data;
+	struct sms_data *data = ofono_sms_get_data(sms);
+	GAtResultIter iter;
+	const char *hexpdu;
+	unsigned char pdu[164];
+	long pdu_len;
+	int tpdu_len;
+	int index;
+	int status;
+	char buf[16];
+
+	dump_response("at_cmgl_notify", TRUE, result);
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CMGL:")) {
+		if (!g_at_result_iter_next_number(&iter, &index))
+			goto err;
+
+		if (!g_at_result_iter_next_number(&iter, &status))
+			goto err;
+
+		if (!g_at_result_iter_skip_next(&iter))
+			goto err;
+
+		if (!g_at_result_iter_next_number(&iter, &tpdu_len))
+			goto err;
+
+		/* Only MT messages */
+		if (status != 0 && status != 1)
+			continue;
+
+		hexpdu = g_at_result_pdu(result);
+
+		ofono_debug("Found an old SMS PDU: %s, with len: %d",
+				hexpdu, tpdu_len);
+
+		decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
+		ofono_sms_deliver_notify(sms, pdu, pdu_len, tpdu_len);
+
+		/* We don't buffer SMS on the SIM/ME, send along a CMGD */
+		sprintf(buf, "AT+CMGD=%d", index);
+		g_at_chat_send(data->chat, buf, none_prefix,
+				at_cmgd_cb, NULL, NULL);
+	}
+	return;
+
+err:
+	ofono_error("Unable to parse CMGL response");
+}
+
+static void at_cmgl_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_sms *sms = user_data;
+
+	if (!ok)
+		ofono_error("Initial listing SMS storage failed!");
+
+	at_cmgl_done(sms);
+}
+
+static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cpms_request *req = user_data;
+	struct ofono_sms *sms = req->sms;
+	struct sms_data *data = ofono_sms_get_data(sms);
+
+	if (!ok) {
+		ofono_error("Initial CPMS request failed");
+		at_cmgl_done(sms);
+		return;
+	}
+
+	data->store = req->store;
+
+	g_at_chat_send_pdu_listing(data->chat, "AT+CMGL=4", cmgl_prefix,
+					at_cmgl_notify, at_cmgl_cb, sms, NULL);
+}
+
+static void at_cmgl_set_cpms(struct ofono_sms *sms, int store)
+{
+	struct sms_data *data = ofono_sms_get_data(sms);
+
+	if (store == data->store) {
+		struct cpms_request req;
+
+		req.sms = sms;
+		req.store = store;
+
+		at_cmgl_cpms_cb(TRUE, NULL, &req);
+	} else {
+		char buf[128];
+		const char *readwrite = storages[store];
+		const char *incoming = storages[data->incoming];
+		struct cpms_request *req = g_new(struct cpms_request, 1);
+
+		req->sms = sms;
+		req->store = store;
+
+		sprintf(buf, "AT+CPMS=\"%s\",\"%s\",\"%s\"",
+			readwrite, readwrite, incoming);
+
+		g_at_chat_send(data->chat, buf, cpms_prefix, at_cmgl_cpms_cb,
+				req, g_free);
+	}
+}
+
 static void at_sms_initialized(struct ofono_sms *sms)
 {
 	struct sms_data *data = ofono_sms_get_data(sms);
@@ -516,6 +635,12 @@ static void at_sms_initialized(struct ofono_sms *sms)
 	g_at_chat_register(data->chat, "+CMGR:", at_cmgr_notify, TRUE,
 				sms, NULL);
 
+	/* Inspect and free the incoming SMS storage */
+	if (data->incoming == MT_STORE)
+		at_cmgl_set_cpms(sms, ME_STORE);
+	else
+		at_cmgl_set_cpms(sms, data->incoming);
+
 	ofono_sms_register(sms);
 }
 
-- 
1.6.1

Re: Read messages left over in ME storage on startup.

[Denis Kenzior]

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

Hi Andrew,

> > It might be worth to scan both ME & SM stores.  Especially since MT
> > literally means combined SM + ME store.
>
> We already do this, in at_cmgl_done after listing ME_STORE we repeat
> the commands for SM_STORE.
>

Yep I missed this part.  Looks good to me.

Patch has been applied.

Regards,
-Denis