[RFC PATCH 2/5] history: print SMS status

[RFC PATCH 2/5] history: print SMS status

[Pasi Miettinen]

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

---
 src/history.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/src/history.c b/src/history.c
index f868ca2..0eef130 100644
--- a/src/history.c
+++ b/src/history.c
@@ -238,6 +238,8 @@ void __ofono_history_sms_send_status(struct ofono_modem *modem,
 					enum ofono_history_sms_status status)
 {
 	struct history_sms_foreach_data hfd;
+	struct tm  *ts;
+	char buf[80];
 
 	hfd.msg_id = msg_id;
 	hfd.address = NULL;
@@ -245,6 +247,15 @@ void __ofono_history_sms_send_status(struct ofono_modem *modem,
 	hfd.when = when;
 	hfd.status = status;
 
+	/* Format time, "ddd yyyy-mm-dd hh:mm:ss zzz" */
+	ts = localtime(&when);
+	strftime(buf, sizeof(buf), "%a %Y-%m-%d %H:%M:%S %Z", ts);
+
+	if (status == OFONO_HISTORY_SMS_STATUS_DELIVERED)
+		DBG("SMS delivered, msg_id: %i, time: %s", msg_id, buf);
+	else if (status == OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED)
+		DBG("SMS undeliverable, msg_id: %i, time: %s", msg_id, buf);
+
 	__ofono_modem_foreach_atom(modem, OFONO_ATOM_TYPE_HISTORY,
 					history_sms_send_status, &hfd);
 }
-- 
1.6.0.4

SMS status report

[Pasi Miettinen]

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


Hi,

Here are the latest modifications for the status report.

Most common use cases are tested in practise. For example:
-Send concatenated message from ofono to phone that is
 powered off.
-Reboot ofono and ofono reads the persistent data from imsi file.
-Power on the phone.
-Ofono receives the status reports and handles them correctly.

Ofono was run under the supervision of valgrind during these tests
and no memory leaks were detected.

Br,
Pasi

[RFC PATCH 1/5] smsutil: Status report assembly

[Pasi Miettinen]

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

---
 src/smsutil.c |  183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h |   31 ++++++++++
 2 files changed, 214 insertions(+), 0 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 95eca06..3db3d28 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2625,6 +2625,189 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
 	}
 }
 
