Next series of delivery report patches

Next series of delivery report patches

[Pasi Miettinen]

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

Patches include:
1. Notify about new status report to sms-core.
2. Add new signal to DBus-interface for status report.
3. Support for basic status report (non-concatenated).
   -function structure is similar to deliver-notify,
    some functions could be combined
4. Implement setting "UseDeliveryReports" property via DBus.
5. Support for concatenated status reports.
   -add_pending_status_report() relates new MR-number to
    existing ofono_msg_id or creates a new ofono_msg_id
    and relates MR to it. Also receiver information is saved.
   -update_pending_status_report_mr_number() searches the
    msg_id to which the received MR is related and updates
    the status of the msg_id and MR accordingly. If message
    is determined as undelivered, related information is
    freed.
6. Saving pending SMS status reports to disk.
   -same basic idea as in SMS assembly storaging
   -function structure is similar to SMS assembly part.
    Status report part only has one more loop compared to it
    because of the deeper directory structure in which the
    pending status report information is saved.
   -storaging and storage freeing are called by
    add_pending_status_report() and
    update_pending_status_report_mr_number()

Next tasks:
1. Combine ofono_sms_deliver_notify() and ofono_sms_status_notify()
   as Aki Niemi requested. Still a valid task?
2. Implement received comments about these patches.

Br,
Pasi Miettinen

[RFC PATCH 1/6] Change in at_cds_notify for status report.

[Pasi Miettinen]

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

---
 drivers/atmodem/sms.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 439beea..3dafbc9 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -283,16 +283,27 @@ static void at_cds_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_sms *sms = user_data;
 	struct sms_data *data = ofono_sms_get_data(sms);
-	int pdulen;
-	const char *pdu;
+	long pdu_len;
+	int tpdu_len;
+	const char *hexpdu;
+	unsigned char pdu[176];
 	char buf[256];
 
-	if (!at_parse_pdu_common(result, "+CDS:", &pdu, &pdulen)) {
+	if (!at_parse_pdu_common(result, "+CDS:", &hexpdu, &tpdu_len)) {
 		ofono_error("Unable to parse CDS notification");
 		return;
 	}
 
-	DBG("Got new Status-Report PDU via CDS: %s, %d", pdu, pdulen);
+	if (strlen(hexpdu) > sizeof(pdu) * 2) {
+		ofono_error("Bad PDU length in CMT notification");
+		return;
+	}
+
+	DBG("Got new Status-Report PDU via CDS: %s, %d", hexpdu, tpdu_len);
+
+	/*Decode pdu and notify about new SMS status report*/
+	decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
+	ofono_sms_status_notify(sms, pdu, pdu_len, tpdu_len);
 
 	/* We must acknowledge the PDU using CNMA */
 	if (data->cnma_ack_pdu)
-- 
1.6.0.4

[RFC PATCH 2/6] Add IncomingStatusReport a{sv} signal.

[Pasi Miettinen]

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

---
 src/sms.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index c0e9fc4..618e035 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -472,9 +472,10 @@ static GDBusMethodTable sms_manager_methods[] = {
 };
 
 static GDBusSignalTable sms_manager_signals[] = {
-	{ "PropertyChanged",	"sv"		},
-	{ "IncomingMessage",	"sa{sv}"	},
-	{ "ImmediateMessage",	"sa{sv}"	},
+	{ "PropertyChanged",		"sv"		},
+	{ "IncomingMessage",		"sa{sv}"	},
+	{ "ImmediateMessage",		"sa{sv}"	},
+	{ "IncomingStatusReport",	"a{sv}"		},
 	{ }
 };
 
-- 
1.6.0.4

[RFC PATCH 3/6] Support for basic SMS Status Report.

[Pasi Miettinen]

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

---
 src/sms.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 109 insertions(+), 1 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 618e035..14748c6 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -553,6 +553,75 @@ static void dispatch_text_message(struct ofono_sms *sms,
 	}
 }
 
+static void dispatch_sms_delivery_report(struct ofono_sms *sms,
+					const enum sms_st *st,
+					const struct sms_address *raddr,
+					const struct sms_scts *scts,
+					const struct sms_scts *dt)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(sms->atom);
+	DBusMessage *signal;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	char buf[128];
+	time_t ts;
+	struct tm remote;
+	struct tm local;
+	const char *str = buf;
+
+	if (!st) {
+		DBG("status unavailable");
+		return;
+	}
+
+	signal = dbus_message_new_signal(path, OFONO_SMS_MANAGER_INTERFACE,
+						"IncomingStatusReport");
+
+	if (!signal)
+		return;
+
+	/*Start assembling dbus-message*/
+	dbus_message_iter_init_append(signal, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+						&dict);
+
+	/*This is the time when sender sent the message*/
+	ts = sms_scts_to_time(scts, &remote);
+	localtime_r(&ts, &local);
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
+	buf[127] = '\0';
+	ofono_dbus_dict_append(&dict, "LocalSentTime", DBUS_TYPE_STRING, &str);
+
+	/*This is the time when the message was delivered to the recipient*/
+	ts = sms_scts_to_time(dt, &remote);
+	localtime_r(&ts, &local);
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
+	buf[127] = '\0';
+	ofono_dbus_dict_append(&dict, "LocalDeliverTime",
+					DBUS_TYPE_STRING, &str);
+
+	/*Status*/
+	if (*st == 0x00) {
+		str = sms_address_to_string(raddr);
+		ofono_dbus_dict_append(&dict, "delivered",
+					DBUS_TYPE_STRING, &str);
+	} else {
+		str = sms_address_to_string(raddr);
+		ofono_dbus_dict_append(&dict, "undeliverable",
+					DBUS_TYPE_STRING, &str);
+	}
+
+	/*dbus-message assembled*/
+	dbus_message_iter_close_container(&iter, &dict);
+
+	g_dbus_send_message(conn, signal);
+}
+
 static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 {
 	GSList *l;
@@ -660,6 +729,17 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	}
 }
 
+static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
+{
+	const struct sms *s;
+
+	s = sms_list->data;
+	dispatch_sms_delivery_report(sms, &s->status_report.st,
+					&s->status_report.raddr,
+					&s->status_report.scts,
+					&s->status_report.dt);
+}
+
 static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 {
 	GSList *l;
@@ -693,6 +773,16 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
+static void handle_sms_status_report(struct ofono_sms *sms,
+						const struct sms *incoming)
+{
+	GSList *l;
+
+	l = g_slist_append(NULL, (void *)incoming);
+	sms_status_report_dispatch(sms, l);
+	g_slist_free(l);
+}
+
 static inline gboolean handle_mwi(struct ofono_sms *sms, struct sms *s)
 {
 	gboolean discard;
@@ -824,7 +914,25 @@ out:
 void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
 				int len, int tpdu_len)
 {
-	ofono_error("SMS Status-Report not yet handled");
+	struct sms s;
+	enum sms_class cls;
+
+	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
+		ofono_error("Unable to decode PDU");
+		return;
+	}
+
+	if (s.type != SMS_TYPE_STATUS_REPORT) {
+		ofono_error("Expecting a STATUS REPORT pdu");
+		return;
+	}
+
+	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
+		ofono_error("Unknown / Reserved DCS.  Ignoring");
+		return;
+	}
+
+	handle_sms_status_report(sms, &s);
 }
 
 int ofono_sms_driver_register(const struct ofono_sms_driver *d)
