SMS status report

SMS status report

[Pasi Miettinen]

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


Hello again,
this is the new version of SMS status report. It is implemented
according to the latest comments which you gave about my previous
patches. These patches include concatenated status report
assembling and status report result printing via history plugin.

Next I am going to take a new look at the saving of the pending
status reports to imsi file.

Br,
Pasi Miettinen 

Re: SMS status report

[Inaky Perez-Gonzalez]

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

Hi Pasi

On Wed, 2010-06-09 at 17:01 +0300, Pasi Miettinen wrote: 
> Hello again,
> this is the new version of SMS status report. It is implemented
> according to the latest comments which you gave about my previous
> patches. These patches include concatenated status report
> assembling and status report result printing via history plugin.
> 
> Next I am going to take a new look at the saving of the pending
> status reports to imsi file.

I found out right before going offline for last week that you were
working on status reports -- good to know I am not alone.

I'd like to coordinate with your work. I had started tackling the issue
from the infrastructure of the whole SMS code and had recently started
work on the actual status-report part. Once your code started showing
up, I've ditched part of my code and rebased on top of what you had.

Could you please take a look at the smsdbus branch in my repo and tell
me if there is anything there that will conflict with the way you are
implementing things?

git://gitorious.org/~inakypg/ofono/ofono-inakypg.git

there is a set of patches there which lay out changes in the SMS code
infrastructure to do the following:

- export of SMS messages to D-Bus

  - each message is an object in the bus

  - messages can be canceled, properties queried, etc

- in transit SMS messages get an state machine describing at which phase
they are. D-Bus signals are sent in transitions

- messages waiting for an status report are queued in a separate queue

- a unique ID is generated based on message contents and destination
nunmber to reduce the chances of collision -- need still to apply it to
received messages so it's easier to gather status reports from different
fragments

- provide an internal C API for managing messages (D-Bus's maps on top
of this)


FIXMEs/TODOs/RFCs:

  still need to cleanup some more to adapt to your code

  plenty of whatifs/FIXMEs litered around the code still

  remove the 'request-status-report' from D-Bus::SendMessage

  message id needs more uniquedness fed into it

Note the branch depends on some more commits that are still not in
mainline; those are in my master branch and pending a resubmit to
upstream.

Thanks!

SMS status report

[Pasi Miettinen]

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

Hi,

I noticed that mr-numbers and msg_ids were not updated to the data
structure if the status was something else than "delivered". In
these two patches it is fixed.

So if one mr is concidered undeliverable, the whole id is removed
from the data structure and the mr numbers that still may arrive
as "delivered" are not taken into account anymore.

Do we already have a plan for how accurately we are going to check
these status report status codes?

Br,
Pasi

[RFC PATCH 1/2] smsutil: Status Report assembly

[Pasi Miettinen]

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

---
 src/smsutil.c |  138 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h |   28 ++++++++++++
 2 files changed, 166 insertions(+), 0 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 278d335..5c816f8 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2625,6 +2625,144 @@ 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);
