[SMS D-Bus 00/19] pull request

[SMS D-Bus 00/19] pull request

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

- UUID code reworked, simplified to Dennis' suggestion

- UUID code integrated -- still kind of incomplete because we need to
  see what to do when the message contents and destination number is
  the same.

- BUG_ON* removed, not used anymore

- removed 'struct sms_msg_dbus_data', pass around 'dbus_path' as
  private data.

- D-Bus: 

  number of PDUs not longer part of the D-Bus name

  added 'To' to the D-Bus properties; ignored for now
  submittion/delivery times as it will complicate the interface -- we
  need another callback for property change specification. Is it worth
  it?

  Fixed GetProperties method mistakenly marked ASYNC

  Interface renamed to SmsManager

  Cleaned up doc/sms-api.txt according to feedback

- unfolded tx_queue_entry_destroy_free  into
  tx_queue_entry_free_foreach() and tx_queue_entry_free() according to
  feedback. 

- broke up multi-directory commits

- cleaned up style violations

- add a fix to generation of symlinks for headers, it broke in VPATH
  builds.

The following changes since commit 6f1ab8b6794333673fefc3d52f9b0388526a56cd:
  Kristen Carlson Accardi (1):
        test-stkutil: unit test for img to xpm converter

are available in the git repository at:

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

Patches follow for reviewing convenience.

Inaky Perez-Gonzalez (19):
      write_file: make transaction-safe
      sms: introduce message ID API
      sms: implement SHA256-based message IDs [incomplete]
      sms: document the org.ofono.SmsMessage interface
      sms: document handle_sms_status_report()
      struct tx_queue_entry: add a destructor
      sms: introduce bare state machine and transitions
      sms: introduce the Wait-for-Status-Report state
      sms: introduce a state change callback for messages
      sms: export outgoing messages over D-Bus
      sms: send PropertyChanged signals on state change
      sms: introduce sms_msg_cancel and its D-Bus wrapper
      sms: Implement D-Bus SMS-MSG::GetProperties
      sms: document SMS Message's D-Bus 'To' property
      sms: test code for message's D-Bus GetProperties
      automake: fix generation of symlinks for headers
      sms: document variable usage in sms_msg_send()
      sms: add test case for message cancel
      sms: add test case for state change signals

 Makefile.am                      |    2 +-
 doc/sms-api.txt                  |   54 ++++-
 src/ofono.h                      |   45 +++-
 src/sms.c                        |  581 ++++++++++++++++++++++++++++++++------
 src/smsutil.c                    |   78 +++++
 src/smsutil.h                    |    8 +
 src/stk.c                        |   22 ++-
 src/storage.c                    |   47 +++-
 test/test-sms-msg-cancel         |  173 +++++++++++
 test/test-sms-msg-get-properties |   26 ++
 test/test-sms-msg-state-change   |   24 ++
 11 files changed, 951 insertions(+), 109 deletions(-)
 create mode 100755 test/test-sms-msg-cancel
 create mode 100755 test/test-sms-msg-get-properties
 create mode 100755 test/test-sms-msg-state-change

[SMS D-Bus 01/19] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

write_file(), as written wasn't transaction-safe; a crash bewtween a
file being open and the buffer being written before a safe close would
leave the file with a set of undetermined contents.

Modified to the file is written to a temporary file name; once
completed, it is renamed to the final name. This way, a crash in the
middle doesn't leave half-baked files.
---
 src/storage.c |   47 ++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/src/storage.c b/src/storage.c
index cac5835..530439d 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -98,11 +98,21 @@ ssize_t read_file(unsigned char *buffer, size_t len,
 	return r;
 }
 
+/*
+ * Write a buffer to a file in a transactionally safe form
+ *
+ * Given a buffer, write it to a file named after
+ * @path_fmt+args. However, to make sure the file contents are
+ * consistent (ie: a crash right after opening or during write()
+ * doesn't leave a file half baked), the contents are written to a
+ * file with a temporary name and when closed, it is renamed to the
+ * specified name (@path_fmt+args).
+ */
 ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode,
 			const char *path_fmt, ...)
 {
 	va_list ap;
-	char *path;
+	char *tmp_path, *path;
 	ssize_t r;
 	int fd;
 
@@ -110,26 +120,41 @@ ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode,
 	path = g_strdup_vprintf(path_fmt, ap);
 	va_end(ap);
 
-	if (create_dirs(path, mode | S_IXUSR) != 0) {
-		g_free(path);
-		return -1;
-	}
+	tmp_path = g_strdup_printf("%s.XXXXXX.tmp", path);
 
-	fd = TFR(open(path, O_WRONLY | O_CREAT | O_TRUNC, mode));
-	if (fd == -1) {
-		g_free(path);
-		return -1;
-	}
+	r = -1;
+	if (create_dirs(path, mode | S_IXUSR) != 0)
+		goto error_create_dirs;
+
+	fd = TFR(g_mkstemp_full(tmp_path, O_WRONLY | O_CREAT | O_TRUNC, mode));
+	if (fd == -1)
+		goto error_mkstemp_full;
 
 	r = TFR(write(fd, buffer, len));
 
 	TFR(close(fd));
 
 	if (r != (ssize_t) len) {
-		unlink(path);
 		r = -1;
+		goto error_write;
 	}
 
+	/*
+	 * Now that the file contents are written, rename to the real
+	 * file name; this way we are uniquely sure that the whole
+	 * thing is there.
+	 */
+	unlink(path);
+
+	/* conserve @r's value from 'write' */
+	if (link(tmp_path, path) == -1)
+		r = -1;
+
+error_write:
+	unlink(tmp_path);
+error_mkstemp_full:
+error_create_dirs:
+	g_free(tmp_path);
 	g_free(path);
 	return r;
 }
-- 
1.6.6.1

[SMS D-Bus 02/19] sms: introduce message ID API

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

This adds a simple API to use for generating unique IDs for SMS
messages. Will be used by follow up commits.

The ID is not generic, but specifc to SMS messages (due to having to
dig inside 'struct sms') and thus generates directly 16-bit IDs.
---
 src/smsutil.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h |    2 +
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 22c70cf..b958ee0 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -3983,3 +3983,81 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)
 
 	return utf8;
 }
+
+static
+int __sms_uuid_from_pdu(GChecksum *checksum, const struct sms *sms_pdu)
+{
+	const guint8 *buf;
+	size_t buf_len;
+
+	switch (sms_pdu->type) {
+	case SMS_TYPE_DELIVER:
+		buf = sms_pdu->deliver.ud;
+		buf_len = sms_pdu->deliver.udl;
+		break;
+	case SMS_TYPE_DELIVER_REPORT_ACK:
+		buf = sms_pdu->deliver_ack_report.ud;
+		buf_len = sms_pdu->deliver_ack_report.udl;
+		break;
+	case SMS_TYPE_DELIVER_REPORT_ERROR:
+		buf = sms_pdu->deliver_err_report.ud;
+		buf_len = sms_pdu->deliver_err_report.udl;
+		break;
+	case SMS_TYPE_STATUS_REPORT:
+		buf = sms_pdu->status_report.ud;
+		buf_len = sms_pdu->status_report.udl;
+		break;
+	case SMS_TYPE_SUBMIT:
+		buf = sms_pdu->submit.ud;
+		buf_len = sms_pdu->submit.udl;
+		break;
+	case SMS_TYPE_SUBMIT_REPORT_ACK:
+		buf = sms_pdu->submit_ack_report.ud;
+		buf_len = sms_pdu->submit_ack_report.udl;
+		break;
+	case SMS_TYPE_SUBMIT_REPORT_ERROR:
+		buf = sms_pdu->submit_err_report.ud;
+		buf_len = sms_pdu->submit_err_report.udl;
+		break;
+	case SMS_TYPE_COMMAND:
+		buf = sms_pdu->command.cd;
+		buf_len = sms_pdu->command.cdl;
+		break;
+	default:
+		return 1;
+	}
+	g_checksum_update(checksum, buf, buf_len);
+	return 0;
+}
+
+/**
+ * Generate a UUID from an SMS PDU List
+ *
+ * @param sms_pdu_list GSlist containing 'struct sms' nodes to
+ *     generate the UUID from.
+ * @return 0 in error (no memory or serious code inconsistency in the
+ *     input data structures), otherwise the SMS UUID.
+ */
+guint16 sms_uuid_from_pdu_list(const GSList *sms_pdu_list)
+{
+	guint16 uuid = 0;
+	GChecksum *checksum;
+	const GSList *node;
+	gsize uuid_size = g_checksum_type_get_length(G_CHECKSUM_SHA256);
+	guint8 data[uuid_size];
+
+	checksum = g_checksum_new(G_CHECKSUM_SHA256);
+	if (checksum == NULL)
+		goto error_new;
+	for (node = sms_pdu_list; node; node = node->next) {
+		struct sms *sms_pdu = node->data;
+		if (__sms_uuid_from_pdu(checksum, sms_pdu))
+			goto error_pdu;
+	}
+	g_checksum_get_digest(checksum, data, &uuid_size);
+	uuid = data[0] | data[1] << 8;
+error_pdu:
+	g_checksum_free(checksum);
+error_new:
+	return uuid;
+}
diff --git a/src/smsutil.h b/src/smsutil.h
index ca64b18..66486b7 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -547,3 +547,5 @@ GSList *cbs_optimize_ranges(GSList *ranges);
 gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges);
 
 char *ussd_decode(int dcs, int len, const unsigned char *data);
+
+guint16 sms_uuid_from_pdu_list(const GSList *sms_pdu_list);
-- 
1.6.6.1

[SMS D-Bus 03/19] sms: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

This patch removes the sequential SMS message identification gig and
replaces it with a 16-bit hash cookie based off message contents.

FIXME: [incomplete] need to figure out how to do so that identical
messages sent to different or the same numbers also have a different
ID.
---
 src/ofono.h |    9 +++++----
 src/sms.c   |   55 +++++++++++++++++++++++--------------------------------
 src/stk.c   |    5 +++--
 3 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index aaa01d9..b73797b 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -185,10 +185,11 @@ enum ofono_sms_submit_flag {
 
 typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
 
-unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
-					unsigned int flags,
-					ofono_sms_txq_submit_cb_t cb,
-					void *data, ofono_destroy_func destroy);
+struct tx_queue_entry *__ofono_sms_txq_submit(
+	struct ofono_sms *sms, GSList *list,
+	unsigned int flags, unsigned ref,
+	ofono_sms_txq_submit_cb_t cb,
+	void *data, ofono_destroy_func destroy);
 
 #include <ofono/sim.h>
 #include <ofono/stk.h>
diff --git a/src/sms.c b/src/sms.c
index 35364db..39d4a32 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -55,7 +55,6 @@ struct ofono_sms {
 	DBusMessage *pending;
 	struct ofono_phone_number sca;
 	struct sms_assembly *assembly;
-	unsigned int next_msg_id;
 	guint ref;
 	GQueue *txq;
 	gint tx_source;
@@ -589,7 +588,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	int ref_offset;
 	struct ofono_modem *modem;
 	unsigned int flags;
-	unsigned int msg_id;
+	guint16 msg_id;
+	struct tx_queue_entry *sms_msg;
 
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
@@ -605,24 +605,19 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	if (!msg_list)
 		return __ofono_error_invalid_format(msg);
 
-	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
-	DBG("ref: %d, offset: %d", sms->ref, ref_offset);
-
-	if (ref_offset != 0) {
-		if (sms->ref == 65536)
-			sms->ref = 1;
-		else
-			sms->ref = sms->ref + 1;
-	}
+	msg_id = sms_uuid_from_pdu_list(msg_list);
+	set_ref_and_to(msg_list, msg_id, ref_offset, to);
+	DBG("SMS ID: 0x%04x", msg_id);
 
 	flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
 	flags |= OFONO_SMS_SUBMIT_FLAG_RETRY;
 	if (sms->use_delivery_reports)
 		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
 
-	msg_id = __ofono_sms_txq_submit(sms, msg_list, flags, send_message_cb,
-						dbus_message_ref(msg),
-						send_message_destroy);
+	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
+					 send_message_cb,
+					 dbus_message_ref(msg),
+					 send_message_destroy);
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
@@ -659,7 +654,7 @@ static void dispatch_app_datagram(struct ofono_sms *sms, int dst, int src,
 }
 
 static void dispatch_text_message(struct ofono_sms *sms,
-					const char *message,
+					const char *message, guint16 msg_id,
 					enum sms_class cls,
 					const struct sms_address *addr,
 					const struct sms_scts *scts)