-- 
1.6.0.4

[RFC PATCH 4/6] SetProperty for UseDeliveryReports.

[Pasi Miettinen]

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

---
 src/sms.c       |   34 +++++++++++++++++++++++++++++++++-
 src/smsutil.c   |    5 +++--
 src/smsutil.h   |    3 ++-
 test/send-sms   |    7 ++++++-
 unit/test-sms.c |   10 +++++-----
 5 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 14748c6..16a4d41 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -67,6 +67,7 @@ struct ofono_sms {
 	const struct ofono_sms_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
+	gboolean use_delivery_reports;
 };
 
 struct pending_pdu {
@@ -131,6 +132,9 @@ static DBusMessage *generate_get_properties_reply(struct ofono_sms *sms,
 	ofono_dbus_dict_append(&dict, "ServiceCenterAddress", DBUS_TYPE_STRING,
 				&sca);
 
+	ofono_dbus_dict_append(&dict, "UseDeliveryReports", DBUS_TYPE_BOOLEAN,
+				&sms->use_delivery_reports);
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
@@ -260,6 +264,27 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg,
 		return NULL;
 	}
 
+	if (!strcmp(property, "UseDeliveryReports")) {
+		const char *path = __ofono_atom_get_path(sms->atom);
+		gboolean value;
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &value);
+
+		sms->use_delivery_reports = value;
+
+		g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
+
+		ofono_dbus_signal_property_changed(conn, path,
+						OFONO_SMS_MANAGER_INTERFACE,
+						"UseDeliveryReports",
+						DBUS_TYPE_BOOLEAN, &value);
+
+		return NULL;
+	}
+
 	return __ofono_error_invalid_args(msg);
 }
 
@@ -426,7 +451,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	if (valid_phone_number_format(to) == FALSE)
 		return __ofono_error_invalid_format(msg);
 
-	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset);
+	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
+					sms->use_delivery_reports);
 
 	if (!msg_list)
 		return __ofono_error_invalid_format(msg);
@@ -1004,6 +1030,9 @@ static void sms_remove(struct ofono_atom *atom)
 					"NextMessageId", sms->next_msg_id);
 		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
 					"NextReference", sms->ref);
+		g_key_file_set_boolean(sms->settings, SETTINGS_GROUP,
+					"UseDeliveryReports",
+					sms->use_delivery_reports);
 
 		storage_close(sms->imsi, SETTINGS_STORE, sms->settings, TRUE);
 
@@ -1090,6 +1119,9 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi)
 							"NextMessageId", NULL);
 	sms->ref = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
 							"NextReference", NULL);
+	sms->use_delivery_reports =
+		g_key_file_get_boolean(sms->settings, SETTINGS_GROUP,
+					"UseDeliveryReports", NULL);
 
 	if (sms->ref >= 65536)
 		sms->ref = 1;