+}
+
+enum ofono_history_sms_status status_report_assembly_report(
+					struct status_report_assembly *assembly,
+					const struct sms *status_report,
+					unsigned int *msg_id)
+{
+	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 = NULL;
+	gpointer key;
+	gpointer value;
+	GHashTableIter iter;
+	int i;
+
+	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)
+		return OFONO_HISTORY_SMS_STATUS_ERROR;
+
+	g_hash_table_iter_init(&iter, id_table);
+	while (g_hash_table_iter_next(&iter, &key, &value)) {
+		node = value;
+
+		if (!(node->mrs[offset] & bit))
+			continue;
+
+		if (status_report->status_report.st ==
+						SMS_ST_COMPLETED_RECEIVED) {
+			/* mr belongs to this node */
+			node->mrs[offset] ^= bit;
+			*msg_id = node->ofono_msg_id;
+
+			for (i = 0; i < 8; i++) {
+				/* There are still pending mr(s). */
+				if (node->mrs[i] != 0)
+					return OFONO_HISTORY_SMS_STATUS_PENDING;
+			}
+		}
+		/* No more pending mrs for this node. Remove node from id_table
+		 * and free it.
+		 */
+		g_hash_table_iter_remove(&iter);
+		/* id_list empty, remove key and value from assembly_table */
+		if (g_hash_table_size(id_table) == 0)
+			g_hash_table_remove(assembly->assembly_table,
+				status_report->status_report.raddr.address);
+
+		return OFONO_HISTORY_SMS_STATUS_DELIVERED;
+	}
+	/* mr not found. */
+	return OFONO_HISTORY_SMS_STATUS_ERROR;
+}
+
+void status_report_assembly_add_fragment(struct status_report_assembly
+					*assembly, unsigned int *msg_id,
+					struct sms_address *to,
+					unsigned char mr, time_t expiration)
+{
+	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);
+
+	if (id_table) {
+		node = g_hash_table_lookup(id_table, msg_id);
+
+		if (node) {
+			node->mrs[offset] |= bit;
+			node->expiration = expiration;
+		} else {
+			id_table_key = g_new0(unsigned int, 1);
+			node = g_new0(struct id_table_node, 1);
+			node->to = *to;
+			node->ofono_msg_id = *msg_id;
+			node->mrs[offset] |= bit;
+			node->expiration = expiration;
+
+			*id_table_key = *msg_id;
+			g_hash_table_insert(id_table, id_table_key, node);
+		}
+	} else {
+		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->ofono_msg_id = *msg_id;
+		node->mrs[offset] |= bit;
+		node->expiration = expiration;
+
+		*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)
+			return;
+
+		g_strlcpy(assembly_table_key, to->address, sizeof(to->address));
+
+		g_hash_table_insert(assembly->assembly_table,
+					assembly_table_key, id_table);
+	}
+}
+
+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 a36a9d3..14a01dc 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,18 @@ struct sms_assembly {
 	GSList *assembly_list;
 };
 
+struct id_table_node {
+	struct sms_address to;
+	unsigned int ofono_msg_id;
+	unsigned int mrs[8];
+	time_t expiration;
+};
+
+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 +495,20 @@ 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);
+enum ofono_history_sms_status status_report_assembly_report(
+					struct status_report_assembly *assembly,
+					const struct sms *status_report,
+					unsigned int *msg_id);
+void status_report_assembly_add_fragment(struct status_report_assembly
+					*assembly, unsigned int *msg_id,
+					struct sms_address *to,
+					unsigned char mr, time_t expiration);
+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);
 
-- 
1.6.0.4

[RFC PATCH 2/2] sms: status report notify

[Pasi Miettinen]

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

---
 include/history.h |    3 ++
 src/sms.c         |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/include/history.h b/include/history.h
index 300a4fb..61a0dea 100644
--- a/include/history.h
+++ b/include/history.h
@@ -33,6 +33,9 @@ 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,
+	OFONO_HISTORY_SMS_STATUS_ERROR,
 };
 
 struct ofono_history_context {
diff --git a/src/sms.c b/src/sms.c
index c0e9fc4..4cd4084 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -67,6 +67,7 @@ struct ofono_sms {
 	const struct ofono_sms_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
+	struct status_report_assembly *sr_assembly;
 };
 
 struct pending_pdu {
@@ -82,6 +83,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,
@@ -306,6 +309,12 @@ 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));
+
 	if (entry->cur_pdu < entry->num_pdus) {
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 		return;
@@ -436,6 +445,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);
 
@@ -448,6 +459,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);
 