@@ -717,11 +712,9 @@ static void dispatch_text_message(struct ofono_sms *sms,
 
 	g_dbus_send_message(conn, signal);
 
-	if (cls != SMS_CLASS_0) {
-		__ofono_history_sms_received(modem, sms->next_msg_id, str,
+	if (cls != SMS_CLASS_0)
+		__ofono_history_sms_received(modem, msg_id, str,
 						&remote, &local, message);
-		sms->next_msg_id += 1;
-	}
 }
 
 static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
@@ -732,6 +725,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	enum sms_class cls;
 	int srcport = -1;
 	int dstport = -1;
+	guint16 msg_id;
 
 	if (sms_list == NULL)
 		return;
@@ -749,6 +743,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	 * used, the addresses are the same across all segments.
 	 */
 
+	msg_id = sms_uuid_from_pdu_list(sms_list);
 	for (l = sms_list; l; l = l->next) {
 		guint8 dcs;
 		gboolean comp = FALSE;
@@ -825,8 +820,8 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 
 		s = sms_list->data;
 
-		dispatch_text_message(sms, message, cls, &s->deliver.oaddr,
-					&s->deliver.scts);
+		dispatch_text_message(sms, message, msg_id, cls,
+					&s->deliver.oaddr, &s->deliver.scts);
 		g_free(message);
 	}
 }
@@ -1104,8 +1099,6 @@ static void sms_remove(struct ofono_atom *atom)
 
 	if (sms->settings) {
 		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
-					"NextMessageId", sms->next_msg_id);
-		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
 					"NextReference", sms->ref);
 		g_key_file_set_boolean(sms->settings, SETTINGS_GROUP,
 					"UseDeliveryReports",
@@ -1201,9 +1194,6 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi)
 
 	sms->imsi = g_strdup(imsi);
 
-	sms->next_msg_id = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
-							"NextMessageId", NULL);
-
 	sms->ref = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
 							"NextReference", NULL);
 	if (sms->ref >= 65536)
@@ -1306,10 +1296,11 @@ void *ofono_sms_get_data(struct ofono_sms *sms)
 	return sms->driver_data;
 }
 
-unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
-					unsigned int flags,
-					ofono_sms_txq_submit_cb_t cb,
-					void *data, ofono_destroy_func destroy)
+struct tx_queue_entry *__ofono_sms_txq_submit(
+	struct ofono_sms *sms, GSList *list,
+	unsigned int flags, unsigned msg_id,
+	ofono_sms_txq_submit_cb_t cb,
+	void *data, ofono_destroy_func destroy)
 {
 	struct tx_queue_entry *entry = tx_queue_entry_new(list);
 
@@ -1320,7 +1311,7 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
 				sizeof(entry->receiver));
 	}
 
-	entry->msg_id = sms->next_msg_id++;
+	entry->msg_id = msg_id;
 	entry->flags = flags;
 	entry->cb = cb;
 	entry->data = data;
@@ -1331,5 +1322,5 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
 	if (g_queue_get_length(sms->txq) == 1)
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 
-	return entry->msg_id;
+	return entry;
 }
diff --git a/src/stk.c b/src/stk.c
index 556dc68..620811b 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -323,6 +323,7 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
 	struct ofono_atom *sms_atom;
 	struct ofono_sms *sms;
 	GSList msg_list;
+	guint16 msg_id;
 
 	sms_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SMS);
 
@@ -338,8 +339,8 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
 
 	msg_list.data = (void *) &cmd->send_sms.gsm_sms;
 	msg_list.next = NULL;
-
-	__ofono_sms_txq_submit(sms, &msg_list, 0, send_sms_submit_cb,
+	msg_id = sms_uuid_from_pdu_list(&msg_list);
+	__ofono_sms_txq_submit(sms, &msg_list, 0, msg_id, send_sms_submit_cb,
 				stk->sms_submit_req, g_free);
 
 	stk->cancel_cmd = send_sms_cancel;
-- 
1.6.6.1

[SMS D-Bus 04/19] sms: document the org.ofono.SmsMessage interface

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

---
 doc/sms-api.txt |   49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/doc/sms-api.txt b/doc/sms-api.txt
index 1fc32ac..2bf588f 100644
--- a/doc/sms-api.txt
+++ b/doc/sms-api.txt
@@ -22,9 +22,27 @@ Methods		dict GetProperties()
 			Possible Errors: [service].Error.InvalidArguments
 					 [service].Error.DoesNotExist
 
-		void SendMessage(string to, string text)
+		object SendMessage(string to, string text)
 
-			Send the message in text to the number in to.
+			Submit a message for delivery /
+			processing. oFono owns it from now on until
+			successful delivery, cancellation (by the
+			user) or cancellation (by oFono). User has to
+			keep a copy around as oFono only offers
+			persistence to satisfy its needs.
+
+			Returns the name of the D-Bus object that
+			represents said SMS Message.
+
+			Possible Errors: [service].Error.InvalidArguments
+
+		array{object} Messages()
+
+			Returns a list of SMS Messages that have been
+			submitted for delivery and that are still
+			pending.
+
+			FIXME: currently not implemented
 
 Signals		PropertyChanged(string name, variant value)
 
@@ -64,3 +82,30 @@ Properties	string ServiceCenterAddress
 				"ps-preferred" - Use CS if PS is unavailable
 
 			By default oFono uses "cs-preferred" setting.
+
+
+SMS / Messaging interface
+=========================
+
+Service		org.ofono
+Interface	org.ofono.SmsMessage
+Object path	[variable prefix]/{modem0,modem1...}/{message01,message02...}
+
+Methods		dict GetProperties()
+
+			Returns a dictionary with the current
+			properties of a message.
+
+		void Cancel()
+
+			Cancels a pending message's delivery.
+
+Signals		PropertyChanged(string name, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	string State
+
+			Current transmission state {queued, wsr, done,
+			canceling, cancelled, failed, expired}
-- 
1.6.6.1

[SMS D-Bus 05/19] sms: document handle_sms_status_report()

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

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

diff --git a/src/sms.c b/src/sms.c
index 39d4a32..8d9a55e 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -859,6 +859,15 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
+
+/*
+ * Handle a delivery/status report has been received for an SMS
+ * apparently sent from here.
+ *
+ * We need to find the message in the SMS manager's
+ * waiting-for-acknoledge queue (sms->tx_wfaq) and remove it. As well,
+ * fill out history for it.
+ */
 static void handle_sms_status_report(struct ofono_sms *sms,
 						const struct sms *incoming)
 {
-- 
1.6.6.1

[SMS D-Bus 06/19] struct tx_queue_entry: add a destructor

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

Introduce a destructor function to encapsulate all the release steps
for this data type, as more are to be added later.
---
 src/sms.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 8d9a55e..fa489d1 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -405,6 +405,26 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg,
 	return __ofono_error_invalid_args(msg);
 }
 
+/*
+ * Destroy/release the contents of a 'struct tx_queue_entry'
+ *
+ * This releases resources allocated *inside* @entry and @entry
+ * itself.
+ */
+static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
+{
+	if (entry->destroy)
+		entry->destroy(entry->data);
+
+	g_free(entry->pdus);
+	g_free(entry);
+}
+
+static void tx_queue_entry_destroy_foreach(gpointer _entry, gpointer unused)
+{
+	tx_queue_entry_destroy(_entry);
+}
+
 static void tx_finished(const struct ofono_error *error, int mr, void *data)
 {
 	struct ofono_sms *sms = data;
@@ -465,11 +485,7 @@ next_q:
 						time(NULL), hs);
 	}
 
-	if (entry->destroy)
-		entry->destroy(entry->data);
-
-	g_free(entry->pdus);
-	g_free(entry);
+	tx_queue_entry_destroy(entry);
 
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
@@ -1101,7 +1117,7 @@ static void sms_remove(struct ofono_atom *atom)
 	}
 
 	if (sms->txq) {
-		g_queue_foreach(sms->txq, (GFunc)g_free, NULL);
+		g_queue_foreach(sms->txq, tx_queue_entry_destroy_foreach, NULL);
 		g_queue_free(sms->txq);
 		sms->txq = NULL;
 	}
-- 
1.6.6.1

[SMS D-Bus 07/19] sms: introduce bare state machine and transitions

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

This adds state to outgoing/in-transit SMS messages. This will be used
later on for persistence / D-Bus, when the SMS life cycle is expanded.

The state is a variable in the 'struct tx_queue_entry' which gets
updated as messages go through the hoops.
---
 src/ofono.h |   33 +++++++++++++++++
 src/sms.c   |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 148 insertions(+), 2 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index b73797b..0ba38ce 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -183,6 +183,39 @@ enum ofono_sms_submit_flag {
 	OFONO_SMS_SUBMIT_FLAG_RETRY =		0x4,
 };
 