diff --git a/src/smsutil.c b/src/smsutil.c
index 30c1250..a92089b 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2642,7 +2642,8 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)
  * if no concatenation took place.
  */
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
-				gboolean use_16bit, int *ref_offset)
+				gboolean use_16bit, int *ref_offset,
+				const gboolean use_delivery_reports)
 {
 	struct sms template;
 	int offset = 0;
@@ -2658,7 +2659,7 @@ GSList *sms_text_prepare(const char *utf8, guint16 ref,
 	template.submit.rd = FALSE;
 	template.submit.vpf = SMS_VALIDITY_PERIOD_FORMAT_RELATIVE;
 	template.submit.rp = FALSE;
-	template.submit.srr = FALSE;
+	template.submit.srr = use_delivery_reports;
 	template.submit.mr = 0;
 	template.submit.vp.relative = 0xA7; /* 24 Hours */
 
diff --git a/src/smsutil.h b/src/smsutil.h
index a36a9d3..cfb2765 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -482,7 +482,8 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
 
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
-				gboolean use_16bit, int *ref_offset);
+				gboolean use_16bit, int *ref_offset,
+				const gboolean use_delivery_reports);
 
 gboolean cbs_dcs_decode(guint8 dcs, gboolean *udhi, enum sms_class *cls,
 			enum sms_charset *charset, gboolean *compressed,
diff --git a/test/send-sms b/test/send-sms
index 8024fda..7c1e56e 100755
--- a/test/send-sms
+++ b/test/send-sms
@@ -15,4 +15,9 @@ path = properties["Modems"][0]
 manager = dbus.Interface(bus.get_object('org.ofono', path),
 						'org.ofono.SmsManager')
 
-manager.SendMessage(sys.argv[1], sys.argv[2])
+if sys.argv[3] == "1":
+	manager.SetProperty("UseDeliveryReports", dbus.Boolean(1))
+	manager.SendMessage(sys.argv[1], sys.argv[2])
+else:
+	manager.SetProperty("UseDeliveryReports", dbus.Boolean(0))
+	manager.SendMessage(sys.argv[1], sys.argv[2])
diff --git a/unit/test-sms.c b/unit/test-sms.c
index d7d97f3..9a20897 100644
--- a/unit/test-sms.c
+++ b/unit/test-sms.c
@@ -685,7 +685,7 @@ static void test_assembly()
 	if (g_test_verbose())
 		g_printf("Text:\n%s\n", utf8);
 
-	l = sms_text_prepare(utf8, ref, TRUE, NULL);
+	l = sms_text_prepare(utf8, ref, TRUE, NULL, FALSE);
 	g_assert(l);
 	g_assert(g_slist_length(l) == 3);
 
@@ -715,7 +715,7 @@ static void test_prepare_7bit()
 	int encoded_tpdu_len;
 	char *encoded_pdu;
 
-	r = sms_text_prepare(test_no_fragmentation_7bit, 0, FALSE, NULL);
+	r = sms_text_prepare(test_no_fragmentation_7bit, 0, FALSE, NULL, FALSE);
 
 	g_assert(r != NULL);
 
@@ -798,7 +798,7 @@ static void test_prepare_concat(gconstpointer data)
 	if (g_test_verbose())
 		g_print("strlen: %zd\n", strlen(test->str));
 
-	r = sms_text_prepare(test->str, 0, TRUE, NULL);
+	r = sms_text_prepare(test->str, 0, TRUE, NULL, FALSE);
 
 	g_assert(r);
 	g_assert(g_slist_length(r) == test->segments);
@@ -875,7 +875,7 @@ static void test_limit(gunichar uni, int target_size, gboolean use_16bit)
 
 	utf8[i] = '\0';
 
-	l = sms_text_prepare(utf8, 0, use_16bit, NULL);
+	l = sms_text_prepare(utf8, 0, use_16bit, NULL, FALSE);
 
 	g_assert(l);
 	g_assert(g_slist_length(l) == 255);
@@ -888,7 +888,7 @@ static void test_limit(gunichar uni, int target_size, gboolean use_16bit)
 	memcpy(utf8 + i, utf8_char, stride);
 	utf8[i+stride] = '\0';
 
-	l = sms_text_prepare(utf8, 0, use_16bit, NULL);
+	l = sms_text_prepare(utf8, 0, use_16bit, NULL, FALSE);
 
 	g_assert(l == NULL);
 	g_free(utf8);
-- 
1.6.0.4

[RFC PATCH 5/6] Support for concatenated SMS status report.

[Pasi Miettinen]

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

---
 include/history.h |    2 +
 src/sms.c         |   67 ++++++++++++++++++++----
 src/smsutil.c     |  152 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h     |   32 +++++++++++
 4 files changed, 243 insertions(+), 10 deletions(-)

diff --git a/include/history.h b/include/history.h
index 300a4fb..17445f0 100644
--- a/include/history.h
+++ b/include/history.h
@@ -33,6 +33,8 @@ enum ofono_history_sms_status {
 	OFONO_HISTORY_SMS_STATUS_PENDING,
 	OFONO_HISTORY_SMS_STATUS_SUBMITTED,
 	OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED,
+	OFONO_HISTORY_SMS_STATUS_DELIVERED,
+	OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED,
 };
 
 struct ofono_history_context {
diff --git a/src/sms.c b/src/sms.c
index 16a4d41..76b0985 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -68,6 +68,7 @@ struct ofono_sms {
 	void *driver_data;
 	struct ofono_atom *atom;
 	gboolean use_delivery_reports;
+	GSList *pending_status_reports;
 };
 
 struct pending_pdu {
@@ -83,6 +84,8 @@ struct tx_queue_entry {
 	unsigned int msg_id;
 	unsigned int retry;
 	DBusMessage *msg;
+	gboolean status_report;
+	struct sms_address receiver;
 };
 
 static void set_sca(struct ofono_sms *sms,
@@ -331,6 +334,10 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	entry->cur_pdu += 1;
 	entry->retry = 0;
 
+	if (entry->status_report)
+		add_pending_status_report(&sms->pending_status_reports, mr,
+					entry->msg_id, &entry->receiver);
+
 	if (entry->cur_pdu < entry->num_pdus) {
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 		return;
@@ -462,6 +469,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
 	entry = create_tx_queue_entry(msg_list);
 
+	sms_address_from_string(&entry->receiver, to);
+
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
 
@@ -474,6 +483,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	entry->msg = dbus_message_ref(msg);
 	entry->msg_id = sms->next_msg_id++;
+	entry->status_report = sms->use_delivery_reports;
 
 	g_queue_push_tail(sms->txq, entry);
 
@@ -583,10 +593,12 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 					const enum sms_st *st,
 					const struct sms_address *raddr,
 					const struct sms_scts *scts,
-					const struct sms_scts *dt)
+					const struct sms_scts *dt,
+					const guint8 mr)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(sms->atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	DBusMessage *signal;
 	DBusMessageIter iter;
 	DBusMessageIter dict;
@@ -595,6 +607,7 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 	struct tm remote;
 	struct tm local;
 	const char *str = buf;
+	unsigned int relating_msg_id;
 
 	if (!st) {
 		DBG("status unavailable");
@@ -631,21 +644,45 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 	ofono_dbus_dict_append(&dict, "LocalDeliverTime",
 					DBUS_TYPE_STRING, &str);
 
+	str = sms_address_to_string(raddr);
+
 	/*Status*/
 	if (*st == 0x00) {
-		str = sms_address_to_string(raddr);
-		ofono_dbus_dict_append(&dict, "delivered",
-					DBUS_TYPE_STRING, &str);
+		enum ofono_history_sms_status status;
+
+		status = update_pending_status_report_mr_number(
+					&sms->pending_status_reports, mr, raddr,
+					SMS_STATUS_REPORT_MESSAGE_DELIVERED,
+					&relating_msg_id);
+
+		if (status == OFONO_HISTORY_SMS_STATUS_DELIVERED) {
+			ofono_dbus_dict_append(&dict, "delivered",
+						DBUS_TYPE_STRING, &str);
+			/*dbus-message assembled*/
+			dbus_message_iter_close_container(&iter, &dict);
+			g_dbus_send_message(conn, signal);
+
+			__ofono_history_sms_send_status(modem, relating_msg_id,
+					time(NULL),
+					OFONO_HISTORY_SMS_STATUS_DELIVERED);
+		}
 	} else {
-		str = sms_address_to_string(raddr);
 		ofono_dbus_dict_append(&dict, "undeliverable",
 					DBUS_TYPE_STRING, &str);
-	}
 
-	/*dbus-message assembled*/
-	dbus_message_iter_close_container(&iter, &dict);
+		/*dbus-message assembled*/
+		dbus_message_iter_close_container(&iter, &dict);
+		g_dbus_send_message(conn, signal);
 
-	g_dbus_send_message(conn, signal);
+		update_pending_status_report_mr_number(
+					&sms->pending_status_reports, mr, raddr,
+					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED,
+					&relating_msg_id);
+
+		__ofono_history_sms_send_status(modem, relating_msg_id,
+				time(NULL),
+				OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);
+	}
 }
 
 static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
@@ -763,7 +800,8 @@ static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	dispatch_sms_delivery_report(sms, &s->status_report.st,
 					&s->status_report.raddr,
 					&s->status_report.scts,
-					&s->status_report.dt);
+					&s->status_report.dt,
+					s->status_report.mr);
 }
 
 static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
@@ -1041,6 +1079,15 @@ static void sms_remove(struct ofono_atom *atom)
 		sms->settings = NULL;
 	}
 
+	if (sms->pending_status_reports) {
+
+		g_slist_foreach(sms->pending_status_reports,
+				(GFunc) free_pending_status_report,
+				&sms->pending_status_reports);
+
+		sms->pending_status_reports = NULL;
+	}
+
 	g_free(sms);
 }
 
diff --git a/src/smsutil.c b/src/smsutil.c
index a92089b..6e3caa3 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2635,6 +2635,158 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)
 	return l;
 }
 
