5:05 p.m.
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
5:05 p.m.
New subject: [RFC PATCH 1/2] smsutil: Status Report assembly
---
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
5:05 p.m.
New subject: [RFC PATCH 2/2] sms: status report notify
---
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
1:07 a.m.
New subject: [RFC PATCH 2/2] sms: status report notify
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
2:14 a.m.
New subject: [RFC PATCH 2/2] sms: status report notify
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
2:52 a.m.
New subject: [RFC PATCH 1/2] smsutil: Status Report assembly
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
9:35 p.m.
New subject: VS: [RFC PATCH 1/2] smsutil: Status Report assembly
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
10:23 p.m.
New subject: VS: [RFC PATCH 1/2] smsutil: Status Report assembly
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 :)
12:47 a.m.
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
4482
days inactive
4483
days old
8 comments
3 participants
participants (3)
-
Denis Kenzior -
Miettinen Pasi -
Pasi Miettinen