+/*
+ * SMS TX message's state
+ *
+ * When a message is queued to be delivered, it will transition
+ * through a set of states.
+ *
+ * Allowed transition table (Allowed, Not-allowed) from left to right:
+ *
+ *          UNINITIALIZED         CANCELING  FAILED
+ *               | QUEUED WSR DONE   | CANCELLED  EXPIRED
+ * UNINITIALIZED -    A    N    N    N    N    N    N
+ * QUEUED        N    -    A    A    A    N    A    N
+ * WSR           N    N    -    A    A    N    A    A
+ * DONE          A    N    N    -    N    N    N    N
+ * CANCELING     N    N    N    N    -    A    A    A
+ * CANCELLED     A    N    N    N    N    -    N    N
+ * FAILED        A    N    N    N    N    N    -    N
+ * EXPIRED       A    N    N    N    N    N    N    -
+ */
+enum ofono_sms_tx_state {
+	OFONO_SMS_TX_ST_UNINITIALIZED,
+	OFONO_SMS_TX_ST_QUEUED,
+	OFONO_SMS_TX_ST_WSR,
+	OFONO_SMS_TX_ST_DONE,
+	OFONO_SMS_TX_ST_CANCELING,
+	OFONO_SMS_TX_ST_CANCELLED,
+	OFONO_SMS_TX_ST_FAILED,
+	OFONO_SMS_TX_ST_EXPIRED,
+	__OFONO_SMS_TX_ST_INVALID,
+};
+
+const char *ofono_sms_tx_state_to_string(enum ofono_sms_tx_state);
+
 typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
 
 struct tx_queue_entry *__ofono_sms_txq_submit(
diff --git a/src/sms.c b/src/sms.c
index fa489d1..5016d07 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -50,6 +50,22 @@ static gboolean tx_next(gpointer user_data);
 
 static GSList *g_drivers = NULL;
 
+const char *ofono_sms_tx_state_to_string(enum ofono_sms_tx_state status)
+{
+	switch (status) {
+	case OFONO_SMS_TX_ST_UNINITIALIZED:	return "uninitialized";
+	case OFONO_SMS_TX_ST_QUEUED:		return "queued";
+	case OFONO_SMS_TX_ST_WSR:		return "wsr";
+	case OFONO_SMS_TX_ST_DONE:		return "done";
+	case OFONO_SMS_TX_ST_CANCELING:		return "canceling";
+	case OFONO_SMS_TX_ST_CANCELLED:		return "cancelled";
+	case OFONO_SMS_TX_ST_FAILED:		return "failed";
+	case OFONO_SMS_TX_ST_EXPIRED:		return "expired";
+	default:
+						return "invalid";
+	}
+}
+
 struct ofono_sms {
 	int flags;
 	DBusMessage *pending;
@@ -77,11 +93,17 @@ struct pending_pdu {
 	int pdu_len;
 };
 
+/*
+ * @sms_mgr: SMS manager / driver object
+ * @state: Current state of the (in-transit) SMS
+ */
 struct tx_queue_entry {
+	struct ofono_sms *sms_mgr;
 	struct pending_pdu *pdus;
 	unsigned char num_pdus;
 	unsigned char cur_pdu;
 	struct sms_address receiver;
+	enum ofono_sms_tx_state state;
 	unsigned int msg_id;
 	unsigned int retry;
 	unsigned int flags;
@@ -405,6 +427,83 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg,
 	return __ofono_error_invalid_args(msg);
 }
 
+/* Check if a state transition is legal */
+static void ofono_sms_tx_state_check(const char *file, unsigned line,
+				     const struct tx_queue_entry *entry,
+				     enum ofono_sms_tx_state state_old,
+				     enum ofono_sms_tx_state state_new,
+				     unsigned states_allowed_bm)
+{
+	if (((1 << state_new) & states_allowed_bm) == 0)
+		ofono_warn("%s:%d: SW BUG? Forbidden state change "
+			   "%p %u -> %u\n",
+			   file, line, entry, state_old, state_new);
+}
+
+/*
+ * Set a pending SMS's state
+ *
+ * When leaving state, we do basic cleanup/release of resources
+ * allocated when we entered it.
+ *
+ * This is just syntactic sugar that validates that the transition is
+ * correct and warns out otherwise. The transition table is defined in
+ * the doc block for 'enum ofono_sms_tx_state'.
+ *
+ * In case of inconsistency, we just warn and press forward.
+ */
+#define ofono_sms_tx_state_set(entry, new_state) \
+	__ofono_sms_tx_state_set(entry, new_state, __FILE__, __LINE__)
+
+static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
+				     enum ofono_sms_tx_state state_new,
+				     const char *file, unsigned line)
+{
+	enum ofono_sms_tx_state state_old = entry->state;
+
+	if (state_new == state_old)
+		return;
+
+	switch (state_old) {
+	case OFONO_SMS_TX_ST_UNINITIALIZED:
+		ofono_sms_tx_state_check(
+			file, line, entry, state_old, state_new,
+			1 << OFONO_SMS_TX_ST_QUEUED);
+		break;
+	case OFONO_SMS_TX_ST_QUEUED:
+		ofono_sms_tx_state_check(
+			file, line, entry, state_old, state_new,
+			1 << OFONO_SMS_TX_ST_DONE
+			| 1 << OFONO_SMS_TX_ST_CANCELING
+			| 1 << OFONO_SMS_TX_ST_FAILED);
+		g_queue_remove(entry->sms_mgr->txq, entry);
+		break;
+	case OFONO_SMS_TX_ST_CANCELING:
+		ofono_sms_tx_state_check(
+			file, line, entry, state_old, state_new,
+			1 << OFONO_SMS_TX_ST_CANCELLED
+			| 1 << OFONO_SMS_TX_ST_FAILED
+			| 1 << OFONO_SMS_TX_ST_EXPIRED);
+		break;
+	case OFONO_SMS_TX_ST_DONE:
+	case OFONO_SMS_TX_ST_CANCELLED:
+	case OFONO_SMS_TX_ST_FAILED:
+	case OFONO_SMS_TX_ST_EXPIRED:
+		ofono_sms_tx_state_check(
+			file, line, entry, state_old, state_new,
+			1 << OFONO_SMS_TX_ST_UNINITIALIZED);
+		break;
+	case __OFONO_SMS_TX_ST_INVALID:
+	default:
+		ofono_warn("%s:%d: SW BUG? Bad state change %p %u -> %u\n",
+			   file, line, entry, state_old, state_new);
+	}
+	ofono_debug("%s:%d: SMS state change: %p %u -> %u\n",
+		    file, line, entry, state_old, state_new);
+	entry->state = state_new;
+}
+
+
 /*
  * Destroy/release the contents of a 'struct tx_queue_entry'
  *
@@ -417,6 +516,11 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 		entry->destroy(entry->data);
 
 	g_free(entry->pdus);
+
+	if (entry->state == OFONO_SMS_TX_ST_QUEUED)
+		g_queue_remove(entry->sms_mgr->txq, entry);
+
+	entry->state = __OFONO_SMS_TX_ST_INVALID;
 	g_free(entry);
 }
 
@@ -435,8 +539,10 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	DBG("tx_finished");
 
 	if (ok == FALSE) {
-		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY))
+		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY)) {
+			ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
 			goto next_q;
+		}
 
 		entry->retry += 1;
 
@@ -449,6 +555,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 		}
 
 		DBG("Max retries reached, giving up");
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
 		goto next_q;
 	}
 
@@ -468,7 +575,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	}
 
 next_q:
-	entry = g_queue_pop_head(sms->txq);
+	entry = g_queue_peek_head(sms->txq);
 
 	if (entry->cb)
 		entry->cb(ok, entry->data);
@@ -484,6 +591,10 @@ next_q:
 		__ofono_history_sms_send_status(modem, entry->msg_id,
 						time(NULL), hs);
 	}
+	if (ok)
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE);
+	else
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
 
 	tx_queue_entry_destroy(entry);
 
@@ -1341,8 +1452,10 @@ struct tx_queue_entry *__ofono_sms_txq_submit(
 	entry->cb = cb;
 	entry->data = data;
 	entry->destroy = destroy;
+	entry->sms_mgr = sms;
 
 	g_queue_push_tail(sms->txq, entry);
+	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_QUEUED);
 
 	if (g_queue_get_length(sms->txq) == 1)
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
-- 
1.6.6.1

[SMS D-Bus 08/19] sms: introduce the Wait-for-Status-Report state

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

A WSR (Waiting for Status Report) state definition is added to the
state transtition machine; as well, a queue (ofono_sms->tx_wfaq) where
messages waiting for an status report are queued. When the message's
delivery is acknowledged, the message is removed from the queue and
the message's status machine is updated, which can trigger things such
as D-Bus signals.
---
 src/sms.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 5016d07..8028ca2 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -66,6 +66,11 @@ const char *ofono_sms_tx_state_to_string(enum ofono_sms_tx_state status)
 	}
 }
 
+/**
+ * @tx_wsrq: Waiting-for-Status-Report queue; messages in this queue
+ *     have been delivered but are waiting to be acknoledged by the
+ *     network.
+ */
 struct ofono_sms {
 	int flags;
 	DBusMessage *pending;
@@ -73,6 +78,7 @@ struct ofono_sms {
 	struct sms_assembly *assembly;
 	guint ref;
 	GQueue *txq;
+	GQueue *tx_wsrq;
 	gint tx_source;
 	struct ofono_message_waiting *mw;
 	unsigned int mw_watch;
@@ -473,10 +479,19 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 	case OFONO_SMS_TX_ST_QUEUED:
 		ofono_sms_tx_state_check(
 			file, line, entry, state_old, state_new,
+			1 << OFONO_SMS_TX_ST_WSR
+			| 1 << OFONO_SMS_TX_ST_DONE
+			| 1 << OFONO_SMS_TX_ST_CANCELING);
+		g_queue_remove(entry->sms_mgr->txq, entry);
+		break;
+	case OFONO_SMS_TX_ST_WSR:
+		ofono_sms_tx_state_check(
+			file, line, entry, state_old, state_new,
 			1 << OFONO_SMS_TX_ST_DONE
 			| 1 << OFONO_SMS_TX_ST_CANCELING
-			| 1 << OFONO_SMS_TX_ST_FAILED);
-		g_queue_remove(entry->sms_mgr->txq, entry);
+			| 1 << OFONO_SMS_TX_ST_FAILED
+			| 1 << OFONO_SMS_TX_ST_EXPIRED);
+		g_queue_remove(entry->sms_mgr->tx_wsrq, entry);
 		break;
 	case OFONO_SMS_TX_ST_CANCELING:
 		ofono_sms_tx_state_check(
@@ -519,6 +534,8 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 
 	if (entry->state == OFONO_SMS_TX_ST_QUEUED)
 		g_queue_remove(entry->sms_mgr->txq, entry);
+	else if (entry->state == OFONO_SMS_TX_ST_WSR)
+		g_queue_remove(entry->sms_mgr->tx_wsrq, entry);
 
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
 	g_free(entry);
@@ -591,12 +608,16 @@ next_q:
 		__ofono_history_sms_send_status(modem, entry->msg_id,
 						time(NULL), hs);
 	}
-	if (ok)
+	if (ok && entry->flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR) {
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_WSR);
+		g_queue_push_tail(sms->tx_wsrq, entry);
+	} else if (ok && !(entry->flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR)) {
 		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE);
-	else
+		tx_queue_entry_destroy(entry);
+	} else {
 		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
-
-	tx_queue_entry_destroy(entry);
+		tx_queue_entry_destroy(entry);
+	}
 
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
@@ -986,13 +1007,45 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
+/*
+ * Pack information needed to locate a message from a Status Report
+ * and decide if it was succesfully delivered.
+ */
+struct sms_msg_sr_locator {
+	guint16 msg_id;
+	gboolean delivered;
+};
+
+/*
+ * This function is a g_queue_foreach() functor called by
+ * handle_sms_status_report() to destroy the message that matches the
+ * reported MSG-ID in the SMS manager's list of messages waiting for
+ * acknowledgement.
+ */
+static void  __sms_find_destroy_by_msg_id(gpointer _sms_msg, gpointer _locator)
+{
+	const struct sms_msg_sr_locator *locator = _locator;
+	struct tx_queue_entry *sms_msg = _sms_msg;
+
+	if (sms_msg->msg_id != locator->msg_id)
+		return;
+	ofono_debug("SMS: ACKED %p msg_id match %x", sms_msg, locator->msg_id);
+	g_queue_remove(sms_msg->sms_mgr->tx_wsrq, sms_msg);
+
+	if (locator->delivered)
+		ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_DONE);
+	else
+		ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_FAILED);
+
+	tx_queue_entry_destroy(sms_msg);
+}
 
 /*
  * Handle a delivery/status report has been received for an SMS
  * apparently sent from here.
  *
  * We need to find the message in the SMS manager's
- * waiting-for-acknoledge queue (sms->tx_wfaq) and remove it. As well,
+ * waiting-for-status report queue (sms->tx_wsrq) and remove it. As well,
  * fill out history for it.
  */
 static void handle_sms_status_report(struct ofono_sms *sms,
@@ -1001,11 +1054,16 @@ static void handle_sms_status_report(struct ofono_sms *sms,
 	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	gboolean delivered;
 	unsigned int msg_id;
+	struct sms_msg_sr_locator locator;
 
 	if (status_report_assembly_report(sms->sr_assembly, incoming, &msg_id,
 						&delivered) == FALSE)
 		return;
 
+	locator.msg_id = msg_id;
+	locator.delivered = delivered;
+	g_queue_foreach(sms->tx_wsrq, __sms_find_destroy_by_msg_id, &locator);
+
 	__ofono_history_sms_send_status(modem, msg_id, time(NULL),
 			delivered ? OFONO_HISTORY_SMS_STATUS_DELIVERED :
 			OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);
@@ -1233,6 +1291,13 @@ static void sms_remove(struct ofono_atom *atom)
 		sms->txq = NULL;
 	}
 
+	if (sms->tx_wsrq) {
+		g_queue_foreach(sms->tx_wsrq,
+				tx_queue_entry_destroy_foreach, NULL);
+		g_queue_free(sms->tx_wsrq);
+		sms->tx_wsrq = NULL;
+	}
+
 	if (sms->settings) {
 		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
 					"NextReference", sms->ref);
@@ -1287,6 +1352,7 @@ struct ofono_sms *ofono_sms_create(struct ofono_modem *modem,
 	sms->sca.type = 129;
 	sms->ref = 1;
 	sms->txq = g_queue_new();
+	sms->tx_wsrq = g_queue_new();
 	sms->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_SMS,
 						sms_remove, sms);
 
-- 
1.6.6.1

[SMS D-Bus 09/19] sms: introduce a state change callback for messages

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

When the SMS message going through the TX hoops changes states, call
the given callback function (if any). This will be used later to hook
up the D-Bus propagation of property changed signals.

Note this will do away with the ofono_sms_txq_submit_cb callback, as
it can be implemented in terms of a state change callback.
---
 src/ofono.h |    5 +++--
 src/sms.c   |   30 ++++++++++++++++++++++--------
 src/stk.c   |   19 +++++++++++++++++--
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index 0ba38ce..679c3c3 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -216,12 +216,13 @@ enum ofono_sms_tx_state {
 
 const char *ofono_sms_tx_state_to_string(enum ofono_sms_tx_state);
 
-typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
+typedef void (*ofono_sms_msg_stch_cb_t)(void *data,
+					enum ofono_sms_tx_state new_state);
 
 struct tx_queue_entry *__ofono_sms_txq_submit(
 	struct ofono_sms *sms, GSList *list,
 	unsigned int flags, unsigned ref,
-	ofono_sms_txq_submit_cb_t cb,
+	ofono_sms_msg_stch_cb_t stch_cb,
 	void *data, ofono_destroy_func destroy);
 
 #include <ofono/sim.h>
diff --git a/src/sms.c b/src/sms.c
index 8028ca2..127e40c 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -113,7 +113,7 @@ struct tx_queue_entry {
 	unsigned int msg_id;
 	unsigned int retry;
 	unsigned int flags;
-	ofono_sms_txq_submit_cb_t cb;
+	ofono_sms_msg_stch_cb_t stch_cb;
 	void *data;
 	ofono_destroy_func destroy;
 };
@@ -516,6 +516,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 	ofono_debug("%s:%d: SMS state change: %p %u -> %u\n",
 		    file, line, entry, state_old, state_new);
 	entry->state = state_new;
+	if (entry->stch_cb)
+		entry->stch_cb(entry->data, state_new);
 }
 
 
@@ -594,9 +596,6 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 next_q:
 	entry = g_queue_peek_head(sms->txq);
 
-	if (entry->cb)
-		entry->cb(ok, entry->data);
-
 	if (entry->flags & OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY) {
 		enum ofono_history_sms_status hs;
 
@@ -692,11 +691,26 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list)
 	return entry;
 }
 