+static gboolean compare_sms_addresses(const struct sms_address *first,
+					const struct sms_address *second)
+{
+	const char *str1 = first->address;
+	const char *str2 = second->address;
+
+	if (first->number_type != second->number_type)
+		return FALSE;
+
+	if (first->numbering_plan != second->numbering_plan)
+		return FALSE;
+
+	if (g_strcmp0(str1, str2) != 0)
+		return FALSE;
+
+	return TRUE;
+}
+
+void add_pending_status_report(GSList **pending_status_reports, const int mr,
+					const unsigned int msg_id,
+					const struct sms_address *receiver)
+{
+	struct pending_status_report *status_report;
+	GSList *l;
+	struct mr_number *new_mr;
+
+	for (l = *pending_status_reports; l; l = l->next) {
+		status_report = l->data;
+
+		if (status_report->ofono_msg_id == msg_id) {
+
+			/*Relate new MR-number to existing ofono_msg_id*/
+			new_mr = g_new0(struct mr_number, 1);
+			new_mr->mr = mr;
+			new_mr->delivery_status =
+					SMS_STATUS_REPORT_STATUS_PENDING;
+			status_report->related_mr_numbers =
+				g_slist_append(status_report-> \
+						related_mr_numbers, new_mr);
+			return;
+		}
+	}
+
+	/*Create new ofono_msg_id and relate MR-number to it*/
+	status_report = g_new0(struct pending_status_report, 1);
+
+	status_report->ofono_msg_id = msg_id;
+	status_report->receiver = *receiver;
+
+	new_mr = g_new0(struct mr_number, 1);
+	new_mr->mr = mr;
+	new_mr->delivery_status = SMS_STATUS_REPORT_STATUS_PENDING;
+	status_report->related_mr_numbers =
+		g_slist_append(status_report->related_mr_numbers, new_mr);
+
+	*pending_status_reports =
+		g_slist_append(*pending_status_reports, status_report);
+}
+
+void free_pending_status_report(struct pending_status_report *status_report,
+				GSList **pending_status_reports)
+{
+
+	/*Free msg_id and relating MR-numbers*/
+	g_slist_foreach(status_report->related_mr_numbers, (GFunc)g_free, NULL);
+	g_slist_free(status_report->related_mr_numbers);
+
+	*pending_status_reports = g_slist_remove(*pending_status_reports,
+								status_report);
+
+	status_report = NULL;
+}
+
+enum ofono_history_sms_status update_pending_status_report_mr_number(
+		GSList **pending_status_reports, const guint8 mr,
+		const struct sms_address *receiver,
+		const enum sms_status_report_result status_report_result,
+		unsigned int *relating_msg_id)
+{
+	struct pending_status_report *status_report;
+	GSList *l;
+	GSList *i;
+	struct mr_number *current_mr;
+	gboolean delivery_successful;
+	gboolean mr_number_found = FALSE;
+
+	/*Each ofono_msg_id can relate to 1-n mr_numbers*/
+	for (l = *pending_status_reports; l; l = l->next) {
+		delivery_successful = TRUE;
+		status_report = l->data;
+
+		/* MR-number does not relate to this msg_id because of
+		 * different addresses
+		 */
+		if (!compare_sms_addresses(&status_report->receiver, receiver))
+			continue;
+
+		for (i = status_report->related_mr_numbers; i; i = i->next) {
+			current_mr = i->data;
+
+			/*Message with current MR-number was delivered*/
+			if (current_mr->mr == mr &&
+				status_report_result ==
+				SMS_STATUS_REPORT_MESSAGE_DELIVERED) {
+				current_mr->delivery_status =
+					SMS_STATUS_REPORT_MESSAGE_DELIVERED;
+
+				mr_number_found = TRUE;
+
+				continue;
+			}
+
+			/* If one part of the fragmented message is undelivered,
+			 * whole message is declared undelivered
+			 */
+			if (current_mr->mr == mr &&
+				status_report_result ==
+				SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED) {
+				free_pending_status_report(status_report,
+						pending_status_reports);
+
+				/*Received MR is handled*/
+				return OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED;
+			}
+
+			if (current_mr->delivery_status ==
+				SMS_STATUS_REPORT_STATUS_PENDING)
+				/*msg_id still relates to pending statusreport*/
+				delivery_successful = FALSE;
+		}
+
+		/*all parts of the message succesfully delivered*/
+		if (delivery_successful) {
+			*relating_msg_id = status_report->ofono_msg_id;
+			free_pending_status_report(status_report,
+							pending_status_reports);
+
+			/*Received MR is handled*/
+			return OFONO_HISTORY_SMS_STATUS_DELIVERED;
+		}
+	}
+
+	if (!mr_number_found) {
+		/* MR-number is not found in pending_status_reports. Can this
+		 * kind of error occur? Maybe reboot just before MR-number is
+		 * saved to disk? What to do?
+		 */
+	}
+
+	return OFONO_HISTORY_SMS_STATUS_PENDING;
+}
+
 /* Prepares the text for transmission.  Breaks up into fragments if
  * necessary using ref as the concatenated message reference number.
  * Returns a list of sms messages in order.  If ref_offset is given,
diff --git a/src/smsutil.h b/src/smsutil.h
index cfb2765..a809453 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -21,6 +21,8 @@
 
 #define CBS_MAX_GSM_CHARS 93
 
+#include "history.h"
+
 enum sms_type {
 	SMS_TYPE_DELIVER = 0,
 	SMS_TYPE_DELIVER_REPORT_ACK,
@@ -178,6 +180,12 @@ enum sms_pid_type {
 	SMS_PID_TYPE_USIM_DOWNLOAD = 0x7f,
 };
 
+enum sms_status_report_result {
+	SMS_STATUS_REPORT_STATUS_PENDING = 0,
+	SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED = 1,
+	SMS_STATUS_REPORT_MESSAGE_DELIVERED = 2,
+};
+
 enum cbs_language {
 	CBS_LANGUAGE_GERMAN = 0x0,
 	CBS_LANGUAGE_ENGLISH = 0x1,
@@ -364,6 +372,17 @@ struct sms_assembly {
 	GSList *assembly_list;
 };
 
+struct mr_number {
+	guint8 mr;
+	enum sms_status_report_result delivery_status;
+};
+
+struct pending_status_report {
+	unsigned int ofono_msg_id;
+	struct sms_address receiver;
+	GSList *related_mr_numbers;
+};
+
 struct cbs {
 	enum cbs_geo_scope gs;			/* 2 bits */
 	guint16 message_code;			/* 10 bits */
@@ -481,6 +500,19 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					guint16 ref, guint8 max, guint8 seq);
 void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
 
+void add_pending_status_report(GSList **pending_status_reports, const int mr,
+					const unsigned int msg_id,
+					const struct sms_address *receiver);
+
+void free_pending_status_report(struct pending_status_report *status_report,
+				GSList **pending_status_reports);
+
+enum ofono_history_sms_status update_pending_status_report_mr_number(
+		GSList **pending_status_reports, const guint8 mr,
+		const struct sms_address *receiver,
+		const enum sms_status_report_result status_report_result,
+		unsigned int *relating_msg_id);
+
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
 				gboolean use_16bit, int *ref_offset,
 				const gboolean use_delivery_reports);