+struct status_report_assembly *status_report_assembly_new(const char *imsi)
+{
+	struct status_report_assembly *ret =
+				g_new0(struct status_report_assembly, 1);
+
+	ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
+				g_free, (GDestroyNotify)g_hash_table_destroy);
+
+	if (imsi)
+		ret->imsi = imsi;
+
+	return ret;
+}
+
+void status_report_assembly_free(struct status_report_assembly *assembly)
+{
+	g_hash_table_destroy(assembly->assembly_table);
+	g_free(assembly);
+}
+
+gboolean status_report_assembly_report(struct status_report_assembly *assembly,
+					const struct sms *status_report,
+					unsigned int *msg_id,
+					gboolean *msg_delivered)
+{
+	unsigned int offset = status_report->status_report.mr / 32;
+	unsigned int bit = 1 << (status_report->status_report.mr % 32);
+	GHashTable *id_table;
+	struct id_table_node *node;
+	unsigned int *key;
+	gpointer value;
+	GHashTableIter iter;
+	int i;
+	gboolean pending = FALSE;
+	gboolean update_history = FALSE;
+
+	id_table = g_hash_table_lookup(assembly->assembly_table,
+				status_report->status_report.raddr.address);
+
+	/* ERROR, key (receiver address) does not exist in assembly */
+	if (!id_table) {
+		*msg_delivered = FALSE;
+		return FALSE;
+	}
+
+	g_hash_table_iter_init(&iter, id_table);
+	while (g_hash_table_iter_next(&iter, (gpointer)&key, &value)) {
+		node = value;
+
+		if (!(node->mrs[offset] & bit))
+			continue;
+
+		/* Mr belongs to this node. */
+		node->mrs[offset] ^= bit;
+		*msg_id = *key;
+
+		for (i = 0; i < 8; i++) {
+				/* There are still pending mr(s). */
+				if (node->mrs[i] != 0 ||
+					(node->sent_mrs < node->total_mrs)) {
+					pending = TRUE;
+					break;
+				}
+		}
+		/* Mr is not delivered. */
+		if (status_report->status_report.st !=
+				SMS_ST_COMPLETED_RECEIVED) {
+			/* First mr which is not delivered. Update ofono history
+			 * and mark the whole message as undeliverable. Upcoming
+			 * mrs can not change the status to deliverable even if
+			 * they are considered as delivered.
+			 */
+			if (node->deliverable) {
+				node->deliverable = FALSE;
+				update_history = TRUE;
+			}
+		}
+
+		/* If there are pending mrs that relate to this message, we do
+		 * not delete the node yet.
+		 */
+		if (pending) {
+			*msg_delivered = FALSE;
+			return update_history;
+		} else {
+			*msg_delivered = node->deliverable;
+
+			g_hash_table_iter_remove(&iter);
+
+			if (g_hash_table_size(id_table) == 0)
+				g_hash_table_remove(assembly->assembly_table,
+				status_report->status_report.raddr.address);
+			/* If there has not been undelivered mrs, message is
+			 * delivered and the ofono history needs to be updated.
+			 * If the message is concidered as undelivered, the
+			 * ofono history has already been updated when the first
+			 * undelivered mr arrived, unless this one is the only
+			 * related mr and was marked undelivered.
+			 */
+			return *msg_delivered || update_history;
+		}
+	}
+	/* ERROR, mr not found. */
+	*msg_delivered = FALSE;
+	return FALSE;
+}
+
+void status_report_assembly_add_fragment(
+					struct status_report_assembly *assembly,
+					unsigned int msg_id,
+					const struct sms_address *to,
+					unsigned char mr, time_t expiration,
+					unsigned char total_mrs)
+{
+	unsigned int offset = mr / 32;
+	unsigned int bit = 1 << (mr % 32);
+	GHashTable *id_table;
+	struct id_table_node *node;
+	char *assembly_table_key;
+	unsigned int *id_table_key;
+
+	id_table = g_hash_table_lookup(assembly->assembly_table, to->address);
+	/* Create id_table and node */
+	if (id_table == NULL) {
+		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
+								g_free, g_free);
+		id_table_key = g_new0(unsigned int, 1);
+
+		node = g_new0(struct id_table_node, 1);
+		node->to = *to;
+		node->mrs[offset] |= bit;
+		node->expiration = expiration;
+		node->total_mrs = total_mrs;
+		node->sent_mrs = 1;
+		node->deliverable = TRUE;
+
+		*id_table_key = msg_id;
+		g_hash_table_insert(id_table, id_table_key, node);
+
+		assembly_table_key = g_try_malloc(sizeof(to->address));
+
+		if (assembly_table_key == NULL) {
+			g_free(node);
+			return;
+		}
+
+		g_strlcpy(assembly_table_key, to->address, sizeof(to->address));
+
+		g_hash_table_insert(assembly->assembly_table,
+					assembly_table_key, id_table);
+		return;
+	}
+
+	node = g_hash_table_lookup(id_table, &msg_id);
+	/* id_table exists, create new node */
+	if (node == NULL) {
+		id_table_key = g_new0(unsigned int, 1);
+		node = g_new0(struct id_table_node, 1);
+		node->to = *to;
+		node->mrs[offset] |= bit;
+		node->expiration = expiration;
+		node->total_mrs = total_mrs;
+		node->sent_mrs = 1;
+		node->deliverable = TRUE;
+
+		*id_table_key = msg_id;
+		g_hash_table_insert(id_table, id_table_key, node);
+
+		return;
+	}
+	/* id_table and node both exists */
+	node->mrs[offset] |= bit;
+	node->expiration = expiration;
+	node->sent_mrs++;
+}
+
+void status_report_assembly_expire(struct status_report_assembly *assembly,
+					time_t before, GFunc foreach_func,
+					gpointer data)
+{
+	/*TODO*/
+}
+
 static inline GSList *sms_list_append(GSList *l, const struct sms *in)
 {
 	struct sms *sms;
diff --git a/src/smsutil.h b/src/smsutil.h
index 1bd42bb..5fb187b 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -19,6 +19,8 @@
  *
  */
 
+#include "history.h"
+
 #define CBS_MAX_GSM_CHARS 93
 
 enum sms_type {
@@ -364,6 +366,20 @@ struct sms_assembly {
 	GSList *assembly_list;
 };
 
+struct id_table_node {
+	struct sms_address to;
+	unsigned int mrs[8];
+	time_t expiration;
+	unsigned char total_mrs;
+	unsigned char sent_mrs;
+	gboolean deliverable;
+};
+
+struct status_report_assembly {
+	const char *imsi;
+	GHashTable *assembly_table;
+};
+
 struct cbs {
 	enum cbs_geo_scope gs;			/* 2 bits */
 	guint16 message_code;			/* 10 bits */
@@ -481,6 +497,21 @@ 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);
 
+struct status_report_assembly *status_report_assembly_new(const char *imsi);
+void status_report_assembly_free(struct status_report_assembly *assembly);
+gboolean status_report_assembly_report(struct status_report_assembly *assembly,
+					const struct sms *status_report,
+					unsigned int *msg_id,
+					gboolean *msg_delivered);
+void status_report_assembly_add_fragment(struct status_report_assembly
+					*assembly, unsigned int msg_id,
+					const struct sms_address *to,
+					unsigned char mr, time_t expiration,
+					unsigned char total_mrs);
+void status_report_assembly_expire(struct status_report_assembly *assembly,
+					time_t before, GFunc foreach_func,
+					gpointer data);
+
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
 				gboolean use_16bit, int *ref_offset,
 				gboolean use_delivery_reports);
-- 
1.6.0.4

[RFC PATCH 2/5] history: print SMS status

[Pasi Miettinen]

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

---
 src/history.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/src/history.c b/src/history.c
index f868ca2..0eef130 100644
--- a/src/history.c
+++ b/src/history.c
@@ -238,6 +238,8 @@ void __ofono_history_sms_send_status(struct ofono_modem *modem,
 					enum ofono_history_sms_status status)
 {
 	struct history_sms_foreach_data hfd;
+	struct tm  *ts;
+	char buf[80];
 
 	hfd.msg_id = msg_id;
 	hfd.address = NULL;
@@ -245,6 +247,15 @@ void __ofono_history_sms_send_status(struct ofono_modem *modem,
 	hfd.when = when;
 	hfd.status = status;
 
+	/* Format time, "ddd yyyy-mm-dd hh:mm:ss zzz" */
+	ts = localtime(&when);
+	strftime(buf, sizeof(buf), "%a %Y-%m-%d %H:%M:%S %Z", ts);
+
+	if (status == OFONO_HISTORY_SMS_STATUS_DELIVERED)
+		DBG("SMS delivered, msg_id: %i, time: %s", msg_id, buf);
+	else if (status == OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED)
+		DBG("SMS undeliverable, msg_id: %i, time: %s", msg_id, buf);
+
 	__ofono_modem_foreach_atom(modem, OFONO_ATOM_TYPE_HISTORY,
 					history_sms_send_status, &hfd);
 }
-- 
1.6.0.4

[RFC PATCH 3/5] history: API change for status report notify

[Pasi Miettinen]

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

---
 include/history.h |    2 ++
 1 files changed, 2 insertions(+), 0 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 {
-- 
1.6.0.4

[RFC PATCH 4/5] sms: Status report notify

[Pasi Miettinen]

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

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

diff --git a/src/sms.c b/src/sms.c
index bf6d261..e6a2dca 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -68,6 +68,8 @@ struct ofono_sms {
 	void *driver_data;
 	struct ofono_atom *atom;
 	ofono_bool_t use_delivery_reports;
+	struct status_report_assembly *sr_assembly;
+
 };
 
 struct pending_pdu {
@@ -83,6 +85,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 +335,13 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	entry->cur_pdu += 1;
 	entry->retry = 0;
 
+	if (entry->status_report)
+		status_report_assembly_add_fragment(sms->sr_assembly,
+							entry->msg_id,
+							&entry->receiver,
+							mr, time(NULL),
+							entry->num_pdus);
+
 	if (entry->cur_pdu < entry->num_pdus) {
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 		return;
@@ -462,6 +473,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 +487,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);
 
@@ -718,6 +732,30 @@ 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)
+{
+	gboolean delivered;
+	unsigned int msg_id;
+	gboolean update_history;
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
+
+	update_history = status_report_assembly_report(sms->sr_assembly,
+						incoming, &msg_id, &delivered);
+
+	if (update_history) {
+
+		if (delivered)
+			__ofono_history_sms_send_status(modem, msg_id,
+				time(NULL), OFONO_HISTORY_SMS_STATUS_DELIVERED);
+		else
+			__ofono_history_sms_send_status(modem, msg_id,
+				time(NULL),
+				OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);
+	}
+}
+
+
 static inline gboolean handle_mwi(struct ofono_sms *sms, struct sms *s)
 {
 	gboolean discard;
@@ -849,7 +887,30 @@ 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 (s.status_report.srq) {
+		ofono_error("Waiting an answer to SMS-SUBMIT, not SMS-COMMAND");
+		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)
@@ -932,6 +993,11 @@ static void sms_remove(struct ofono_atom *atom)
 		sms->settings = NULL;
 	}
 
+	if (sms->sr_assembly) {
+		status_report_assembly_free(sms->sr_assembly);
+		sms->sr_assembly = NULL;
+	}
+
 	g_free(sms);
 }
 
@@ -1069,9 +1135,12 @@ void ofono_sms_register(struct ofono_sms *sms)
 		imsi = ofono_sim_get_imsi(sms->sim);
 		sms->assembly = sms_assembly_new(imsi);
 
+		sms->sr_assembly = status_report_assembly_new(imsi);
+
 		sms_load_settings(sms, imsi);
 	} else {
 		sms->assembly = sms_assembly_new(NULL);
+		sms->sr_assembly = status_report_assembly_new(NULL);
 	}
 
 	__ofono_atom_register(sms->atom, sms_unregister);
-- 
1.6.0.4

[RFC PATCH 5/5] smsutil: save pending status reports to imsi file

[Pasi Miettinen]

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

---
 src/smsutil.c |  205 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 204 insertions(+), 1 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 3db3d28..a30a281 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"
+
 #define SMS_ADDR_FMT "%24[0-9A-F]"
 
 static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
@@ -2625,20 +2629,209 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
 	}
 }
 