-static void send_message_cb(gboolean ok, void *data)
+static void send_message_stch_cb(void *data, enum ofono_sms_tx_state new_state)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	DBusMessage *msg = data;
 	DBusMessage *reply;
+	gboolean ok;
+
+	/*
+	 * We care about only the final states
+	 * (DONE/CANCELLED/FAILED/EXPIRED), the rest are just
+	 * ignored.
+	 */
+	if (new_state == OFONO_SMS_TX_ST_DONE)
+		ok = TRUE;
+	else if (new_state == OFONO_SMS_TX_ST_CANCELLED
+		 || new_state == OFONO_SMS_TX_ST_FAILED
+		 || new_state == OFONO_SMS_TX_ST_EXPIRED)
+		ok = FALSE;
+	else
+		return;
 
 	if (ok)
 		reply = dbus_message_new_method_return(msg);
@@ -763,7 +777,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
 
 	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
-					 send_message_cb,
+					 send_message_stch_cb,
 					 dbus_message_ref(msg),
 					 send_message_destroy);
 
@@ -1501,7 +1515,7 @@ void *ofono_sms_get_data(struct ofono_sms *sms)
 struct tx_queue_entry *__ofono_sms_txq_submit(
 	struct ofono_sms *sms, GSList *list,
 	unsigned int flags, unsigned msg_id,
-	ofono_sms_txq_submit_cb_t cb,
+	ofono_sms_msg_stch_cb_t stch_cb,
 	void *data, ofono_destroy_func destroy)
 {
 	struct tx_queue_entry *entry = tx_queue_entry_new(list);
@@ -1515,7 +1529,7 @@ struct tx_queue_entry *__ofono_sms_txq_submit(
 
 	entry->msg_id = msg_id;
 	entry->flags = flags;
-	entry->cb = cb;
+	entry->stch_cb = stch_cb;
 	entry->data = data;
 	entry->destroy = destroy;
 	entry->sms_mgr = sms;
diff --git a/src/stk.c b/src/stk.c
index 620811b..a9c2d17 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -287,12 +287,27 @@ static void send_sms_cancel(struct ofono_stk *stk)
 	stk_alpha_id_unset(stk);
 }
 
-static void send_sms_submit_cb(gboolean ok, void *data)
+static void send_sms_stch_cb(void *data, enum ofono_sms_tx_state new_state)
 {
 	struct sms_submit_req *req = data;
 	struct ofono_stk *stk = req->stk;
 	struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE };
 	struct stk_response rsp;
+	gboolean ok;
+
+	/*
+	 * We care about only the final states
+	 * (DONE/CANCELLED/FAILED/EXPIRED), the rest are just
+	 * ignored.
+	 */
+	if (new_state == OFONO_SMS_TX_ST_DONE)
+		ok = TRUE;
+	else if (new_state == OFONO_SMS_TX_ST_CANCELLED
+		 || new_state == OFONO_SMS_TX_ST_FAILED
+		 || new_state == OFONO_SMS_TX_ST_EXPIRED)
+		ok = FALSE;
+	else
+		return;
 
 	ofono_debug("SMS submission %s", ok ? "successful" : "failed");
 
@@ -340,7 +355,7 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
 	msg_list.data = (void *) &cmd->send_sms.gsm_sms;
 	msg_list.next = NULL;
 	msg_id = sms_uuid_from_pdu_list(&msg_list);
-	__ofono_sms_txq_submit(sms, &msg_list, 0, msg_id, send_sms_submit_cb,
+	__ofono_sms_txq_submit(sms, &msg_list, 0, msg_id, send_sms_stch_cb,
 				stk->sms_submit_req, g_free);
 
 	stk->cancel_cmd = send_sms_cancel;
-- 
1.6.6.1

[SMS D-Bus 10/19] sms: export outgoing messages over D-Bus

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

Splits sms_send_message() into a D-Bus front end and an internal API:

- sms_msg_send(), a full C interface for SMS sending

- dbus_sms_msg_send(): adapts sms_msg_send() to be a D-Bus call,
  exporting the object on the bus, returning its name and setting up
  all the callbacks and D-Bus specific data. The call is synchronous.

This allows internal code to use the same infrastructure as D-Bus
clients to send SMS messages.
---
 src/sms.c     |  183 +++++++++++++++++++++++++++++++++++++++++----------------
 src/smsutil.h |    6 ++
 2 files changed, 137 insertions(+), 52 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 127e40c..06dfd08 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -41,6 +41,10 @@
 
 #define SMS_MANAGER_FLAG_CACHED 0x1
 
+/* D-Bus interface name for SMS messages (not for the manager!) */
+static
+const char SMS_MSG_INTERFACE[] = "org.ofono.SmsMessage";
+
 #define SETTINGS_STORE "sms"
 #define SETTINGS_GROUP "Settings"
 
@@ -522,6 +526,24 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 
 
 /*
+ * D-Bus SMS Message interface
+ *
+ * NOTE: the sms_msg_{methods,signals} tables should be const, but
+ * then g_dbus_register_interface() type warns.
+ */
+
+static
+GDBusMethodTable sms_msg_methods[] = {
+	{ }
+};
+
+static
+GDBusSignalTable sms_msg_signals[] = {
+	{ "PropertyChanged",	"sv"		},
+	{ }
+};
+
+/*
  * Destroy/release the contents of a 'struct tx_queue_entry'
  *
  * This releases resources allocated *inside* @entry and @entry
@@ -541,6 +563,7 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
 	g_free(entry);
+	ofono_debug("%s():%d: sms_msg %p freed", __func__, __LINE__, entry);
 }
 
 static void tx_queue_entry_destroy_foreach(gpointer _entry, gpointer unused)
@@ -687,52 +710,34 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list)
 		DBG("pdu_len: %d, tpdu_len: %d",
 				pdu->pdu_len, pdu->tpdu_len);
 	}
-
 	return entry;
 }
 
-static void send_message_stch_cb(void *data, enum ofono_sms_tx_state new_state)
+
+static void sms_msg_dbus_destroy(void *_dbus_path)
 {
-	DBusConnection *conn = ofono_dbus_get_connection();
-	DBusMessage *msg = data;
-	DBusMessage *reply;
-	gboolean ok;
+	char *dbus_path = _dbus_path;
 
-	/*
-	 * We care about only the final states
-	 * (DONE/CANCELLED/FAILED/EXPIRED), the rest are just
-	 * ignored.
-	 */
-	if (new_state == OFONO_SMS_TX_ST_DONE)
-		ok = TRUE;
-	else if (new_state == OFONO_SMS_TX_ST_CANCELLED
-		 || new_state == OFONO_SMS_TX_ST_FAILED
-		 || new_state == OFONO_SMS_TX_ST_EXPIRED)
-		ok = FALSE;
-	else
+	if (dbus_path == NULL)
 		return;
 
-	if (ok)
-		reply = dbus_message_new_method_return(msg);
-	else
-		reply = __ofono_error_failed(msg);
-
-	g_dbus_send_message(conn, reply);
+	g_dbus_unregister_interface(ofono_dbus_get_connection(),
+				    dbus_path, SMS_MSG_INTERFACE);
+	g_free(dbus_path);
 }
 
-static void send_message_destroy(void *data)
-{
-	DBusMessage *msg = data;
-
-	dbus_message_unref(msg);
-}
 
 /*
- * Pre-process a SMS text message and deliver it [D-Bus SendMessage()]
+ * Pre-process a SMS text message and deliver it
  *
  * @conn: D-Bus connection
  * @msg: message data (telephone number and text)
  * @data: SMS object to use for transmision
+ * @send_flags: flags that manipulate how the message is sent.
+ * @send_cb: function called when the message is sent
+ * @destroy_cb: function called when the message structure is
+ *    being released
+ * @priv: pointer to pass to the @data and @send_cb functions
  *
  * An alphabet is chosen for the text and it (might be) segmented in
  * fragments by sms_text_prepare() into @msg_list. A queue list @entry
@@ -740,12 +745,13 @@ static void send_message_destroy(void *data)
  * appends that entry to the SMS transmit queue. Then the tx_next()
  * function is scheduled to run to process the queue.
  */
-static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
-					void *data)
+static
+struct tx_queue_entry *sms_msg_send(
+	struct ofono_sms *sms, const char *to, const char *text,
+	enum sms_msg_send_flags send_flags,
+	ofono_sms_msg_stch_cb_t stch_cb,
+	void (*destroy_cb)(void *), void *priv)
 {
-	struct ofono_sms *sms = data;
-	const char *to;
-	const char *text;
 	GSList *msg_list;
 	int ref_offset;
 	struct ofono_modem *modem;
@@ -753,41 +759,115 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	guint16 msg_id;
 	struct tx_queue_entry *sms_msg;
 
-	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
-					DBUS_TYPE_STRING, &text,
-					DBUS_TYPE_INVALID))
-		return __ofono_error_invalid_args(msg);
-
 	if (valid_phone_number_format(to) == FALSE)
-		return __ofono_error_invalid_format(msg);
+		return NULL;
 
 	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
-					sms->use_delivery_reports);
+				    sms->use_delivery_reports);
 
 	if (!msg_list)
-		return __ofono_error_invalid_format(msg);
+		return NULL;
 
 	msg_id = sms_uuid_from_pdu_list(msg_list);
 	set_ref_and_to(msg_list, msg_id, ref_offset, to);
 	DBG("SMS ID: 0x%04x", msg_id);
 
-	flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
+	if (send_flags & SMS_MSG_SEND_HISTORY)
+		flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
 	flags |= OFONO_SMS_SUBMIT_FLAG_RETRY;
 	if (sms->use_delivery_reports)
 		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
 
 	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
-					 send_message_stch_cb,
-					 dbus_message_ref(msg),
-					 send_message_destroy);
+					 stch_cb, priv, destroy_cb);
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
 
 	modem = __ofono_atom_get_modem(sms->atom);
-	__ofono_history_sms_send_pending(modem, msg_id, to, time(NULL), text);
 