-- 
1.6.0.4

[RFC PATCH 6/6] Save pending SMS Status Reports to disk.

[Pasi Miettinen]

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

---
 src/sms.c     |   11 ++-
 src/smsutil.c |  263 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/smsutil.h |    9 ++-
 3 files changed, 273 insertions(+), 10 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 76b0985..bf54f0b 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -336,7 +336,8 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 
 	if (entry->status_report)
 		add_pending_status_report(&sms->pending_status_reports, mr,
-					entry->msg_id, &entry->receiver);
+					entry->msg_id, &entry->receiver,
+					sms->assembly->imsi);
 
 	if (entry->cur_pdu < entry->num_pdus) {
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
@@ -653,11 +654,12 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 		status = update_pending_status_report_mr_number(
 					&sms->pending_status_reports, mr, raddr,
 					SMS_STATUS_REPORT_MESSAGE_DELIVERED,
-					&relating_msg_id);
+					&relating_msg_id, sms->assembly->imsi);
 
 		if (status == OFONO_HISTORY_SMS_STATUS_DELIVERED) {
 			ofono_dbus_dict_append(&dict, "delivered",
 						DBUS_TYPE_STRING, &str);
+
 			/*dbus-message assembled*/
 			dbus_message_iter_close_container(&iter, &dict);
 			g_dbus_send_message(conn, signal);
@@ -677,7 +679,7 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 		update_pending_status_report_mr_number(
 					&sms->pending_status_reports, mr, raddr,
 					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED,
-					&relating_msg_id);
+					&relating_msg_id, sms->assembly->imsi);
 
 		__ofono_history_sms_send_status(modem, relating_msg_id,
 				time(NULL),
@@ -1226,6 +1228,9 @@ void ofono_sms_register(struct ofono_sms *sms)
 		imsi = ofono_sim_get_imsi(sms->sim);
 		sms->assembly = sms_assembly_new(imsi);
 
+		sms->pending_status_reports =
+			pending_status_reports_load_backup(imsi);
+
 		sms_load_settings(sms, imsi);
 	} else {
 		sms->assembly = sms_assembly_new(NULL);
diff --git a/src/smsutil.c b/src/smsutil.c
index 6e3caa3..1083b76 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -45,6 +45,10 @@
 #define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
 #define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
 
+#define SMS_SR_BACKUP_PATH STORAGEDIR "/%s/sms_sr"
+#define SMS_SR_BACKUP_PATH_DIR SMS_SR_BACKUP_PATH "/%s-%i-%i"
+#define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH_DIR "/%i/%i"
+
 #define SMS_ADDR_FMT "%24[0-9A-F]"
 
 static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
@@ -2635,6 +2639,239 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)
 	return l;
 }
 