@@ -692,6 +704,27 @@ 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)
+{
+	unsigned int msg_id;
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
+
+	if (incoming->status_report.st == SMS_ST_COMPLETED_RECEIVED) {
+
+		if (status_report_assembly_report(sms->sr_assembly,
+		incoming, &msg_id) == OFONO_HISTORY_SMS_STATUS_DELIVERED)
+			__ofono_history_sms_send_status(modem, msg_id,
+				time(NULL), OFONO_HISTORY_SMS_STATUS_DELIVERED);
+	} else {
+		status_report_assembly_report(sms->sr_assembly,
+						incoming, &msg_id);
+		__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;
@@ -823,7 +856,25 @@ out:
 void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
 				int len, int tpdu_len)
 {
-	ofono_error("SMS Status-Report not yet handled");
+	struct sms s;
+	enum sms_class cls;
+
+	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
+		ofono_error("Unable to decode PDU");
+		return;
+	}
+
+	if (s.type != SMS_TYPE_STATUS_REPORT) {
+		ofono_error("Expecting a STATUS REPORT pdu");
+		return;
+	}
+
+	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
+		ofono_error("Unknown / Reserved DCS.  Ignoring");
+		return;
+	}
+
+	handle_sms_status_report(sms, &s);
 }
 
 int ofono_sms_driver_register(const struct ofono_sms_driver *d)
@@ -903,6 +954,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);
 }
 
@@ -1038,9 +1094,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

Re: SMS status report

[Denis Kenzior]

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

Hi Pasi,

> So if one mr is concidered undeliverable, the whole id is removed
> from the data structure and the mr numbers that still may arrive
> as "delivered" are not taken into account anymore.

That sounds fine.

> 
> Do we already have a plan for how accurately we are going to check
> these status report status codes?

My thinking right now is that we should take all success and all permanent 
error codes into account.  But that is quite easy...

Was there something else to your question?

Regards,
-Denis

Re: [RFC PATCH 2/2] sms: status report notify

[Denis Kenzior]

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

Hi Pasi,

> ---
>  include/history.h |    3 ++
>  src/sms.c         |   61
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files 
changed, 63
>  insertions(+), 1 deletions(-)
> 
> diff --git a/include/history.h b/include/history.h
> index 300a4fb..61a0dea 100644
> --- a/include/history.h
> +++ b/include/history.h
> @@ -33,6 +33,9 @@ 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,

Please resubmit the above two lines as a separate patch.  I like to keep 
public API changes separate.

> +	OFONO_HISTORY_SMS_STATUS_ERROR,

Drop this change, lets modify status_report_assembly to do something different.

>  };
> 
> +static void handle_sms_status_report(struct ofono_sms *sms,
> +						const struct sms *incoming)
> +{
> +	unsigned int msg_id;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
> +
> +	if (incoming->status_report.st == SMS_ST_COMPLETED_RECEIVED) {
> +
> +		if (status_report_assembly_report(sms->sr_assembly,
> +		incoming, &msg_id) == OFONO_HISTORY_SMS_STATUS_DELIVERED)
> +			__ofono_history_sms_send_status(modem, msg_id,
> +				time(NULL), OFONO_HISTORY_SMS_STATUS_DELIVERED);
> +	} else {
> +		status_report_assembly_report(sms->sr_assembly,
> +						incoming, &msg_id);

This looks a little ugly, let the status report assembly decide whether the 
message was delivered / failed.  So something like:

gboolean delivered;
unsigned int message_id;
gboolean update_history;

update_history = status_report_assembly_report(assembly, incoming, &msg_id, 
&delivered);
if (update_history)
	...
> +		__ofono_history_sms_send_status(modem, msg_id, time(NULL),
> +				OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);

Otherwise patch looks good to me.

Regards,
-Denis

Re: [RFC PATCH 2/2] sms: status report notify

[Denis Kenzior]

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

Hi Pasi,

> +	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
> +		ofono_error("Unknown / Reserved DCS.  Ignoring");
> +		return;
> +	}
> +

This reminds me, one other check I'd like you to make here is that the status 
report belongs to an SMS Submit, not an SMS Command.  See TP-SRQ in 23.040.  I 
like to be paranoid :)

Regards,
-Denis

Re: [RFC PATCH 1/2] smsutil: Status Report assembly

[Denis Kenzior]

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

Hi Pasi,