-	return NULL;
+	return sms_msg;
+}
+
+/*
+ * D-Bus: Send a SMS text message
+ *
+ * @conn: D-Bus connection
+ * @msg: message data (telephone number and text)
+ * @data: SMS object to use for transmision
+ *
+ * Unwraps the arguments and sends the request to sms_msg_send()
+ */
+static DBusMessage *dbus_sms_msg_send(DBusConnection *conn, DBusMessage *msg,
+				      void *data)
+{
+	struct ofono_sms *sms = data;
+	const char *to;
+	const char *text;
+	struct tx_queue_entry *sms_msg;
+	DBusMessage *reply;
+	char *dbus_path;
+	const char *sms_path = __ofono_atom_get_path(sms->atom);
+
+	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
+				   DBUS_TYPE_STRING, &text,
+				   DBUS_TYPE_INVALID))
+		return __ofono_error_invalid_args(msg);
+
+	/*
+	 * This is released by @ sms_msg_dbus_destroy() -- first we
+	 * allocate and then once we have the MSG ID, we re-write it.
+	 */
+	dbus_path = g_strdup_printf("%s/%04x", sms_path, 0);
+	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY,
+			       NULL,
+			       sms_msg_dbus_destroy,
+			       dbus_path);
+
+	if (sms_msg == NULL)
+		goto error_sms_msg_send;
+
+	/* Set the MSG ID in the D-Bus path now that we have it. */
+	sprintf(dbus_path, "%s/%04x", sms_path, sms_msg->msg_id & 0xffff);
+
+	if (!g_dbus_register_interface(ofono_dbus_get_connection(),
+				       dbus_path, SMS_MSG_INTERFACE,
+				       sms_msg_methods, sms_msg_signals,
+				       NULL, sms_msg, NULL)) {
+		ofono_error("%s():%d: %s: Could not create %s interface",
+			    __func__, __LINE__, dbus_path, SMS_MSG_INTERFACE);
+		goto error_dbus_register_interface;
+	}
+
+	ofono_debug("%s():%d: sms %p @ %s: MSG registered",
+		    __func__, __LINE__, sms_msg, dbus_path);
+
+	reply = dbus_message_new_method_return(msg);
+
+	if (!reply)
+		goto error_dbus_new_method_return;
+
+	dbus_message_append_args(reply,
+				 DBUS_TYPE_STRING, &dbus_path,
+				 DBUS_TYPE_INVALID);
+	return reply;
+
+
+error_dbus_new_method_return:
+	g_dbus_unregister_interface(ofono_dbus_get_connection(),
+				    dbus_path, SMS_MSG_INTERFACE);
+error_dbus_register_interface:
+	/*
+	 * Note we don't want the destructor called, as we do it's
+	 * steps manually here when falling through. Kind of layering
+	 * violation, but it is the cleaner way as things are
+	 * currently laid out.
+	 */
+	sms_msg->destroy = NULL;
+	tx_queue_entry_destroy(sms_msg);
+error_sms_msg_send:
+	g_free(dbus_path);
+	return __ofono_error_invalid_format(msg);
 }
 
 static GDBusMethodTable sms_manager_methods[] = {
@@ -795,8 +875,7 @@ static GDBusMethodTable sms_manager_methods[] = {
 							G_DBUS_METHOD_FLAG_ASYNC },
 	{ "SetProperty",	"sv",	"",		sms_set_property,
 							G_DBUS_METHOD_FLAG_ASYNC },
-	{ "SendMessage",	"ss",	"",		sms_send_message,
-							G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SendMessage",	"ss",	"",		dbus_sms_msg_send, 0 },
 	{ }
 };
 
diff --git a/src/smsutil.h b/src/smsutil.h
index 66486b7..4ceb721 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -209,6 +209,12 @@ enum cbs_geo_scope {
 	CBS_GEO_SCOPE_CELL_NORMAL
 };
 
+/* Flags to sms_msg_send() */
+enum sms_msg_send_flags {
+	/* Record/dont this message to the history database */
+	SMS_MSG_SEND_HISTORY = 0x01,
+};
+
 struct sms_address {
 	enum sms_number_type number_type;
 	enum sms_numbering_plan numbering_plan;
-- 
1.6.6.1

[SMS D-Bus 11/19] sms: send PropertyChanged signals on state change

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

This hooks sms_msg_stch_dbus_cb() into the SMS state change callback
so that a D-Bus signal will be emitted whenever an SMS Message
transitions state. This allows a client to track, if desired, what is
going on with the SMS Message it cares about.
---
 src/sms.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 06dfd08..fbaa271 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -790,6 +790,40 @@ struct tx_queue_entry *sms_msg_send(
 }
 
 /*
+ * Send a PropertyChange signal when the state changes
+ *
+ * In D-Bus we don't care about the initial state transition to
+ * _QUEUED, as the object path hasn't even been published yet in the
+ * bus.
+ */
+static
+void sms_msg_stch_dbus_cb(void *_dbus_path,
+			  enum ofono_sms_tx_state new_state)
+{
+	char *dbus_path = _dbus_path;
+	DBusConnection *dbus_conn = ofono_dbus_get_connection();
+	DBusMessage *dbus_signal;
+	DBusMessageIter iter;
+	const char *property = "State";
+	const char *new_state_str;
+
+	if (new_state == OFONO_SMS_TX_ST_QUEUED)
+		return;
+
+	dbus_signal = dbus_message_new_signal(
+		dbus_path, SMS_MSG_INTERFACE, "PropertyChanged");
+
+	if (!dbus_signal)
+		return;
+
+	dbus_message_iter_init_append(dbus_signal, &iter);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &property);
+	new_state_str = ofono_sms_tx_state_to_string(new_state);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &new_state_str);
+	g_dbus_send_message(dbus_conn, dbus_signal);
+}
+
+/*
  * D-Bus: Send a SMS text message
  *
  * @conn: D-Bus connection
@@ -820,7 +854,7 @@ static DBusMessage *dbus_sms_msg_send(DBusConnection *conn, DBusMessage *msg,
 	 */
 	dbus_path = g_strdup_printf("%s/%04x", sms_path, 0);
 	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY,
-			       NULL,
+			       sms_msg_stch_dbus_cb,
 			       sms_msg_dbus_destroy,
 			       dbus_path);
 
-- 
1.6.6.1

[SMS D-Bus 12/19] sms: introduce sms_msg_cancel and its D-Bus wrapper

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

This introduces the ability to cancel a pending SMS message,
accessible via an internal API and over a D-Bus wrapper.

Sending a note to the network to cancel an in-transit message is not
yet implemented.

Note the test case code requires follow up commits that propagate the
message's state changes over D-Bus.
---
 src/sms.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index fbaa271..a595f7d 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -525,6 +525,20 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 }
 
 
+
+static void sms_msg_cancel(struct tx_queue_entry *);
+/*
+ * Note that the D-Bus specific cleanups are taken care by the
+ * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send().
+ */
+static DBusMessage *dbus_sms_msg_cancel(
+	DBusConnection * conn, DBusMessage *msg, void *data)
+{
+	struct tx_queue_entry *sms_msg = data;
+	sms_msg_cancel(sms_msg);
+	return dbus_message_new_method_return(msg);
+}
+
 /*
  * D-Bus SMS Message interface
  *
@@ -534,6 +548,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 
 static
 GDBusMethodTable sms_msg_methods[] = {
+	{ "Cancel",		"",	"",
+	  dbus_sms_msg_cancel, 0 },
 	{ }
 };
 
@@ -579,6 +595,11 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	gboolean ok = error->type == OFONO_ERROR_TYPE_NO_ERROR;
 
 	DBG("tx_finished");
+	/*
+	 * Queue is empty? Messages might have been cancelled.
+	 */
+	if (entry == NULL)
+		return;
 
 	if (ok == FALSE) {
 		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY)) {
@@ -789,6 +810,40 @@ struct tx_queue_entry *sms_msg_send(
 	return sms_msg;
 }
 
+/**
+ * Cancel a pending SMS message
+ *
+ * @sms_msg: message to cancel
+ *
+ * This function cancels a message that is pending or being
+ * actively transmitted. Note that after this function returns, the
+ * @sms_msg handle is no longer valid.
+ *
+ * \internal
+ *
+ * There is no need to cancel the calling of tx_next() by
+ * g_timeout_add() scheduled in sms_msg_send(). The rationale behind
+ * this is that the tx_next() function is scheduled to go over the
+ * list of messages in the @sms object, so it might have been
+ * scheduled for other messages also rather than just for this one
+ * @sms_msg. By the time it gets to run, it might see the list empty
+ * or see other messages, but @sms_msg won't be there.
+ */
+static void sms_msg_cancel(struct tx_queue_entry *sms_msg)
+{
+	struct ofono_sms *sms = sms_msg->sms_mgr;
+	DBG("%s (%p)\n", __func__, sms_msg);
+
+	if (sms_msg->state == OFONO_SMS_TX_ST_QUEUED)
+		g_queue_remove(sms->txq, sms_msg);
+	else if (sms_msg->state == OFONO_SMS_TX_ST_WSR)
+		g_queue_remove(sms->tx_wsrq, sms_msg);
+
+	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELING);
+	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELLED);
+	tx_queue_entry_destroy(sms_msg);
+}
+
 /*
  * Send a PropertyChange signal when the state changes
  *
-- 
1.6.6.1

[SMS D-Bus 13/19] sms: Implement D-Bus SMS-MSG::GetProperties

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

Currently this only returns the state of the SMS message and the
destination number. Note we make the @receiver field in 'struct
tx_queue_entry' be always filled out in __ofono_sms_txq_submit() as
now it is always needed for the properties, not only when a Status
Report is requested.
---
 src/sms.c |   39 +++++++++++++++++++++++++++++++++------
 1 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index a595f7d..0aa058b 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -524,7 +524,35 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 		entry->stch_cb(entry->data, state_new);
 }
 
+static DBusMessage *dbus_sms_msg_get_properties(
+	DBusConnection * conn, DBusMessage *dbus_msg, void *_sms_msg)
+{
+	struct tx_queue_entry *sms_msg = _sms_msg;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	const char *str;
+
+	reply = dbus_message_new_method_return(dbus_msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
 
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					 OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					 &dict);
+
+	str = ofono_sms_tx_state_to_string(sms_msg->state);
+	ofono_dbus_dict_append(&dict, "State", DBUS_TYPE_STRING, &str);
+
+	str = sms_address_to_string(&sms_msg->receiver);
+	ofono_dbus_dict_append(&dict, "To", DBUS_TYPE_STRING, &str);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
 
 static void sms_msg_cancel(struct tx_queue_entry *);
 /*
@@ -548,6 +576,8 @@ static DBusMessage *dbus_sms_msg_cancel(
 
 static
 GDBusMethodTable sms_msg_methods[] = {
+	{ "GetProperties",	"",	"a{sv}",
+	  dbus_sms_msg_get_properties, 0 },
 	{ "Cancel",		"",	"",
 	  dbus_sms_msg_cancel, 0 },
 	{ }
@@ -1687,13 +1717,10 @@ struct tx_queue_entry *__ofono_sms_txq_submit(
 	void *data, ofono_destroy_func destroy)
 {
 	struct tx_queue_entry *entry = tx_queue_entry_new(list);
+	struct sms *head = list->data;
 
-	if (flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR) {
-		struct sms *head = list->data;
-
-		memcpy(&entry->receiver, &head->submit.daddr,
-				sizeof(entry->receiver));
-	}
+	memcpy(&entry->receiver, &head->submit.daddr,
+		sizeof(entry->receiver));
 
 	entry->msg_id = msg_id;
 	entry->flags = flags;
-- 
1.6.6.1

[SMS D-Bus 14/19] sms: document SMS Message's D-Bus 'To' property

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

---
 doc/sms-api.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/doc/sms-api.txt b/doc/sms-api.txt
index 2bf588f..70eb492 100644
--- a/doc/sms-api.txt
+++ b/doc/sms-api.txt
@@ -109,3 +109,8 @@ Properties	string State
 
 			Current transmission state {queued, wsr, done,
 			canceling, cancelled, failed, expired}
+
+		string To
+
+			Destination number to which the message is
+			being sent.
-- 
1.6.6.1

[SMS D-Bus 15/19] sms: test code for message's D-Bus GetProperties

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

---
 test/test-sms-msg-get-properties |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100755 test/test-sms-msg-get-properties

diff --git a/test/test-sms-msg-get-properties b/test/test-sms-msg-get-properties
new file mode 100755
index 0000000..eaac7b8
--- /dev/null
+++ b/test/test-sms-msg-get-properties
@@ -0,0 +1,26 @@
+#!/usr/bin/python
+
+import sys
+import dbus
+
+if len(sys.argv) != 2:
+    print "Error: missing argument (SMS message D-Bus object name)"
+    sys.exit(1)
+
+
+bus = dbus.SystemBus()
+
+manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+                         'org.ofono.Manager')
+
+properties = manager.GetProperties()
+
+path = properties["Modems"][0]
+
+manager = dbus.Interface(bus.get_object('org.ofono', path),
+                         'org.ofono.SmsManager')
+
+sms_msg = dbus.Interface(bus.get_object('org.ofono', sys.argv[1]),
+                         'org.ofono.SmsMessage')
+properties = sms_msg.GetProperties()
+print "To: %s\nState: %s\n" % (properties["To"], properties["State"])
-- 
1.6.6.1

[SMS D-Bus 16/19] automake: fix generation of symlinks for headers

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

When running 'make distcheck' from a vpath build directory (ie: one
that is not where the source lives), the target headers in
include/ofono are not generated properly due to a typo in the
Makefile.am, that is using _srcdir where _buildir should be being
used.
---
 Makefile.am |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e256841..313c555 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -444,7 +444,7 @@ include/ofono/version.h: include/version.h
 
 include/ofono/%.h: include/%.h
 	$(AM_V_at)$(MKDIR_P) include/ofono
-	$(AM_V_GEN)$(LN_S) $(abs_top_srcdir)/$< $@
+	$(AM_V_GEN)$(LN_S) $(abs_top_builddir)/$< $@
 
 clean-local:
 	@$(RM) -rf include/ofono
-- 
1.6.6.1

[SMS D-Bus 17/19] sms: document variable usage in sms_msg_send()

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

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

diff --git a/src/sms.c b/src/sms.c
index 0aa058b..0c5b4c5 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -795,6 +795,9 @@ static void sms_msg_dbus_destroy(void *_dbus_path)
  * is created by tx_queue_entry_new() and g_queue_push_tail()
  * appends that entry to the SMS transmit queue. Then the tx_next()
  * function is scheduled to run to process the queue.
+ *
+ * @sms is the main SMS driver struct, @entry and @msg_list represent
+ * the current message being processed.
  */
 static
 struct tx_queue_entry *sms_msg_send(
-- 
1.6.6.1

[SMS D-Bus 18/19] sms: add test case for message cancel

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

---
 test/test-sms-msg-cancel |  173 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100755 test/test-sms-msg-cancel

diff --git a/test/test-sms-msg-cancel b/test/test-sms-msg-cancel
new file mode 100755
index 0000000..e9bb40f
--- /dev/null
+++ b/test/test-sms-msg-cancel
@@ -0,0 +1,173 @@
+#!/usr/bin/python
+#
+# Sends a message and inmediately cancels it once the state moves to
+# "queued"; fails if the message doesn't move to the CANCELLED state.
+#
+# Arguments are: DEST-PHONE-NUMBER MSG-TEXT BOOLEAN
+#
+# BOOLEAN: request delivery confirmation / no
+#
+# Notes: accessing the globals result and reason using 'globals'
+#
+
+import gobject
+import sys
+import dbus
+import dbus.mainloop.glib
+
+dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+class sms_msg_cancel_test:
+        number = ""
+        text = ""
+        wsr = ""
+        target_state = ""
+
+        result = ""
+        reason = ""
+
+        sms_msg_path = ""
+
+        # FIXME
+        def __init__(self, number, text, wsr):
+                self.number = number
+                self.text = text
+                self.wsr = wsr
+                self.mainloop = gobject.MainLoop()
+
+                self.dbus = dbus.SystemBus()
+                ofono_manager = dbus.Interface(self.dbus.get_object('org.ofono', '/'),
+                                               'org.ofono.Manager')
+                path = ofono_manager.GetProperties()["Modems"][0]
+                self.sms_manager = dbus.Interface(self.dbus.get_object('org.ofono', path),
+                                                  'org.ofono.SmsManager')
+                self.sms_manager.SetProperty("UseDeliveryReports", dbus.Boolean(int(self.wsr)))
+
+                # Connect the signal *before* creating the message, otherwise we miss
+                # the transition - this also means we can't use sms_msg.connect_to_signal()
+                self.dbus.add_signal_receiver(self.property_changed, bus_name="org.ofono",
+                                              signal_name = "PropertyChanged",
+                                              path_keyword="path",
+                                              interface_keyword="interface")
+                self.result = "FAILED"
+                self.reason = "didn't see CANCELING state"
+
+        # Cancel the message and update test state
+        def msg_cancel(self):
+                print "I: %s[%s]: SMS message: canceling because " \
+                    "target state was reached" \
+                    % (self.sms_msg_path, self.target_state)
+                sms_msg = dbus.Interface(self.dbus.get_object('org.ofono', self.sms_msg_path),
+                                         'org.ofono.SmsMessage')
+                try:
+                        sms_msg.Cancel()
+                except dbus.DBusException as dbus_ex:
+                        if dbus_ex.get_dbus_name() != 'org.freedesktop.DBus.Error.UnknownMethod':
+                                pass
+                        # Object has been cancelled already--this is
+                        # why we get the D-Bus exception, so we can
+                        # consider this a win
+                        print "I: %s[%s]: SMS message: ok, D-Bus object already removed" \
+                            % (self.sms_msg_path, self.target_state)
+                        self.result = "SUCCESS"
+                        self.reason = "object dissapeared from the bus, meaning it was cancelled"
+                        self.mainloop.quit()
+                        return
+
+        # Run the actual test FIXME
+        def run(self, target_state):
+                self.target_state = target_state
+
+                self.result = "FAILED"
+                self.reason = "didn't see %s state to allow cancelling" \
+                    % target_state
+
+                print "I: [%s]: starting test" % self.target_state
+                self.sms_msg_path = self.sms_manager.SendMessage(number, text)
+                print "I: %s[%s]: SMS message: D-Bus object created" \
+                    % (self.sms_msg_path, self.target_state)
+
+                # wait for property changed signals, property_changed() will be called
+                # when there is a transition and the thing cancelled.
+                gobject.timeout_add(20000, self.timeout_fail)
+                # If the target state to cancel on is QUEUED, schedule a
+                # cancellation inmediately -- note we need to schedule it in
+                # the mainloop to ensure property_changed() signals are
+                # properly propagated.
+                if target_state == "queued" \
+                            or target_state == "canceling" \
+                            or target_state == "cancelled":
+                        gobject.idle_add(self.msg_cancel)
+                self.mainloop.run()
+                if self.result == "SUCCESS":
+                        letter = "I"
+                        result = 0
+                else:
+                        letter = "E"
+                        result = 1
+                print "%s: %s[%s]: %s: %s" % (letter, self.sms_msg_path,
+                                              self.target_state,
+                                              self.result, self.reason)
+                return result
+
+        def timeout_fail(self):
+                self.result = "FAILED"
+                self.reason = "%s (timedout)" % self.reason
+                self.mainloop.quit()
+                # Don't rearm the timeout ... doesn't really matter as we
+                # quit the mainloop.
+                return 0
+
+
+        # When the message's state (the one we created) switches to QUEUED, cancel
+        def property_changed(self, property_name, property_value,
+                             path, interface):
+                if interface != "org.ofono.SmsMessage":
+                        return
+                if self.sms_msg_path != path:
+                        return
+                if property_name != "State":
+                        return
+                print "I: %s[%s]: SMS message: transitioning to %s state" \
+                    % (self.sms_msg_path, self.target_state, property_value)
+                if property_value == "canceling":
+                        print "I: %s[%s]: SMS message: ok, saw canceling state" \
+                            % (self.sms_msg_path, self.target_state)
+                        self.result = "FAIL"
+                        self.reason = "didn't see the CANCELLED state"
+                if property_value == "cancelled":
+                        print "I: %s[%s]: SMS message: ok, saw cancelled state, quiting" \
+                            % (self.sms_msg_path, self.target_state)
+                        self.result = "SUCCESS"
+                        self.reason = "saw CANCELLED state"
+                        self.mainloop.quit()
+                # If we get to the state that is the target state for
+                # the test, cancel the message -- we need to do this
+                # after evaluating propery_value or we might enter
+                # into conflicts (as self.msg_cancel() will change
+                # self.{result,reason} if it turns out the operation
+                # is complete).
+                if self.target_state == property_value:
+                        # Cancel the message
+                        self.msg_cancel()
+
+
+# Send the message
+if len(sys.argv) == 4:
+        wsr = int(sys.argv[3])
+elif len(sys.argv) == 3:
+        wsr = 0
+else:
+        raise NameError("E: Bad number of arguments (expected NUMBER TEXT [BOOLEAN])")
+
+number = sys.argv[1]
+text = sys.argv[2]
+global test
+
+test = sms_msg_cancel_test(number, text, wsr)
+
+test.run('queued')
+test.run('wsr')
+test.run('canceling')
+test.run('cancelled')
+test.run('done')
-- 
1.6.6.1

[SMS D-Bus 19/19] sms: add test case for state change signals

[Inaky Perez-Gonzalez]

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

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

---
 test/test-sms-msg-state-change |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
 create mode 100755 test/test-sms-msg-state-change

diff --git a/test/test-sms-msg-state-change b/test/test-sms-msg-state-change
new file mode 100755
index 0000000..331b722
--- /dev/null
+++ b/test/test-sms-msg-state-change
@@ -0,0 +1,24 @@
+#!/usr/bin/python
+
+import gobject
+
+import dbus
+import dbus.mainloop.glib
+
+def property_changed(name, value, path, interface):
+        if interface == "org.ofono.SmsMessage":
+                print "%s: name %s value %s interface %s" \
+                    % (path, name, value, interface)
+
+if __name__ == '__main__':
+        print "FIXME: this should test the whole state change transition machine"
+	dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+	bus = dbus.SystemBus()
+
+	bus.add_signal_receiver(
+                property_changed, bus_name="org.ofono",
+                signal_name = "PropertyChanged", path_keyword="path",
+                interface_keyword="interface")
+	mainloop = gobject.MainLoop()
+	mainloop.run()
-- 
1.6.6.1

Re: [SMS D-Bus 01/19] write_file: make transaction-safe

[Denis Kenzior]

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

Hi Inaky,

On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> write_file(), as written wasn't transaction-safe; a crash bewtween a
> file being open and the buffer being written before a safe close would
> leave the file with a set of undetermined contents.
> 
> Modified to the file is written to a temporary file name; once
> completed, it is renamed to the final name. This way, a crash in the
> middle doesn't leave half-baked files.

This patch has now been applied.

Regards,
-Denis

Re: [SMS D-Bus 06/19] struct tx_queue_entry: add a destructor

[Denis Kenzior]

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

Hi Inaky,

On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> Introduce a destructor function to encapsulate all the release steps
> for this data type, as more are to be added later.

This patch has been applied.  Thanks.

Regards,
-Denis

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Denis Kenzior]

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