+static gboolean pending_status_report_backup_mr(
+			const struct pending_status_report *status_report,
+			const struct sms_address *receiver, const char *imsi,
+			const struct mr_number *mr)
+{
+	int len = sizeof(enum sms_status_report_result);
+	unsigned char buf[3];
+
+	if (!imsi)
+		return FALSE;
+
+	/* storagedir/%s/sms_assembly/%s-%i-%i/%i/%i */
+	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
+			receiver->address, receiver->number_type,
+			receiver->numbering_plan, status_report->ofono_msg_id,
+			mr->mr) != len)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean pending_status_report_backup_free_mr(
+			const struct pending_status_report *status_report,
+			const struct sms_address *receiver, const char *imsi,
+			const struct mr_number *mr)
+{
+	char *path;
+
+	if (!imsi)
+		return FALSE;
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, receiver->address,
+				receiver->number_type,
+				receiver->numbering_plan,
+				status_report->ofono_msg_id, mr->mr);
+
+	unlink(path);
+	g_free(path);
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR "/%i", imsi,
+				receiver->address, receiver->number_type,
+				receiver->numbering_plan,
+				status_report->ofono_msg_id);
+
+	/* If the msg_id does not have relating MR-numbers anymore, remove it */
+	rmdir(path);
+	g_free(path);
+
+	return TRUE;
+}
+
+static gboolean pending_status_report_backup_update_mr(
+			const struct pending_status_report *status_report,
+			const struct sms_address *receiver,
+			const char *imsi, const struct mr_number *mr)
+{
+	if (!pending_status_report_backup_free_mr(status_report, receiver,
+								imsi, mr))
+		return FALSE;
+
+	if (!pending_status_report_backup_mr(status_report, receiver, imsi, mr))
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean pending_status_report_backup_free_id(
+			const struct pending_status_report *status_report,
+			const struct sms_address *receiver, const char *imsi)
+{
+	char *path;
+	GSList *l;
+	struct mr_number *current_mr;
+
+	if (!imsi)
+		return FALSE;
+
+	for (l = status_report->related_mr_numbers; l; l = l->next) {
+		current_mr = l->data;
+
+		pending_status_report_backup_free_mr(status_report, receiver,
+							imsi, current_mr);
+	}
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR "/%i", imsi,
+				receiver->address, receiver->number_type,
+				receiver->numbering_plan,
+				status_report->ofono_msg_id);
+
+	rmdir(path);
+	g_free(path);
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi,
+				receiver->address, receiver->number_type,
+				receiver->numbering_plan);
+
+	/* If the address does not have pending msg_ids anymore, remove it */
+	rmdir(path);
+	g_free(path);
+
+	return TRUE;
+}
+
+static void pending_status_reports_load_mrs(
+				struct pending_status_report *status_report,
+				const struct dirent *id_dir,
+				const struct dirent *addr_dir, const char *imsi)
+{
+	char *path;
+	struct dirent **mrs;
+	int len;
+	struct mr_number *mr_nmbr;
+	unsigned char buf[3];
+	int r;
+
+	if (id_dir->d_type != DT_DIR)
+		return;
+
+	if (sscanf(id_dir->d_name, "%i", &status_report->ofono_msg_id) < 1)
+		return;
+
+	/* Go through different MR-numbers. MR-number contains the status of
+	 * the related message.
+	 */
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
+		imsi, addr_dir->d_name, id_dir->d_name);
+	len = scandir(path, &mrs, NULL, versionsort);
+
+	g_free(path);
+
+	if (len < 0)
+		return;
+
+	while (len--) {
+		mr_nmbr = g_new0(struct mr_number, 1);
+		r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH "/%s/%s/%s",
+				imsi, addr_dir->d_name,
+				id_dir->d_name, mrs[len]->d_name);
+
+		if (r < 0)
+			return;
+
+		mr_nmbr->mr = atoi(mrs[len]->d_name);
+		mr_nmbr->delivery_status = atoi((char *)buf);
+		status_report->related_mr_numbers =
+			g_slist_append(status_report->related_mr_numbers,
+								mr_nmbr);
+
+		free(mrs[len]);
+	}
+
+	free(mrs);
+}
+
+static void pending_status_reports_load_ids(GSList **pending_status_reports,
+						const struct dirent *addr_dir,
+						const char *imsi)
+{
+	char *path;
+	struct dirent **ids;
+	struct sms_address *addr;
+	struct pending_status_report *status_report;
+	int len;
+	if (addr_dir->d_type != DT_DIR)
+		return;
+
+	addr = g_new0(struct sms_address, 1);
+
+	if (sscanf(addr_dir->d_name, "%[0-9]-%i-%i",
+			addr->address, (int *) &addr->number_type,
+			(int *) &addr->numbering_plan) < 3)
+		return;
+
+	/* Go through different msg_ids. Each msg_id can relate to
+	 * 1-n MR-numbers.
+	 */
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
+							addr_dir->d_name);
+	len = scandir(path, &ids, NULL, versionsort);
+
+	g_free(path);
+
+	if (len < 0)
+		return;
+
+	/* Do not try to load . and .. directories */
+	while (2 < len--) {
+		status_report = g_new0(struct pending_status_report, 1);
+		status_report->receiver = g_new0(struct sms_address, 1);
+		pending_status_reports_load_mrs(status_report, ids[len],
+								addr_dir, imsi);
+		status_report->ofono_msg_id = atoi(ids[len]->d_name);
+		*status_report->receiver = *addr;
+
+		*pending_status_reports =
+			g_slist_append(*pending_status_reports, status_report);
+		free(ids[len]);
+	}
+
+	free(ids);
+}
+
+GSList *pending_status_reports_load_backup(const char *imsi)
+{
+	GSList *ret = NULL;
+	char *path;
+	struct dirent **addresses;
+	int len;
+
+	if (imsi) {
+		/* Restore state from backup */
+		path = g_strdup_printf(SMS_SR_BACKUP_PATH, imsi);
+		len = scandir(path, &addresses, NULL, alphasort);
+
+		g_free(path);
+
+		if (len < 0)
+			return ret;
+		/* Go through different addresses. Each address can relate to
+		 * 1-n msg_ids. Do not try to load . and .. directories.
+		 */
+		while (2 < len--) {
+			pending_status_reports_load_ids(&ret, addresses[len],
+									imsi);
+			free(addresses[len]);
+		}
+
+		free(addresses);
+	}
+
+	return ret;
+}
+
 static gboolean compare_sms_addresses(const struct sms_address *first,
 					const struct sms_address *second)
 {
@@ -2655,7 +2892,8 @@ static gboolean compare_sms_addresses(const struct sms_address *first,
 
 void add_pending_status_report(GSList **pending_status_reports, const int mr,
 					const unsigned int msg_id,
-					const struct sms_address *receiver)
+					struct sms_address *receiver,
+					const char *imsi)
 {
 	struct pending_status_report *status_report;
 	GSList *l;
@@ -2674,15 +2912,18 @@ void add_pending_status_report(GSList **pending_status_reports, const int mr,
 			status_report->related_mr_numbers =
 				g_slist_append(status_report-> \
 						related_mr_numbers, new_mr);
+			pending_status_report_backup_mr(status_report, receiver,
+							imsi, new_mr);
 			return;
 		}
 	}
 
 	/*Create new ofono_msg_id and relate MR-number to it*/
 	status_report = g_new0(struct pending_status_report, 1);
+	status_report->receiver = g_new0(struct sms_address, 1);
 
 	status_report->ofono_msg_id = msg_id;
-	status_report->receiver = *receiver;
+	*status_report->receiver = *receiver;
 
 	new_mr = g_new0(struct mr_number, 1);
 	new_mr->mr = mr;
@@ -2690,6 +2931,8 @@ void add_pending_status_report(GSList **pending_status_reports, const int mr,
 	status_report->related_mr_numbers =
 		g_slist_append(status_report->related_mr_numbers, new_mr);
 
+	pending_status_report_backup_mr(status_report, receiver, imsi, new_mr);
+
 	*pending_status_reports =
 		g_slist_append(*pending_status_reports, status_report);
 }
@@ -2702,6 +2945,8 @@ void free_pending_status_report(struct pending_status_report *status_report,
 	g_slist_foreach(status_report->related_mr_numbers, (GFunc)g_free, NULL);
 	g_slist_free(status_report->related_mr_numbers);
 
+	g_free(status_report->receiver);
+
 	*pending_status_reports = g_slist_remove(*pending_status_reports,
 								status_report);
 
@@ -2712,7 +2957,7 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 		GSList **pending_status_reports, const guint8 mr,
 		const struct sms_address *receiver,
 		const enum sms_status_report_result status_report_result,
-		unsigned int *relating_msg_id)
+		unsigned int *relating_msg_id, const char *imsi)
 {
 	struct pending_status_report *status_report;
 	GSList *l;
@@ -2729,7 +2974,7 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 		/* MR-number does not relate to this msg_id because of
 		 * different addresses
 		 */
-		if (!compare_sms_addresses(&status_report->receiver, receiver))
+		if (!compare_sms_addresses(status_report->receiver, receiver))
 			continue;
 
 		for (i = status_report->related_mr_numbers; i; i = i->next) {
@@ -2742,6 +2987,10 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 				current_mr->delivery_status =
 					SMS_STATUS_REPORT_MESSAGE_DELIVERED;
 
+				pending_status_report_backup_update_mr(
+						status_report, receiver, imsi,
+								current_mr);
+
 				mr_number_found = TRUE;
 
 				continue;
@@ -2756,6 +3005,9 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 				free_pending_status_report(status_report,
 						pending_status_reports);
 
+				pending_status_report_backup_free_id(
+						status_report, receiver, imsi);
+
 				/*Received MR is handled*/
 				return OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED;
 			}
@@ -2769,6 +3021,9 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 		/*all parts of the message succesfully delivered*/
 		if (delivery_successful) {
 			*relating_msg_id = status_report->ofono_msg_id;
+
+			pending_status_report_backup_free_id(status_report,
+								receiver, imsi);
 			free_pending_status_report(status_report,
 							pending_status_reports);
 
diff --git a/src/smsutil.h b/src/smsutil.h
index a809453..11481db 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -379,7 +379,7 @@ struct mr_number {
 
 struct pending_status_report {
 	unsigned int ofono_msg_id;
-	struct sms_address receiver;
+	struct sms_address *receiver;
 	GSList *related_mr_numbers;
 };
 
@@ -500,9 +500,12 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					guint16 ref, guint8 max, guint8 seq);
 void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
 
+GSList *pending_status_reports_load_backup(const char *imsi);
+
 void add_pending_status_report(GSList **pending_status_reports, const int mr,
 					const unsigned int msg_id,
-					const struct sms_address *receiver);
+					struct sms_address *receiver,
+					const char *imsi);
 
 void free_pending_status_report(struct pending_status_report *status_report,
 				GSList **pending_status_reports);
@@ -511,7 +514,7 @@ enum ofono_history_sms_status update_pending_status_report_mr_number(
 		GSList **pending_status_reports, const guint8 mr,
 		const struct sms_address *receiver,
 		const enum sms_status_report_result status_report_result,
-		unsigned int *relating_msg_id);
+		unsigned int *relating_msg_id, const char *imsi);
 
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
 				gboolean use_16bit, int *ref_offset,