> +void status_report_assembly_free(struct status_report_assembly *assembly)
> +{
> +	g_hash_table_destroy(assembly->assembly_table);

You should also be g_freeing the assembly structure itself.  In general, it is 
a very good idea to run valgrind --leak-check=full on the modified binary 
before submitting any patches upstream.  This helps to identify such issues 
early.

And did I mention unit tests? :)

> +}
> +
> +enum ofono_history_sms_status status_report_assembly_report(
> +					struct status_report_assembly *assembly,
> +					const struct sms *status_report,
> +					unsigned int *msg_id)
> +{
> +	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 = NULL;

There's no need to initialize this to NULL.

> +	gpointer key;
> +	gpointer value;
> +	GHashTableIter iter;
> +	int i;
> +
> +	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)
> +		return OFONO_HISTORY_SMS_STATUS_ERROR;
> +
> +	g_hash_table_iter_init(&iter, id_table);
> +	while (g_hash_table_iter_next(&iter, &key, &value)) {
> +		node = value;
> +
> +		if (!(node->mrs[offset] & bit))
> +			continue;
> +
> +		if (status_report->status_report.st ==
> +						SMS_ST_COMPLETED_RECEIVED) {
> +			/* mr belongs to this node */
> +			node->mrs[offset] ^= bit;
> +			*msg_id = node->ofono_msg_id;
> +
> +			for (i = 0; i < 8; i++) {
> +				/* There are still pending mr(s). */
> +				if (node->mrs[i] != 0)
> +					return OFONO_HISTORY_SMS_STATUS_PENDING;
> +			}
> +		}
> +		/* No more pending mrs for this node. Remove node from id_table
> +		 * and free it.
> +		 */
> +		g_hash_table_iter_remove(&iter);
> +		/* id_list empty, remove key and value from assembly_table */
> +		if (g_hash_table_size(id_table) == 0)
> +			g_hash_table_remove(assembly->assembly_table,
> +				status_report->status_report.raddr.address);
> +
> +		return OFONO_HISTORY_SMS_STATUS_DELIVERED;

You're marking this message as delivered even if a failure status report was 
sent.  Also, in the case of a failure, you're not providing the message id.

> +void status_report_assembly_add_fragment(struct status_report_assembly
> +					*assembly, unsigned int *msg_id,

Please don't put the assembly on a separate line.  The var name should be on 
the same line as the type.

msg_id should simply be unsigned int msg_id.  This is not an in/out argument.

> +					struct sms_address *to,

const struct sms_address *to please.

> +					unsigned char mr, time_t expiration)
> +{
> +	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);

I'd structure the code like this:
if (id_table == NULL) {
 what is in the ... else clause
 return;
}

node = g_hash_table_lookup();

if (node) {
return;
}

...

This makes the code much more readable.  In general, avoid nested if 
statements if possible.

> +
> +	if (id_table) {
> +		node = g_hash_table_lookup(id_table, msg_id);
> +
> +		if (node) {
> +			node->mrs[offset] |= bit;
> +			node->expiration = expiration;
> +		} else {
> +			id_table_key = g_new0(unsigned int, 1);
> +			node = g_new0(struct id_table_node, 1);
> +			node->to = *to;
> +			node->ofono_msg_id = *msg_id;
> +			node->mrs[offset] |= bit;
> +			node->expiration = expiration;
> +
> +			*id_table_key = *msg_id;
> +			g_hash_table_insert(id_table, id_table_key, node);
> +		}
> +	} else {
> +		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->ofono_msg_id = *msg_id;
> +		node->mrs[offset] |= bit;
> +		node->expiration = expiration;
> +
> +		*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)
> +			return;
> +
> +		g_strlcpy(assembly_table_key, to->address, sizeof(to->address));
> +
> +		g_hash_table_insert(assembly->assembly_table,
> +					assembly_table_key, id_table);
> +	}

I see one problem with this function, namely that under some bizarre 
circumstances, the status report might come back before additional fragments 
have been added.  So this function should also take a num_fragments argument 
and keep track of how many fragment status reports were actually received.