Hi Inaky,

On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> This adds a simple API to use for generating unique IDs for SMS
> messages. Will be used by follow up commits.
> 
> The ID is not generic, but specifc to SMS messages (due to having to
> dig inside 'struct sms') and thus generates directly 16-bit IDs.
> ---
>  src/smsutil.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/smsutil.h |    2 +
>  2 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 22c70cf..b958ee0 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -3983,3 +3983,81 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)
>  
>  	return utf8;
>  }
> +
> +static
> +int __sms_uuid_from_pdu(GChecksum *checksum, const struct sms *sms_pdu)

Since this is a static function, it is preferable not to prefix it with __

> +{
> +	const guint8 *buf;
> +	size_t buf_len;
> +
> +	switch (sms_pdu->type) {
> +	case SMS_TYPE_DELIVER:
> +		buf = sms_pdu->deliver.ud;
> +		buf_len = sms_pdu->deliver.udl;
> +		break;
> +	case SMS_TYPE_DELIVER_REPORT_ACK:
> +		buf = sms_pdu->deliver_ack_report.ud;
> +		buf_len = sms_pdu->deliver_ack_report.udl;
> +		break;
> +	case SMS_TYPE_DELIVER_REPORT_ERROR:
> +		buf = sms_pdu->deliver_err_report.ud;
> +		buf_len = sms_pdu->deliver_err_report.udl;
> +		break;
> +	case SMS_TYPE_STATUS_REPORT:
> +		buf = sms_pdu->status_report.ud;
> +		buf_len = sms_pdu->status_report.udl;
> +		break;
> +	case SMS_TYPE_SUBMIT:
> +		buf = sms_pdu->submit.ud;
> +		buf_len = sms_pdu->submit.udl;
> +		break;
> +	case SMS_TYPE_SUBMIT_REPORT_ACK:
> +		buf = sms_pdu->submit_ack_report.ud;
> +		buf_len = sms_pdu->submit_ack_report.udl;
> +		break;
> +	case SMS_TYPE_SUBMIT_REPORT_ERROR:
> +		buf = sms_pdu->submit_err_report.ud;
> +		buf_len = sms_pdu->submit_err_report.udl;
> +		break;
> +	case SMS_TYPE_COMMAND:
> +		buf = sms_pdu->command.cd;
> +		buf_len = sms_pdu->command.cdl;
> +		break;

This part is wrong, the user-data (ud) is not the entirety of the PDU.
It is just the message payload.  Hashing over it only is not enough.

> +	default:
> +		return 1;

You might want to make this function return TRUE/FALSE...

> +	}
> +	g_checksum_update(checksum, buf, buf_len);

An empty line before g_checksum_update()
> +	return 0;
> +}
> +
> +/**
> + * Generate a UUID from an SMS PDU List
> + *
> + * @param sms_pdu_list GSlist containing 'struct sms' nodes to
> + *     generate the UUID from.
> + * @return 0 in error (no memory or serious code inconsistency in the
> + *     input data structures), otherwise the SMS UUID.
> + */
> +guint16 sms_uuid_from_pdu_list(const GSList *sms_pdu_list)
> +{
> +	guint16 uuid = 0;
> +	GChecksum *checksum;
> +	const GSList *node;
> +	gsize uuid_size = g_checksum_type_get_length(G_CHECKSUM_SHA256);
> +	guint8 data[uuid_size];
> +
> +	checksum = g_checksum_new(G_CHECKSUM_SHA256);
> +	if (checksum == NULL)
> +		goto error_new;
> +	for (node = sms_pdu_list; node; node = node->next) {
> +		struct sms *sms_pdu = node->data;
> +		if (__sms_uuid_from_pdu(checksum, sms_pdu))
> +			goto error_pdu;
> +	}
> +	g_checksum_get_digest(checksum, data, &uuid_size);
> +	uuid = data[0] | data[1] << 8;

The current message id is 32 bits.  Why are we stopping@16?  We could
go all the way to 64 if needed...

Also, we need to take time and some random value into account.  Perhaps
including time() output and a simple static counter into the hashing
function would make it unique for those 'same sender, same contents
messages'

> +error_pdu:
> +	g_checksum_free(checksum);
> +error_new:
> +	return uuid;
> +}
> diff --git a/src/smsutil.h b/src/smsutil.h
> index ca64b18..66486b7 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -547,3 +547,5 @@ GSList *cbs_optimize_ranges(GSList *ranges);
>  gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges);
>  
>  char *ussd_decode(int dcs, int len, const unsigned char *data);
> +
> +guint16 sms_uuid_from_pdu_list(const GSList *sms_pdu_list);

Regards,
-Denis

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Inaky Perez-Gonzalez]

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