-- 
1.6.0.4

Re: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Denis Kenzior]

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

Hi Pasi,

> ---
>  include/history.h |    2 +
>  src/sms.c         |   67 ++++++++++++++++++++----
>  src/smsutil.c     |  152
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h     
| 
>   32 +++++++++++

Please break this up into at least 3 patches:
- include/history api change
- smsutil changes
- sms core atom changes

Ideally I'd also like to see a unit test for smsutil parts in the series.
> +void add_pending_status_report(GSList **pending_status_reports, const int
>  mr, +					const unsigned int msg_id,
> +					const struct sms_address *receiver);
> +
> +void free_pending_status_report(struct pending_status_report
>  *status_report, +				GSList **pending_status_reports);
> +
> +enum ofono_history_sms_status update_pending_status_report_mr_number(
> +		GSList **pending_status_reports, const guint8 mr,
> +		const struct sms_address *receiver,
> +		const enum sms_status_report_result status_report_result,
> +		unsigned int *relating_msg_id);
> +

So my first suggestion here is to make the status report assembly into its own 
class.  This way you can mess around with the internal data structures and 
optimize them later if needed without breaking the rest of the code.

This is also where unit tests come in. Once you have something working, you 
can optimize and make sure things still work... without affecting the rest of 
the code.

I suggest thinking about the data structure use some more.  In particular, 
using a list of struct mr_numbers is very inefficient.  Using a pair of bitmaps 
for mr numbers (which can only be 0..255) would be way more efficient and 
probably would make the code much easier to write.

Consider if making multi-level data structures would increase efficiency.  E.g. 
Hastable[destination_address] = list of structures mapping between message id 
and message references.  Or anything else efficient you can come up with.

Having said that, don't over-complicate it in the beginning.  Having something 
good enough and working is better.

For the object interface I suggest something like:

struct status_report_assembly;

- Creates a new assembly object, and populates pending status reports from the 
imsi-keyed store.  If imsi is NULL, no status reports are saved or loaded from 
disk.  This is useful for pseudo-modems:
struct status_report_assembly *status_report_assembly_new(const char *imsi);

- Frees the assembly:
void status_report_assembly_free(struct status_report_assembly *assembly);

- Just a thought, but some networks simply don't support status reports, so we 
need to expire status reports for messages that have lived past their validity 
period:
void status_report_assembly_expire(struct status_report_assembly *assembly,
								time_t before, (GFunc) foreach_func,
								gpointer data);

- Add a status report to the assembly and return whether this resulted in a 
message being delivered / not:
gboolean status_report_assembly_report(struct status_report_assembly 
*assembly,
										const struct sms *status_report,
										unsigned int *msg_id,
										enum sms_st *status)

- Add a message fragment to the assembly for tracking:
void status_report_assembly_add_fragment(struct status_report_assembly 
*assembly,
										unsigned int *msg_id,
										struct sms_address *to,
										unsigned char mr,
										time_t expiration);

Feel free to modify the above as needed, this is just something I came up with 
while reviewing your patch.

Regards,
-Denis

VS: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Miettinen Pasi]

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

> Hi Pasi,
>
> >  ---
> >  include/history.h |    2 +
> >  src/sms.c         |   67 ++++++++++++++++++++----
> >  src/smsutil.c     |  152
> >  +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h
> |
> >   32 +++++++++++
>
|
>
> Having said that, don't over-complicate it in the beginning.  Having something
> good enough and working is better.
>

OK. Actually this already was my current way of thinking and I planned to tune
it up according to your comments. Thanks again for the adjusting comments.
I will continue working with the code on Monday.

Br,
-Pasi Miettinen

VS: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Miettinen Pasi]

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

Hi Denis,
> Hi Pasi,
>
> > ---
> >  include/history.h |    2 +
> >  src/sms.c         |   67 ++++++++++++++++++++----
> >  src/smsutil.c     |  152
> >  +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h
> |
> >   32 +++++++++++
|
> I suggest thinking about the data structure use some more.  In particular,
> using a list of struct mr_numbers is very inefficient.  Using a pair of bitmaps
> for mr numbers (which can only be 0..255) would be way more efficient and
> probably would make the code much easier to write.

I am not sure if I am quite getting what do you mean. Do you mean something
like:
struct mr_numbers {
        unsigned int mr1 : 8,
        unsigned int mr2 : 8,
        unsigned int mr3 : 8,
        unsigned int mr4 : 8
};
So there wouldn't be so much padding bits or what's your angle here?
If that is it, how to indicate the status of the mr (enum sms_status_report_result)?

Br,
Pasi

VS: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Miettinen Pasi]

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

Hi Denis,
> > Hi Pasi,
> >
> > > ---
> > >  include/history.h |    2 +
> > >  src/sms.c         |   67 ++++++++++++++++++++----
> > >  src/smsutil.c     |  152
> > >  +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h
> > |
> > >   32 +++++++++++
> |
> > I suggest thinking about the data structure use some more.  In particular,
> > using a list of struct mr_numbers is very inefficient.  Using a pair of bitmaps
> > for mr numbers (which can only be 0..255) would be way more efficient and
> > probably would make the code much easier to write.
> 
> I am not sure if I am quite getting what do you mean. Do you mean something
> like:
|
> Br,
> Pasi

OK, now I noticed that there's some bitmapping done in sms_assembly and
now I am beginning to get at least some kind of sense.

Br,
Pasi

Re: VS: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Denis Kenzior]

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

Hi Pasi,

> > I suggest thinking about the data structure use some more.  In
> > particular, using a list of struct mr_numbers is very inefficient.  Using
> > a pair of bitmaps for mr numbers (which can only be 0..255) would be way
> > more efficient and probably would make the code much easier to write.
> 
> I am not sure if I am quite getting what do you mean. Do you mean something
> like:
> struct mr_numbers {
>         unsigned int mr1 : 8,
>         unsigned int mr2 : 8,
>         unsigned int mr3 : 8,
>         unsigned int mr4 : 8
> };

No, I mean something like:
	unsigned int needed_mrs[8];
	unsigned int obtained_mrs[8];

where each set bit represents a number at that particular offset.  See how cbs 
/ sms assemblies do this.

Regards,
-Denis