This gets a bit tricky when a failure is reported before the entire set of 
fragments has been submitted...

> +struct id_table_node {
> +	struct sms_address to;
> +	unsigned int ofono_msg_id;

Why are you storing the message id when it is also the key for the hash table?

> +	unsigned int mrs[8];
> +	time_t expiration;
> +};
> +

Regards,
-Denis

VS: [RFC PATCH 1/2] smsutil: Status Report assembly

[Miettinen Pasi]

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

Hi Denis,
> Hi Pasi,
> 
> > +void status_report_assembly_free(struct status_report_assembly *assembly)
> > +{
> > +     g_hash_table_destroy(assembly->assembly_table);
> 
> You should also be g_freeing the assembly structure itself.  In general, it is
> a very good idea to run valgrind --leak-check=full on the modified binary
> before submitting any patches upstream.  This helps to identify such issues
> early.

Yes, I'll start using valgrind so I don't forget to free something.

> And did I mention unit tests? :)

And will add the unit tests to my todo list :)
|
> > +             if (status_report->status_report.st ==
> > +                                             SMS_ST_COMPLETED_RECEIVED) {
> > +                     /* mr belongs to this node */
> > +                     node->mrs[offset] ^= bit;
> > +                     *msg_id = node->ofono_msg_id;
> > +
> > +                     for (i = 0; i < 8; i++) {
> > +                             /* There are still pending mr(s). */
> > +                             if (node->mrs[i] != 0)
> > +                                     return OFONO_HISTORY_SMS_STATUS_PENDING;
> > +                     }
> > +             }
> > +             /* No more pending mrs for this node. Remove node from id_table
> > +              * and free it.
> > +              */
> > +             g_hash_table_iter_remove(&iter);
> > +             /* id_list empty, remove key and value from assembly_table */
> > +             if (g_hash_table_size(id_table) == 0)
> > +                     g_hash_table_remove(assembly->assembly_table,
> > +                             status_report->status_report.raddr.address);
> > +
> > +             return OFONO_HISTORY_SMS_STATUS_DELIVERED;
>
> You're marking this message as delivered even if a failure status report was
> sent.  Also, in the case of a failure, you're not providing the message id.

Yes, this was like this because the non-delivered status report results were
identified by the  function caller in this version. But I agree that the function logic
by itself is not right. And you're correct in that the if clause should start after
the msg_id setting. Will correct these. I was in too much of a haste at the end
of the day when I made these changes.

> Regards,
> -Denis

Br,
-Pasi

VS: [RFC PATCH 1/2] smsutil: Status Report assembly

[Miettinen Pasi]

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

> I see one problem with this function, namely that under some bizarre
> circumstances, the status report might come back before additional fragments
> have been added.  So this function should also take a num_fragments argument
> and keep track of how many fragment status reports were actually received.
> 
> This gets a bit tricky when a failure is reported before the entire set of
> fragments has been submitted...

Yes, with the current implementation the last part would be little bit tricky.
I think that maybe we shoudn't delete the whole msg_id after the first
"undelivered" mr-number. This is just quick brainstorming, but maybe we
could mark the msg_id "dead" when the first undelivered mr arrives and
then just wait for the remaining mrs like we do with the delivered ones?
When all of the mrs has arrived, we check if the msg_id is marked "dead"
and continue from there..

Br,
-Pasi

PS. Have a nice weekend all :)

SMS status report

[Pasi Miettinen]

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


Hi,
These are the latest modifications for the SMS status report. Pathc
includes:

1. SMS status report assembly in smsutil.
2. Status report result printing in history plugin.
3. API change in history plugin.
4. SMS notify for status report.
5. Saving the pending status report info to imsi file.

The most common use cases were tested in practice. For example
a concatenated message was sent to phone that was turned off and
Ofono was rebooted. Then the phone was turned on and message was
received and  Ofono received the status report and was able to
work with the data which was loaded from the imsi file. Ofono was
run under valgrind supervision and no memory leaks were
detected.

Br,
Pasi Miettinen

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