+static void sr_assembly_load_backup(GHashTable *assembly_table,
+					const char *imsi,
+					const struct dirent *addr_dir)
+{
+	char *path;
+	struct dirent **ids;
+	struct sms_address *addr;
+	struct id_table_node *node;
+	GHashTable *id_table;
+	int len;
+	int r;
+	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
+			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
+	char *assembly_table_key;
+	unsigned int *id_table_key;
+	struct stat segment_stat;
+
+	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) {
+		g_free(addr);
+		return;
+	}
+
+	/* Go through different msg_ids. */
+	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) {
+		g_free(addr);
+		return;
+	}
+
+	id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
+							g_free, g_free);
+
+	assembly_table_key = g_try_malloc(sizeof(addr->address));
+
+	if (assembly_table_key == NULL) {
+		g_free(addr);
+		return;
+	}
+
+	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
+	g_hash_table_insert(assembly_table, assembly_table_key, id_table);
+
+	while (len--) {
+		path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
+				imsi, addr_dir->d_name, ids[len]->d_name);
+		r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH "/%s/%s",
+				imsi, addr_dir->d_name, ids[len]->d_name);
+
+		if (r < 0) {
+			g_free(path);
+			g_free(ids[len]);
+			continue;
+		}
+
+		r = stat(path, &segment_stat);
+
+		if (r != 0) {
+			g_free(path);
+			g_free(ids[len]);
+			continue;
+		}
+		/* Gather the data for id_table node */
+		node = g_new0(struct id_table_node, 1);
+		node->to = *addr;
+		node->expiration = segment_stat.st_mtime;
+		memcpy(node->mrs, buf, sizeof(node->mrs));
+		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
+						sizeof(node->total_mrs));
+		memcpy(&node->sent_mrs,
+			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
+			sizeof(node->sent_mrs));
+
+		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
+			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
+			sizeof(node->deliverable));
+		/* Node ready, create key and add them to the table */
+		id_table_key = g_new0(unsigned int, 1);
+		*id_table_key = atoi(ids[len]->d_name);
+
+		g_hash_table_insert(id_table, id_table_key, node);
+
+		g_free(path);
+		g_free(ids[len]);
+	}
+	g_free(addr);
+	g_free(ids);
+}
+
 struct status_report_assembly *status_report_assembly_new(const char *imsi)
 {
+	char *path;
+	int len;
+	struct dirent **addresses;
 	struct status_report_assembly *ret =
 				g_new0(struct status_report_assembly, 1);
 
 	ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
 				g_free, (GDestroyNotify)g_hash_table_destroy);
 
