SMS status report

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 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

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 5/5] smsutil: save pending status reports to imsi file

[Tikander Petteri]

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

Hi Denis,

I'm the guy, who already introduced shortly yesterday to Inaky, and I'm continuing Pasi Miettinen's fine
work with SMS status-reports.
So, here are some comments (included below) for updating status report-functionality for your earlier comments,
how SR-functionality could be improved.

Br, Petteri

> ________________________________________
> Lähettäjä: ofono-bounces(a)ofono.org [ofono-bounces(a)ofono.org]
> käyttäjän ext Denis Kenzior [denkenz(a)gmail.com] puolesta
> Lähetetty: 22. kesäkuuta 2010 0:07
> Vastaanottaja: ofono(a)ofono.org
> Aihe: Re: [RFC PATCH 5/5] smsutil: save pending status reports to
>  imsi file
>
> 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.

OK, I will change that, so that mallocs (and freeings) not needed anymore for addr.

>
> > +
> > +     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.
>

OK.

> > +             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.
>

OK.

> > +             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.
>

OK. I agree that generally single function for heavy lifting is nicer solution.
I will implement some general function, which handles adding of new id-table (if not already added)
to assembly-table, and storing necessary node-information there. After that I will move some functionality from
sr_assembly_load_backup() to this general function. Also this function can be used when sending new sms-text message.
And I will add new parameter for telling, if this function will save data to imsi-file on disk or not.
Existing sms-assembly handles it that way. But, one difference with sms-assembly and status-report logic is, that
sms-assembly naturally stores every messages of concatenated, long SMS-message to own backup files, but status report
gathers all information belonging to the same long SMS-message (using message reference numbers) to the single file.

So Pasi implemented in the loading-phase (sr_assembly_load_backup) storing of MR-numbers simply
by copying the whole buffer containing MR's belonging to the single concatenated message-node:

1) memcpy(node->mrs, buf, sizeof(node->mrs));

And naturally when sending single message, single MR-bit is set up ('status_report_assembly_add_fragment()'):

2) node->mrs[offset] |= bit;

So, when now implementing new general function, one solution could be to bring MR-string as parameter
so way (1) could be used in different cases.

Also I have to take care of variable 'sent_mrs', which will be incremented in the sms-submit, but not in the loading phase.

> >  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
>

OK.

> > +             /* 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.
>

OK.

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

OK.

>
> > +             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.

OK.

> >       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.
>

OK. Lets give main assembly to this (and corresponding) function(s).
How about return value? For instance existing sms assembly-function 'sms_assembly_add_fragment_backup()'
already returns something else than void.

> > +{
> > +     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.
>

OK. Removal of backup file is not necessary, so I will handle updating of existing file simple
by overwriting the existing file.

> >  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
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono
>
>

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

[Denis Kenzior]

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

Hi Petteri,

On 08/10/2010 11:25 AM, Tikander Petteri wrote:
> Hi Denis,
> 
> I'm the guy, who already introduced shortly yesterday to Inaky, and I'm continuing Pasi Miettinen's fine
> work with SMS status-reports.

Good to have you aboard :)

> So, here are some comments (included below) for updating status report-functionality for your earlier comments,
> how SR-functionality could be improved.
> 

<snip>

>> 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.
>>
> 
> OK. I agree that generally single function for heavy lifting is nicer solution.
> I will implement some general function, which handles adding of new id-table (if not already added)
> to assembly-table, and storing necessary node-information there. After that I will move some functionality from
> sr_assembly_load_backup() to this general function. Also this function can be used when sending new sms-text message.
> And I will add new parameter for telling, if this function will save data to imsi-file on disk or not.
> Existing sms-assembly handles it that way. But, one difference with sms-assembly and status-report logic is, that
> sms-assembly naturally stores every messages of concatenated, long SMS-message to own backup files, but status report
> gathers all information belonging to the same long SMS-message (using message reference numbers) to the single file.
> 
> So Pasi implemented in the loading-phase (sr_assembly_load_backup) storing of MR-numbers simply
> by copying the whole buffer containing MR's belonging to the single concatenated message-node:
> 
> 1) memcpy(node->mrs, buf, sizeof(node->mrs));
> 
> And naturally when sending single message, single MR-bit is set up ('status_report_assembly_add_fragment()'):
> 
> 2) node->mrs[offset] |= bit;
> 
> So, when now implementing new general function, one solution could be to bring MR-string as parameter
> so way (1) could be used in different cases.
> 
> Also I have to take care of variable 'sent_mrs', which will be incremented in the sms-submit, but not in the loading phase.
>

I agree there are going to be differences.  See how far you get trying
to unify these behind a single function and send for another round of
reviews.

<snip>

>>>       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.
>>
> 
> OK. Lets give main assembly to this (and corresponding) function(s).
> How about return value? For instance existing sms assembly-function 'sms_assembly_add_fragment_backup()'
> already returns something else than void.
> 

sms_assembly_add_fragment_backup returns the new list of fragments.
What value can we return from this function?  I think returning
TRUE/FALSE for file write failures are not worth it.  There's not much
that can be done in that case anyway...  So it seems void is good
enough, but feel free to suggest something.

Regards,
-Denis

[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