Re: [RFC PATCH 4/6] SetProperty for UseDeliveryReports.

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/sms.c       |   34 +++++++++++++++++++++++++++++++++-
>  src/smsutil.c   |    5 +++--
>  src/smsutil.h   |    3 ++-
>  test/send-sms   |    7 ++++++-
>  unit/test-sms.c |   10 +++++-----

Please break this up into a series of 4 patches:
1. smsutil changes
2. unit test changes
3. sms core atom changes
4. unit test changes.

> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -67,6 +67,7 @@ struct ofono_sms {
>  	const struct ofono_sms_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	gboolean use_delivery_reports;

Use ofono_bool_t as the type here

>  };
> 
> +	if (!strcmp(property, "UseDeliveryReports")) {
> +		const char *path = __ofono_atom_get_path(sms->atom);
> +		gboolean value;

use dbus_bool_t as the type as it is more type-correct.

>  GSList *sms_text_prepare(const char *utf8, guint16 ref,
> -				gboolean use_16bit, int *ref_offset)
> +				gboolean use_16bit, int *ref_offset,
> +				const gboolean use_delivery_reports)

For basic integer types, no need to declare them const.

Regards,
-Denis

Re: [RFC PATCH 3/6] Support for basic SMS Status Report.

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/sms.c |  110
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 
1 files
>  changed, 109 insertions(+), 1 deletions(-)
> 
> diff --git a/src/sms.c b/src/sms.c
> index 618e035..14748c6 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -553,6 +553,75 @@ static void dispatch_text_message(struct ofono_sms
>  *sms, }
>  }
> 
> +static void dispatch_sms_delivery_report(struct ofono_sms *sms,
> +					const enum sms_st *st,
> +					const struct sms_address *raddr,
> +					const struct sms_scts *scts,
> +					const struct sms_scts *dt)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sms->atom);
> +	DBusMessage *signal;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +	char buf[128];
> +	time_t ts;
> +	struct tm remote;
> +	struct tm local;
> +	const char *str = buf;
> +
> +	if (!st) {
> +		DBG("status unavailable");
> +		return;
> +	}
> +
> +	signal = dbus_message_new_signal(path, OFONO_SMS_MANAGER_INTERFACE,
> +						"IncomingStatusReport");
> +
> +	if (!signal)
> +		return;
> +
> +	/*Start assembling dbus-message*/
> +	dbus_message_iter_init_append(signal, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +						&dict);
> +
> +	/*This is the time when sender sent the message*/
> +	ts = sms_scts_to_time(scts, &remote);
> +	localtime_r(&ts, &local);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "LocalSentTime", DBUS_TYPE_STRING, &str);
> +
> +	/*This is the time when the message was delivered to the recipient*/
> +	ts = sms_scts_to_time(dt, &remote);
> +	localtime_r(&ts, &local);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "LocalDeliverTime",
> +					DBUS_TYPE_STRING, &str);
> +
> +	/*Status*/
> +	if (*st == 0x00) {
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "delivered",
> +					DBUS_TYPE_STRING, &str);
> +	} else {
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "undeliverable",
> +					DBUS_TYPE_STRING, &str);
> +	}
> +
> +	/*dbus-message assembled*/
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	g_dbus_send_message(conn, signal);
> +}
> +

We tend to do things bottom up here, so this patch should really come last and 
merged with changes from Patch 5.

Regards,
-Denis

Re: [RFC PATCH 2/6] Add IncomingStatusReport a{sv} signal.

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/sms.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/sms.c b/src/sms.c
> index c0e9fc4..618e035 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -472,9 +472,10 @@ static GDBusMethodTable sms_manager_methods[] = {
>  };
> 
>  static GDBusSignalTable sms_manager_signals[] = {
> -	{ "PropertyChanged",	"sv"		},
> -	{ "IncomingMessage",	"sa{sv}"	},
> -	{ "ImmediateMessage",	"sa{sv}"	},
> +	{ "PropertyChanged",		"sv"		},
> +	{ "IncomingMessage",		"sa{sv}"	},
> +	{ "ImmediateMessage",		"sa{sv}"	},
> +	{ "IncomingStatusReport",	"a{sv}"		},
>  	{ }
>  };
> 

IncomingStatusReport signal is the wrong approach, we need to actually 
associate a status report with a message.  This will be done in two ways:
	- Via sms history
	- Via sms message D-Bus interface.  See the "See / Cancel pending..." task in 
TODO

Until that task is done, I would only go through the history plugin.  Feel 
free to modify the example history to simply print the delivery status.

Regards,
-Denis

Re: [RFC PATCH 1/6] Change in at_cds_notify for status report.

[Denis Kenzior]

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

Hi Pasi,

> ---
>  drivers/atmodem/sms.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
> index 439beea..3dafbc9 100644
> -	DBG("Got new Status-Report PDU via CDS: %s, %d", pdu, pdulen);
> +	if (strlen(hexpdu) > sizeof(pdu) * 2) {
> +		ofono_error("Bad PDU length in CMT notification");
> +		return;
> +	}

Watch out for copy-paste mistakes.  Fixed it for you.

Patch has been applied, thanks.

Regards,
-Denis

VS: Next series of delivery report patches

[Miettinen Pasi]

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

> Patches include:
> 1. Notify about new status report to sms-core.
> 2. Add new signal to DBus-interface for status report.
> 3. Support for basic status report (non-concatenated).
>    -function structure is similar to deliver-notify,
>     some functions could be combined
> 4. Implement setting "UseDeliveryReports" property via DBus.
> 5. Support for concatenated status reports.
>    -add_pending_status_report() relates new MR-number to
>     existing ofono_msg_id or creates a new ofono_msg_id
>     and relates MR to it. Also receiver information is saved.
>    -update_pending_status_report_mr_number() searches the
>     msg_id to which the received MR is related and updates
>     the status of the msg_id and MR accordingly. If message
>     is determined as undelivered, related information is
>     freed.
> 6. Saving pending SMS status reports to disk.
>    -same basic idea as in SMS assembly storaging
>    -function structure is similar to SMS assembly part.
>     Status report part only has one more loop compared to it
>     because of the deeper directory structure in which the
>     pending status report information is saved.
>    -storaging and storage freeing are called by
>     add_pending_status_report() and
>     update_pending_status_report_mr_number()
>
> Next tasks:
> 1. Combine ofono_sms_deliver_notify() and ofono_sms_status_notify()
>    as Aki Niemi requested. Still a valid task?
> 2. Implement received comments about these patches.
>
> Br,
> Pasi Miettinen

Hmm, my starting paragraph disappeared to somewhere while sending,
but here it is:

I have been implementing your comments considering my previous
patches and hopefully these are a step in the right direction.
I checked the patches with checkpatch.pl and only error I get
is "ERROR: Missing Signed-off-by". Patches 5 and 6 have a little bit
more stuff on them. I tried to include comments to critical
code points, but if something is unclear, please ask and I
try to clarify my intentions.

Br,
Pasi