-	if (imsi)
+	if (imsi) {
 		ret->imsi = 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.
+		 */
+		g_free(addresses[0]);
+		g_free(addresses[1]);
+
+		while (2 < len--) {
+			sr_assembly_load_backup(ret->assembly_table, imsi,
+								addresses[len]);
+			g_free(addresses[len]);
+		}
+		g_free(addresses);
+	}
 	return ret;
 }
 
+static gboolean sr_assembly_add_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					unsigned int msg_id)
+{
+	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
+			sizeof(node->sent_mrs) + sizeof(node->deliverable);
+	unsigned char buf[len];
+
+	if (!imsi)
+		return FALSE;
+
+	memcpy(buf, node->mrs, sizeof(node->mrs));
+
+	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
+					sizeof(node->total_mrs));
+
+	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
+				&node->sent_mrs, sizeof(node->sent_mrs));
+
+	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
+	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
+
+	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
+	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
+			node->to.address, node->to.number_type,
+			node->to.numbering_plan, msg_id) != len)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					unsigned int msg_id)
+{
+	char *path;
+
+	if (!imsi)
+		return FALSE;
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
+				node->to.number_type, node->to.numbering_plan,
+				msg_id);
+
+	unlink(path);
+	g_free(path);
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, node->to.address,
+				node->to.number_type, node->to.numbering_plan);
+
+	/* If the address does not have relating msg_ids anymore, remove it */
+	rmdir(path);
+	g_free(path);
+
+	return TRUE;
+}
+
+static gboolean sr_assembly_update_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					unsigned int msg_id)
+{
+	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
+			sr_assembly_add_fragment_backup(imsi, node, msg_id);
+}
+
 void status_report_assembly_free(struct status_report_assembly *assembly)
 {
 	g_hash_table_destroy(assembly->assembly_table);
@@ -2707,10 +2900,14 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 		 * not delete the node yet.
 		 */
 		if (pending) {
+			sr_assembly_update_fragment_backup(assembly->imsi, node,
+								*msg_id);
 			*msg_delivered = FALSE;
 			return update_history;
 		} else {
 			*msg_delivered = node->deliverable;
+			sr_assembly_remove_fragment_backup(assembly->imsi, node,
+								*msg_id);
 
 			g_hash_table_iter_remove(&iter);
 
@@ -2747,6 +2944,7 @@ void status_report_assembly_add_fragment(
 	unsigned int *id_table_key;
 
 	id_table = g_hash_table_lookup(assembly->assembly_table, to->address);
+
 	/* Create id_table and node */
 	if (id_table == NULL) {
 		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
@@ -2775,6 +2973,9 @@ void status_report_assembly_add_fragment(
 
 		g_hash_table_insert(assembly->assembly_table,
 					assembly_table_key, id_table);
+
+		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
+
 		return;
 	}
 
@@ -2793,12 +2994,14 @@ void status_report_assembly_add_fragment(
 		*id_table_key = msg_id;
 		g_hash_table_insert(id_table, id_table_key, node);
 
+		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
 		return;
 	}
 	/* id_table and node both exists */
 	node->mrs[offset] |= bit;
 	node->expiration = expiration;
 	node->sent_mrs++;
+	sr_assembly_update_fragment_backup(assembly->imsi, node, msg_id);
 }
 
 void status_report_assembly_expire(struct status_report_assembly *assembly,
-- 
1.6.0.4

Re: [RFC PATCH 5/5] smsutil: save pending status reports to imsi file

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/smsutil.c |  205
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files 
changed,
>  204 insertions(+), 1 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 3db3d28..a30a281 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"
> +
>  #define SMS_ADDR_FMT "%24[0-9A-F]"
> 
>  static GSList *sms_assembly_add_fragment_backup(struct sms_assembly
>  *assembly, @@ -2625,20 +2629,209 @@ void sms_assembly_expire(struct
>  sms_assembly *assembly, time_t before) }
>  }
> 
> +static void sr_assembly_load_backup(GHashTable *assembly_table,
> +					const char *imsi,
> +					const struct dirent *addr_dir)
> +{
> +	char *path;
> +	struct dirent **ids;
> +	struct sms_address *addr;
> +	struct id_table_node *node;
> +	GHashTable *id_table;
> +	int len;
> +	int r;
> +	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
> +			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
> +	char *assembly_table_key;
> +	unsigned int *id_table_key;
> +	struct stat segment_stat;
> +
> +	if (addr_dir->d_type != DT_DIR)
> +		return;
> +
> +	addr = g_new0(struct sms_address, 1);

Why are you mallocing addr here? Seems that this only makes the code more 
complicated.

> +
> +	if (sscanf(addr_dir->d_name, "%[0-9]-%i-%i",
> +			addr->address, (int *) &addr->number_type,
> +			(int *) &addr->numbering_plan) < 3) {

Lets make this consistent with sms assembly, please use SMS_ADDR_FMT.  You're 
also free to use sms_assembly utility functions if that helps you.

> +		g_free(addr);
> +		return;
> +	}
> +
> +	/* Go through different msg_ids. */
> +	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) {
> +		g_free(addr);
> +		return;
> +	}
> +
> +	id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> +							g_free, g_free);
> +
> +	assembly_table_key = g_try_malloc(sizeof(addr->address));
> +
> +	if (assembly_table_key == NULL) {
> +		g_free(addr);
> +		return;
> +	}
> +
> +	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
> +	g_hash_table_insert(assembly_table, assembly_table_key, id_table);
> +
> +	while (len--) {
> +		path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
> +				imsi, addr_dir->d_name, ids[len]->d_name);
> +		r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH "/%s/%s",
> +				imsi, addr_dir->d_name, ids[len]->d_name);
> +
> +		if (r < 0) {
> +			g_free(path);
> +			g_free(ids[len]);
> +			continue;
> +		}
> +
> +		r = stat(path, &segment_stat);
> +
> +		if (r != 0) {
> +			g_free(path);
> +			g_free(ids[len]);
> +			continue;
> +		}
> +		/* Gather the data for id_table node */
> +		node = g_new0(struct id_table_node, 1);
> +		node->to = *addr;

Please use memcpy for copying structures.

> +		node->expiration = segment_stat.st_mtime;
> +		memcpy(node->mrs, buf, sizeof(node->mrs));
> +		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> +						sizeof(node->total_mrs));
> +		memcpy(&node->sent_mrs,
> +			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> +			sizeof(node->sent_mrs));
> +
> +		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
> +			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
> +			sizeof(node->deliverable));
> +		/* Node ready, create key and add them to the table */
> +		id_table_key = g_new0(unsigned int, 1);
> +		*id_table_key = atoi(ids[len]->d_name);
> +
> +		g_hash_table_insert(id_table, id_table_key, node);
> +
> +		g_free(path);
> +		g_free(ids[len]);
> +	}
> +	g_free(addr);
> +	g_free(ids);
> +}
> +