On Thu, 2010-08-05 at 12:10 -0500, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > This adds a simple API to use for generating unique IDs for SMS
> > messages. Will be used by follow up commits.
> > 
> > The ID is not generic, but specifc to SMS messages (due to having to
> > dig inside 'struct sms') and thus generates directly 16-bit IDs.
> > ---
> >  src/smsutil.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/smsutil.h |    2 +
> >  2 files changed, 80 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/smsutil.c b/src/smsutil.c
> > index 22c70cf..b958ee0 100644
> > --- a/src/smsutil.c
> > +++ b/src/smsutil.c
> > @@ -3983,3 +3983,81 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)
> >  
> >  	return utf8;
> >  }
> > +
> > +static
> > +int __sms_uuid_from_pdu(GChecksum *checksum, const struct sms *sms_pdu)
> 
> Since this is a static function, it is preferable not to prefix it with __

Will do

> > +{
> > +	const guint8 *buf;
> > +	size_t buf_len;
> > +
> > +	switch (sms_pdu->type) {
> > +	case SMS_TYPE_DELIVER:
> > +		buf = sms_pdu->deliver.ud;
> > +		buf_len = sms_pdu->deliver.udl;
> > +		break;
> > +	case SMS_TYPE_DELIVER_REPORT_ACK:
> > +		buf = sms_pdu->deliver_ack_report.ud;
> > +		buf_len = sms_pdu->deliver_ack_report.udl;
> > +		break;
> > +	case SMS_TYPE_DELIVER_REPORT_ERROR:
> > +		buf = sms_pdu->deliver_err_report.ud;
> > +		buf_len = sms_pdu->deliver_err_report.udl;
> > +		break;
> > +	case SMS_TYPE_STATUS_REPORT:
> > +		buf = sms_pdu->status_report.ud;
> > +		buf_len = sms_pdu->status_report.udl;
> > +		break;
> > +	case SMS_TYPE_SUBMIT:
> > +		buf = sms_pdu->submit.ud;
> > +		buf_len = sms_pdu->submit.udl;
> > +		break;
> > +	case SMS_TYPE_SUBMIT_REPORT_ACK:
> > +		buf = sms_pdu->submit_ack_report.ud;
> > +		buf_len = sms_pdu->submit_ack_report.udl;
> > +		break;
> > +	case SMS_TYPE_SUBMIT_REPORT_ERROR:
> > +		buf = sms_pdu->submit_err_report.ud;
> > +		buf_len = sms_pdu->submit_err_report.udl;
> > +		break;
> > +	case SMS_TYPE_COMMAND:
> > +		buf = sms_pdu->command.cd;
> > +		buf_len = sms_pdu->command.cdl;
> > +		break;
> 
> This part is wrong, the user-data (ud) is not the entirety of the PDU.
> It is just the message payload.  Hashing over it only is not enough.

So which other fields for each type in the union are to be taken into
account?

From our discussions, I was under the impression that we wanted to hash
only on the contents of the message and destination -- this actually led
to the problem of the same sender and data issue below...[more on it
below].

> > +	default:
> > +		return 1;
> 
> You might want to make this function return TRUE/FALSE...

Ok 

> > +	}
> > +	g_checksum_update(checksum, buf, buf_len);
> 
> An empty line before g_checksum_update()

k

> > +	return 0;
> > +}
> > +
> > +/**
> > + * Generate a UUID from an SMS PDU List
> > + *
> > + * @param sms_pdu_list GSlist containing 'struct sms' nodes to
> > + *     generate the UUID from.
> > + * @return 0 in error (no memory or serious code inconsistency in the
> > + *     input data structures), otherwise the SMS UUID.
> > + */
> > +guint16 sms_uuid_from_pdu_list(const GSList *sms_pdu_list)
> > +{
> > +	guint16 uuid = 0;
> > +	GChecksum *checksum;
> > +	const GSList *node;
> > +	gsize uuid_size = g_checksum_type_get_length(G_CHECKSUM_SHA256);
> > +	guint8 data[uuid_size];
> > +
> > +	checksum = g_checksum_new(G_CHECKSUM_SHA256);
> > +	if (checksum == NULL)
> > +		goto error_new;
> > +	for (node = sms_pdu_list; node; node = node->next) {
> > +		struct sms *sms_pdu = node->data;
> > +		if (__sms_uuid_from_pdu(checksum, sms_pdu))
> > +			goto error_pdu;
> > +	}
> > +	g_checksum_get_digest(checksum, data, &uuid_size);
> > +	uuid = data[0] | data[1] << 8;
> 
> The current message id is 32 bits.  Why are we stopping at 16?  We could
> go all the way to 64 if needed...

Well, at the end what gets sent to the network is 16 bits right?

> Also, we need to take time and some random value into account.  Perhaps
> including time() output and a simple static counter into the hashing
> function would make it unique for those 'same sender, same contents
> messages'

So then we really don't care about being able to regenerate the hash ID
from the contents?

The design discussions we held about this involved being able to, with
the content of the message and destination number, generating a hash
that could be uniquely reproduced by rehashing it again. You had a
reason for it and I can't remember which one it was -- I should have
written it down. But that brought the problem of 'somebody sending the
same message to the same number' which yields the same ID.

However, if to fix that we are adding randomness in the form of time(),
why bother with the contents? let's just do a random ID from the ground
up and save us the pain of hashing.

Re: [SMS D-Bus 03/19] sms: implement SHA256-based message IDs [incomplete]

[Denis Kenzior]

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

Hi Inaky,

On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> This patch removes the sequential SMS message identification gig and
> replaces it with a 16-bit hash cookie based off message contents.
> 
> FIXME: [incomplete] need to figure out how to do so that identical
> messages sent to different or the same numbers also have a different
> ID.
> ---
>  src/ofono.h |    9 +++++----
>  src/sms.c   |   55 +++++++++++++++++++++++--------------------------------
>  src/stk.c   |    5 +++--
>  3 files changed, 31 insertions(+), 38 deletions(-)
> 
> diff --git a/src/ofono.h b/src/ofono.h
> index aaa01d9..b73797b 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -185,10 +185,11 @@ enum ofono_sms_submit_flag {
>  
>  typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
>  
> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> -					unsigned int flags,
> -					ofono_sms_txq_submit_cb_t cb,
> -					void *data, ofono_destroy_func destroy);
> +struct tx_queue_entry *__ofono_sms_txq_submit(
> +	struct ofono_sms *sms, GSList *list,
> +	unsigned int flags, unsigned ref,
> +	ofono_sms_txq_submit_cb_t cb,
> +	void *data, ofono_destroy_func destroy);

As I said before, please don't mess with this function.

>  
>  #include <ofono/sim.h>
>  #include <ofono/stk.h>
> diff --git a/src/sms.c b/src/sms.c
> index 35364db..39d4a32 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -55,7 +55,6 @@ struct ofono_sms {
>  	DBusMessage *pending;
>  	struct ofono_phone_number sca;
>  	struct sms_assembly *assembly;
> -	unsigned int next_msg_id;
>  	guint ref;
>  	GQueue *txq;
>  	gint tx_source;
> @@ -589,7 +588,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  	int ref_offset;
>  	struct ofono_modem *modem;
>  	unsigned int flags;
> -	unsigned int msg_id;
> +	guint16 msg_id;
> +	struct tx_queue_entry *sms_msg;
>  
>  	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
>  					DBUS_TYPE_STRING, &text,
> @@ -605,24 +605,19 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  	if (!msg_list)
>  		return __ofono_error_invalid_format(msg);
>  
> -	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
> -	DBG("ref: %d, offset: %d", sms->ref, ref_offset);
> -
> -	if (ref_offset != 0) {
> -		if (sms->ref == 65536)
> -			sms->ref = 1;
> -		else
> -			sms->ref = sms->ref + 1;
> -	}
> +	msg_id = sms_uuid_from_pdu_list(msg_list);

You're calculating the ID before setting the ref and to? That is pretty
silly ;)

> +	set_ref_and_to(msg_list, msg_id, ref_offset, to);
> +	DBG("SMS ID: 0x%04x", msg_id);
>  
>  	flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
>  	flags |= OFONO_SMS_SUBMIT_FLAG_RETRY;
>  	if (sms->use_delivery_reports)
>  		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
>  
> -	msg_id = __ofono_sms_txq_submit(sms, msg_list, flags, send_message_cb,
> -						dbus_message_ref(msg),
> -						send_message_destroy);
> +	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
> +					 send_message_cb,
> +					 dbus_message_ref(msg),
> +					 send_message_destroy);

Make the ID calculation happen inside tx_queue_entry_new.  That function
already converts all the SMS objects into a PDU.  So simply hashing the
PDUs there would be very easy.

>  
>  	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
>  	g_slist_free(msg_list);
> @@ -659,7 +654,7 @@ static void dispatch_app_datagram(struct ofono_sms *sms, int dst, int src,
>  }
>  
>  static void dispatch_text_message(struct ofono_sms *sms,
> -					const char *message,
> +					const char *message, guint16 msg_id,
>  					enum sms_class cls,
>  					const struct sms_address *addr,
>  					const struct sms_scts *scts)
> @@ -717,11 +712,9 @@ static void dispatch_text_message(struct ofono_sms *sms,
>  
>  	g_dbus_send_message(conn, signal);
>  
> -	if (cls != SMS_CLASS_0) {
> -		__ofono_history_sms_received(modem, sms->next_msg_id, str,
> +	if (cls != SMS_CLASS_0)
> +		__ofono_history_sms_received(modem, msg_id, str,
>  						&remote, &local, message);
> -		sms->next_msg_id += 1;
> -	}
>  }
>  
>  static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
> @@ -732,6 +725,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  	enum sms_class cls;
>  	int srcport = -1;
>  	int dstport = -1;
> +	guint16 msg_id;
>  
>  	if (sms_list == NULL)
>  		return;
> @@ -749,6 +743,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  	 * used, the addresses are the same across all segments.
>  	 */
>  
> +	msg_id = sms_uuid_from_pdu_list(sms_list);
>  	for (l = sms_list; l; l = l->next) {
>  		guint8 dcs;
>  		gboolean comp = FALSE;
> @@ -825,8 +820,8 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  
>  		s = sms_list->data;
>  
> -		dispatch_text_message(sms, message, cls, &s->deliver.oaddr,
> -					&s->deliver.scts);
> +		dispatch_text_message(sms, message, msg_id, cls,
> +					&s->deliver.oaddr, &s->deliver.scts);
>  		g_free(message);
>  	}
>  }
> @@ -1104,8 +1099,6 @@ static void sms_remove(struct ofono_atom *atom)
>  
>  	if (sms->settings) {
>  		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
> -					"NextMessageId", sms->next_msg_id);
> -		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
>  					"NextReference", sms->ref);
>  		g_key_file_set_boolean(sms->settings, SETTINGS_GROUP,
>  					"UseDeliveryReports",
> @@ -1201,9 +1194,6 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi)
>  
>  	sms->imsi = g_strdup(imsi);
>  
> -	sms->next_msg_id = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
> -							"NextMessageId", NULL);
> -
>  	sms->ref = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
>  							"NextReference", NULL);
>  	if (sms->ref >= 65536)
> @@ -1306,10 +1296,11 @@ void *ofono_sms_get_data(struct ofono_sms *sms)
>  	return sms->driver_data;
>  }
>  
> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> -					unsigned int flags,
> -					ofono_sms_txq_submit_cb_t cb,
> -					void *data, ofono_destroy_func destroy)
> +struct tx_queue_entry *__ofono_sms_txq_submit(
> +	struct ofono_sms *sms, GSList *list,
> +	unsigned int flags, unsigned msg_id,
> +	ofono_sms_txq_submit_cb_t cb,
> +	void *data, ofono_destroy_func destroy)
>  {
>  	struct tx_queue_entry *entry = tx_queue_entry_new(list);
>  
> @@ -1320,7 +1311,7 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  				sizeof(entry->receiver));
>  	}
>  
> -	entry->msg_id = sms->next_msg_id++;
> +	entry->msg_id = msg_id;
>  	entry->flags = flags;
>  	entry->cb = cb;
>  	entry->data = data;
> @@ -1331,5 +1322,5 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  	if (g_queue_get_length(sms->txq) == 1)
>  		sms->tx_source = g_timeout_add(0, tx_next, sms);
>  
> -	return entry->msg_id;
> +	return entry;
>  }
> diff --git a/src/stk.c b/src/stk.c
> index 556dc68..620811b 100644
> --- a/src/stk.c
> +++ b/src/stk.c
> @@ -323,6 +323,7 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  	struct ofono_atom *sms_atom;
>  	struct ofono_sms *sms;
>  	GSList msg_list;
> +	guint16 msg_id;
>  
>  	sms_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SMS);
>  
> @@ -338,8 +339,8 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  
>  	msg_list.data = (void *) &cmd->send_sms.gsm_sms;
>  	msg_list.next = NULL;
> -
> -	__ofono_sms_txq_submit(sms, &msg_list, 0, send_sms_submit_cb,
> +	msg_id = sms_uuid_from_pdu_list(&msg_list);
> +	__ofono_sms_txq_submit(sms, &msg_list, 0, msg_id, send_sms_submit_cb,
>  				stk->sms_submit_req, g_free);
>  
>  	stk->cancel_cmd = send_sms_cancel;

Regards,
-Denis

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Denis Kenzior]

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