General comment here is that you're duplicating much of the code that inserts 
the information into the data structure.  You can play the same trick as 
sms_assembly which uses a single function to do the heavy lifting, but takes 
an argument whether to store this info on disk.  Obviously during load you 
don't want to re-save this information on disk.  Then your load function 
becomes much smaller and easier to understand.

>  struct status_report_assembly *status_report_assembly_new(const char
>  *imsi) {
> +	char *path;
> +	int len;
> +	struct dirent **addresses;
>  	struct status_report_assembly *ret =
>  				g_new0(struct status_report_assembly, 1);
> 
>  	ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
>  				g_free, (GDestroyNotify)g_hash_table_destroy);
> 
> -	if (imsi)
> +	if (imsi) {
>  		ret->imsi = 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;

There should be an empty line here

> +		/* Go through different addresses. Each address can relate to
> +		 * 1-n msg_ids. Do not try to load . and .. directories.
> +		 */
> +		g_free(addresses[0]);
> +		g_free(addresses[1]);

This really isn't necessary, the '.' and '..' directories will not pass the 
sscanf test above.

> +
> +		while (2 < len--) {
> +			sr_assembly_load_backup(ret->assembly_table, imsi,
> +								addresses[len]);
> +			g_free(addresses[len]);
> +		}

Empty line here please

> +		g_free(addresses);
> +	}

And another empty line here.  Please put an empty line after each block unless 
it is at the end of the function.
>  	return ret;
>  }
> 
> +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)

Lets be consistent with how sms assembly works.  E.g. take the main assembly 
object and return void.  Just helps to read the code when it is consistent.

> +{
> +	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
> +			sizeof(node->sent_mrs) + sizeof(node->deliverable);
> +	unsigned char buf[len];
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	memcpy(buf, node->mrs, sizeof(node->mrs));
> +
> +	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
> +					sizeof(node->total_mrs));
> +
> +	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> +				&node->sent_mrs, sizeof(node->sent_mrs));
> +
> +	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
> +	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
> +
> +	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
> +	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
> +			node->to.address, node->to.number_type,
> +			node->to.numbering_plan, msg_id) != len)
> +		return FALSE;
> +
> +	return TRUE;
> +}
> +
> +static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)

Same consistency comment here.  Take main assembly object and return void.

> +{
> +	char *path;
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
> +				node->to.number_type, node->to.numbering_plan,
> +				msg_id);
> +
> +	unlink(path);
> +	g_free(path);
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, node->to.address,
> +				node->to.number_type, node->to.numbering_plan);
> +
> +	/* If the address does not have relating msg_ids anymore, remove it */
> +	rmdir(path);
> +	g_free(path);
> +
> +	return TRUE;
> +}
> +
> +static gboolean sr_assembly_update_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)
> +{
> +	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
> +			sr_assembly_add_fragment_backup(imsi, node, msg_id);
> +}
> +