Hi Inaky,

>> The current message id is 32 bits.  Why are we stopping at 16?  We could
>> go all the way to 64 if needed...
> 
> Well, at the end what gets sent to the network is 16 bits right?

So I think we're all confused here.  This ID is not being sent to the
network.  It is only used for uniquely identifying the message over
D-Bus and to message history.  This number can be as small or as big as
we want.

> 
>> Also, we need to take time and some random value into account.  Perhaps
>> including time() output and a simple static counter into the hashing
>> function would make it unique for those 'same sender, same contents
>> messages'
> 
> So then we really don't care about being able to regenerate the hash ID
> from the contents?
> 
> The design discussions we held about this involved being able to, with
> the content of the message and destination number, generating a hash
> that could be uniquely reproduced by rehashing it again. You had a
> reason for it and I can't remember which one it was -- I should have
> written it down. But that brought the problem of 'somebody sending the
> same message to the same number' which yields the same ID.
> 
> However, if to fix that we are adding randomness in the form of time(),
> why bother with the contents? let's just do a random ID from the ground
> up and save us the pain of hashing.
> 

I don't remember this discussion any more.  There might be a case where
an old message is re-delivered because oFono crashed after delivery but
before deletion.

At least to me it seems that generating a random int is really not
guaranteed to be unique over reboots.  Hashing seems way better.
However, I'm not a math/crypto geek, so feel free to prove me wrong.

Regards,
-Denis

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Inaky Perez-Gonzalez]

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

On Thu, 2010-08-05 at 13:02 -0500, Denis Kenzior wrote: 
> Hi Inaky,
> 
> >> The current message id is 32 bits.  Why are we stopping at 16?  We could
> >> go all the way to 64 if needed...
> > 
> > Well, at the end what gets sent to the network is 16 bits right?
> 
> So I think we're all confused here.  This ID is not being sent to the
> network.  It is only used for uniquely identifying the message over
> D-Bus and to message history.  This number can be as small or as big as
> we want.

I was under the impression that you wanted to use this for the ref also,
and thus it is imprinted into the PDUs by set_ref_and_to(). Can you
confirm this?

> >> Also, we need to take time and some random value into account.  Perhaps
> >> including time() output and a simple static counter into the hashing
> >> function would make it unique for those 'same sender, same contents
> >> messages'
> > 
> > So then we really don't care about being able to regenerate the hash ID
> > from the contents?
> > 
> > The design discussions we held about this involved being able to, with
> > the content of the message and destination number, generating a hash
> > that could be uniquely reproduced by rehashing it again. You had a
> > reason for it and I can't remember which one it was -- I should have
> > written it down. But that brought the problem of 'somebody sending the
> > same message to the same number' which yields the same ID.
> > 
> > However, if to fix that we are adding randomness in the form of time(),
> > why bother with the contents? let's just do a random ID from the ground
> > up and save us the pain of hashing.
> > 
> 
> I don't remember this discussion any more.  There might be a case where
> an old message is re-delivered because oFono crashed after delivery but
> before deletion.
> 
> At least to me it seems that generating a random int is really not
> guaranteed to be unique over reboots.  Hashing seems way better.
> However, I'm not a math/crypto geek, so feel free to prove me wrong.

Ok, I see your concern. Proper seeding of the RNG should take care of
that case, but either way works. If you want hashing + time() seeding
that can be done easily.

I still could use clarification on which other fields of the 'struct
sms' union you want to have hashed other than the PDU payloads.

Re: [SMS D-Bus 03/19] sms: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

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

On Thu, 2010-08-05 at 12:20 -0500, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > This patch removes the sequential SMS message identification gig and
> > replaces it with a 16-bit hash cookie based off message contents.
> > 
> > FIXME: [incomplete] need to figure out how to do so that identical
> > messages sent to different or the same numbers also have a different
> > ID.
> > ---
> >  src/ofono.h |    9 +++++----
> >  src/sms.c   |   55 +++++++++++++++++++++++--------------------------------
> >  src/stk.c   |    5 +++--
> >  3 files changed, 31 insertions(+), 38 deletions(-)
> > 
> > diff --git a/src/ofono.h b/src/ofono.h
> > index aaa01d9..b73797b 100644
> > --- a/src/ofono.h
> > +++ b/src/ofono.h
> > @@ -185,10 +185,11 @@ enum ofono_sms_submit_flag {
> >  
> >  typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
> >  
> > -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> > -					unsigned int flags,
> > -					ofono_sms_txq_submit_cb_t cb,
> > -					void *data, ofono_destroy_func destroy);
> > +struct tx_queue_entry *__ofono_sms_txq_submit(
> > +	struct ofono_sms *sms, GSList *list,
> > +	unsigned int flags, unsigned ref,
> > +	ofono_sms_txq_submit_cb_t cb,
> > +	void *data, ofono_destroy_func destroy);
> 
> As I said before, please don't mess with this function.

Once we settle on the ref / msg_id part, that one will sort out by
itself.

On later commits, the callback has to change in order to be able to
implement PropertChanged signals. Unless you have a better way to do it
that doesn't involve poking with 'struct tx_queue_entry' internals. A
helper to set the callback after the fact is another option, but it is
way dirtier.

Likewise with the return value (msg id vs the struct itself). You
suggested poking at the head of the queue upon return. That is peeking
on internals and you have adamantly (and rightfully) argued against it
in other cases.

> >  
> >  #include <ofono/sim.h>
> >  #include <ofono/stk.h>
> > diff --git a/src/sms.c b/src/sms.c
> > index 35364db..39d4a32 100644
> > --- a/src/sms.c
> > +++ b/src/sms.c
> > @@ -55,7 +55,6 @@ struct ofono_sms {
> >  	DBusMessage *pending;
> >  	struct ofono_phone_number sca;
> >  	struct sms_assembly *assembly;
> > -	unsigned int next_msg_id;
> >  	guint ref;
> >  	GQueue *txq;
> >  	gint tx_source;
> > @@ -589,7 +588,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  	int ref_offset;
> >  	struct ofono_modem *modem;
> >  	unsigned int flags;
> > -	unsigned int msg_id;
> > +	guint16 msg_id;
> > +	struct tx_queue_entry *sms_msg;
> >  
> >  	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
> >  					DBUS_TYPE_STRING, &text,
> > @@ -605,24 +605,19 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  	if (!msg_list)
> >  		return __ofono_error_invalid_format(msg);
> >  
> > -	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
> > -	DBG("ref: %d, offset: %d", sms->ref, ref_offset);
> > -
> > -	if (ref_offset != 0) {
> > -		if (sms->ref == 65536)
> > -			sms->ref = 1;
> > -		else
> > -			sms->ref = sms->ref + 1;
> > -	}
> > +	msg_id = sms_uuid_from_pdu_list(msg_list);
> 
> You're calculating the ID before setting the ref and to? That is pretty
> silly ;)

the ref was supposed to be generated off the msg ID according to your
initial design decisions, so this is following spec. We need to clarify
this (and this touches with the other email). 

> > +	set_ref_and_to(msg_list, msg_id, ref_offset, to);
> > +	DBG("SMS ID: 0x%04x", msg_id);
> >  
> >  	flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
> >  	flags |= OFONO_SMS_SUBMIT_FLAG_RETRY;
> >  	if (sms->use_delivery_reports)
> >  		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
> >  
> > -	msg_id = __ofono_sms_txq_submit(sms, msg_list, flags, send_message_cb,
> > -						dbus_message_ref(msg),
> > -						send_message_destroy);
> > +	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
> > +					 send_message_cb,
> > +					 dbus_message_ref(msg),
> > +					 send_message_destroy);
> 
> Make the ID calculation happen inside tx_queue_entry_new.  That function
> already converts all the SMS objects into a PDU.  So simply hashing the
> PDUs there would be very easy.

Ok, holding until we confirm what relationship there will be between ref
and msg_id.

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Denis Kenzior]

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

Hi Inaky,

On 08/05/2010 01:07 PM, Inaky Perez-Gonzalez wrote:
> On Thu, 2010-08-05 at 13:02 -0500, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>>>> The current message id is 32 bits.  Why are we stopping at 16?  We could
>>>> go all the way to 64 if needed...
>>>
>>> Well, at the end what gets sent to the network is 16 bits right?
>>
>> So I think we're all confused here.  This ID is not being sent to the
>> network.  It is only used for uniquely identifying the message over
>> D-Bus and to message history.  This number can be as small or as big as
>> we want.
> 
> I was under the impression that you wanted to use this for the ref also,
> and thus it is imprinted into the PDUs by set_ref_and_to(). Can you
> confirm this?
> 

No, you're totally confused ;)  Ref is only sent on messages that are
multi-fragment.  It is not used for single-fragment messages.  The logic
is currently implemented per spec and I never had the intention to do it
any other way.

> Ok, I see your concern. Proper seeding of the RNG should take care of
> that case, but either way works. If you want hashing + time() seeding
> that can be done easily.
> 
> I still could use clarification on which other fields of the 'struct
> sms' union you want to have hashed other than the PDU payloads.
> 

All of them.  Again, it might be much simpler to do this over the
fragments when they are in true PDU form.  This can be done in
sms_assembly and tx_queue_entry_new since they deal with fragments already.

Alternatively handling the individual structure fields can be done as well.

Regards,
-Denis

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Inaky Perez-Gonzalez]

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

On Thu, 2010-08-05 at 13:21 -0500, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 08/05/2010 01:07 PM, Inaky Perez-Gonzalez wrote:
> > On Thu, 2010-08-05 at 13:02 -0500, Denis Kenzior wrote: 
> >> Hi Inaky,
> >>
> >>>> The current message id is 32 bits.  Why are we stopping at 16?  We could
> >>>> go all the way to 64 if needed...
> >>>
> >>> Well, at the end what gets sent to the network is 16 bits right?
> >>
> >> So I think we're all confused here.  This ID is not being sent to the
> >> network.  It is only used for uniquely identifying the message over
> >> D-Bus and to message history.  This number can be as small or as big as
> >> we want.
> > 
> > I was under the impression that you wanted to use this for the ref also,
> > and thus it is imprinted into the PDUs by set_ref_and_to(). Can you
> > confirm this?
> > 
> 
> No, you're totally confused ;)  Ref is only sent on messages that are
> multi-fragment.  It is not used for single-fragment messages.  The logic
> is currently implemented per spec and I never had the intention to do it
> any other way.

Okay, so then this means that all the current ref infrastructure
(monotonically increased) is left in place and the message ID stuff is
used only internally for D-Bus and whichever other needs there are. I'll
up it to 64-bits.

> > Ok, I see your concern. Proper seeding of the RNG should take care of
> > that case, but either way works. If you want hashing + time() seeding
> > that can be done easily.
> > 
> > I still could use clarification on which other fields of the 'struct
> > sms' union you want to have hashed other than the PDU payloads.
> > 
> 
> All of them.  Again, it might be much simpler to do this over the
> fragments when they are in true PDU form.  This can be done in
> sms_assembly and tx_queue_entry_new since they deal with fragments already.
> 
> Alternatively handling the individual structure fields can be done as well.

I'll look into it.

Re: [SMS D-Bus 02/19] sms: introduce message ID API

[Inaky Perez-Gonzalez]

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

On Thu, 2010-08-05 at 13:21 -0500, Denis Kenzior wrote:

> > I still could use clarification on which other fields of the 'struct
> > sms' union you want to have hashed other than the PDU payloads.
> > 
> 
> All of them.  Again, it might be much simpler to do this over the
> fragments when they are in true PDU form.  This can be done in
> sms_assembly and tx_queue_entry_new since they deal with fragments already.
> 
> Alternatively handling the individual structure fields can be done as well.

On the RX path, it is not going to be easy to use the raw PDUs to
generate a UUID:

whoever code gets the PDU
  ofono_sms_deliver_notify(PDU)
    PDU decoded into a 'struct sms'
    handle_deliver(struct sms)
      added to fragment list in assembly
      sms_dispatch(fragment list of 'struct sms')

so if we also want to generate a UUID of this based on RAW PDUs, either
we keep the raw PDU around or we base the UUID generation from the
struct sms fields. We could really only generate this at sms_dispatch()
or similar, where we have the fragments in the proper order.

Thoughts?