Is this function really necessary? write_file overwrites the file, so simply 
using add_fragment_backup seems sufficient.

>  void status_report_assembly_free(struct status_report_assembly *assembly)
>  {
>  	g_hash_table_destroy(assembly->assembly_table);
> @@ -2707,10 +2900,14 @@ gboolean status_report_assembly_report(struct
>  status_report_assembly *assembly, * not delete the node yet.
>  		 */
>  		if (pending) {
> +			sr_assembly_update_fragment_backup(assembly->imsi, node,
> +								*msg_id);

It seems to me fragment is the wrong name here.  What about 
'store_node_backup'?

>  			*msg_delivered = FALSE;
>  			return update_history;
>  		} else {
>  			*msg_delivered = node->deliverable;
> +			sr_assembly_remove_fragment_backup(assembly->imsi, node,
> +								*msg_id);

maybe 'remove_node_backup'

> 
>  			g_hash_table_iter_remove(&iter);
> 
> @@ -2747,6 +2944,7 @@ void status_report_assembly_add_fragment(
>  	unsigned int *id_table_key;
> 
>  	id_table = g_hash_table_lookup(assembly->assembly_table, to->address);
> +
>  	/* Create id_table and node */
>  	if (id_table == NULL) {
>  		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> @@ -2775,6 +2973,9 @@ void status_report_assembly_add_fragment(
> 
>  		g_hash_table_insert(assembly->assembly_table,
>  					assembly_table_key, id_table);
> +
> +		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
> +
>  		return;
>  	}
> 
> @@ -2793,12 +2994,14 @@ void status_report_assembly_add_fragment(
>  		*id_table_key = msg_id;
>  		g_hash_table_insert(id_table, id_table_key, node);
> 
> +		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
>  		return;
>  	}
>  	/* id_table and node both exists */
>  	node->mrs[offset] |= bit;
>  	node->expiration = expiration;
>  	node->sent_mrs++;
> +	sr_assembly_update_fragment_backup(assembly->imsi, node, msg_id);
>  }
> 
>  void status_report_assembly_expire(struct status_report_assembly
>  *assembly,
> 

Regards,
-Denis

Re: [RFC PATCH 1/5] smsutil: Status report assembly

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/smsutil.c |  183
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
src/smsutil.h | 
>   31 ++++++++++
>  2 files changed, 214 insertions(+), 0 deletions(-)
> 

I pushed patches 1, 3 and 4 in this series upstream and refactored them.  
Mostly just the code-flow was tweaked and made easier to follow.

I'm happy to report status reports are now working for me on T-Mobile USA & 
mbm.

Regards,
-Denis 

Re: [RFC PATCH 2/5] history: print SMS status

[Denis Kenzior]

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

Hi Pasi,

> ---
>  src/history.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/src/history.c b/src/history.c
> index f868ca2..0eef130 100644
> --- a/src/history.c
> +++ b/src/history.c
> @@ -238,6 +238,8 @@ void __ofono_history_sms_send_status(struct ofono_modem
>  *modem, enum ofono_history_sms_status status)
>  {
>  	struct history_sms_foreach_data hfd;
> +	struct tm  *ts;
> +	char buf[80];
> 
>  	hfd.msg_id = msg_id;
>  	hfd.address = NULL;
> @@ -245,6 +247,15 @@ void __ofono_history_sms_send_status(struct
>  ofono_modem *modem, hfd.when = when;
>  	hfd.status = status;
> 
> +	/* Format time, "ddd yyyy-mm-dd hh:mm:ss zzz" */
> +	ts = localtime(&when);
> +	strftime(buf, sizeof(buf), "%a %Y-%m-%d %H:%M:%S %Z", ts);
> +
> +	if (status == OFONO_HISTORY_SMS_STATUS_DELIVERED)
> +		DBG("SMS delivered, msg_id: %i, time: %s", msg_id, buf);
> +	else if (status == OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED)
> +		DBG("SMS undeliverable, msg_id: %i, time: %s", msg_id, buf);
> +

I didn't take this patch because these debugs really don't belong here.  
Instead I put them in plugins/example_history.c.

Regards,
-Denis