[PATCH v2 0/8] sms: Cancel pending message

[PATCH v2 0/8] sms: Cancel pending message

[Lucas De Marchi]

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

In this version I fixed some issues pointed out by Andrzej and added another
patch, not so related, but that would conflict with these ones if applied to
current master branch. Difference from previous version:

Tested with phonesim, slightly changing its plugin in oFono in order to hold
messages for a certain period.

Lucas De Marchi (8):
  history: add cancelled status
  examples: handle cancelled history status
  message: add cancelled state
  sms: factor out 'remove entry' from tx_finished()
  sms: allow message submission to be cancelled
  doc: add CancelMessage to MessageManager
  TODO: mark task 'cancel pending SMS' as done
  sms: remove check for impossible NULL condition

 TODO                       |    9 --
 doc/messagemanager-api.txt |   12 +++
 examples/history.c         |    5 +
 include/history.h          |    1 +
 src/message.c              |    2 +
 src/message.h              |    3 +-
 src/sms.c                  |  196 +++++++++++++++++++++++++++++++++----------
 7 files changed, 172 insertions(+), 56 deletions(-)

-- 
1.7.4

[PATCH v2 1/8] history: add cancelled status

[Lucas De Marchi]

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

Based on patch from Yang Gu <gyagp0@gmail.com>
---
 include/history.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/history.h b/include/history.h
index c1c4aa1..756097e 100644
--- a/include/history.h
+++ b/include/history.h
@@ -35,6 +35,7 @@ enum ofono_history_sms_status {
 	OFONO_HISTORY_SMS_STATUS_PENDING,
 	OFONO_HISTORY_SMS_STATUS_SUBMITTED,
 	OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED,
+	OFONO_HISTORY_SMS_STATUS_SUBMIT_CANCELLED,
 	OFONO_HISTORY_SMS_STATUS_DELIVERED,
 	OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED,
 };
-- 
1.7.4

[PATCH v2 2/8] examples: handle cancelled history status

[Lucas De Marchi]

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

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

diff --git a/examples/history.c b/examples/history.c
index 0a1e8d7..21668ec 100644
--- a/examples/history.c
+++ b/examples/history.c
@@ -172,6 +172,11 @@ static void example_history_sms_send_status(
 		ofono_debug("Sending SMS %s failed", ofono_uuid_to_str(uuid));
 		ofono_debug("Failure Time: %s", buf);
 		break;
+	case OFONO_HISTORY_SMS_STATUS_SUBMIT_CANCELLED:
+		ofono_debug("Submission of SMS %s was canceled",
+					ofono_uuid_to_str(uuid));
+		ofono_debug("Cancel time: %s", buf);
+		break;
 	case OFONO_HISTORY_SMS_STATUS_DELIVERED:
 		ofono_debug("SMS delivered, msg_id: %s, time: %s",
 					ofono_uuid_to_str(uuid), buf);
-- 
1.7.4

[PATCH v2 3/8] message: add cancelled state

[Lucas De Marchi]

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

Based on patch from Yang Gu <gyagp0@gmail.com>
---
 src/message.c |    2 ++
 src/message.h |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/message.c b/src/message.c
index eb85adf..668a693 100644
--- a/src/message.c
+++ b/src/message.c
@@ -46,6 +46,8 @@ static const char *message_state_to_string(enum message_state s)
 		return "sent";
 	case MESSAGE_STATE_FAILED:
 		return "failed";
+	case MESSAGE_STATE_CANCELLED:
+		return "cancelled";
 	}
 
 	return NULL;
diff --git a/src/message.h b/src/message.h
index 14e66c3..ad30798 100644
--- a/src/message.h
+++ b/src/message.h
@@ -24,7 +24,8 @@
 enum message_state {
 	MESSAGE_STATE_PENDING,
 	MESSAGE_STATE_SENT,
-	MESSAGE_STATE_FAILED
+	MESSAGE_STATE_FAILED,
+	MESSAGE_STATE_CANCELLED,
 };
 
 struct ofono_atom;
-- 
1.7.4

[PATCH v2 4/8] sms: factor out 'remove entry' from tx_finished()

[Lucas De Marchi]

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

Refactor tx_finished() and create a function to remove an entry from the
tx queue. This function will be used also when a message is cancelled.
Thus, handle the case in which state is MESSAGE_STATE_CANCELLED as well.

Based on patch from Yang Gu <gyagp0@gmail.com>
---
 src/sms.c |  105 ++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 73e067e..dfec456 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -65,6 +65,7 @@ struct ofono_sms {
 	struct sms_assembly *assembly;
 	guint ref;
 	GQueue *txq;
+	enum message_state tx_state;
 	unsigned long tx_counter;
 	guint tx_source;
 	struct ofono_message_waiting *mw;
@@ -538,15 +539,68 @@ static void tx_queue_entry_destroy_foreach(gpointer _entry, gpointer unused)
 	tx_queue_entry_destroy(_entry);
 }
 
+static void sms_tx_queue_remove_entry(struct ofono_sms *sms, GList *entry_list)
+{
+	struct tx_queue_entry *entry = entry_list->data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
+
+	g_queue_delete_link(sms->txq, entry_list);
+
+	DBG("%p", entry);
+
+	if (entry->cb)
+		entry->cb(sms->tx_state == MESSAGE_STATE_SENT, entry->data);
+
+	if (entry->flags & OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY) {
+		enum ofono_history_sms_status hs;
+
+		switch(sms->tx_state) {
+		case MESSAGE_STATE_SENT:
+			hs = OFONO_HISTORY_SMS_STATUS_SUBMITTED;
+			break;
+		case MESSAGE_STATE_FAILED:
+			hs = OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED;
+			break;
+		case MESSAGE_STATE_CANCELLED:
+			hs = OFONO_HISTORY_SMS_STATUS_SUBMIT_CANCELLED;
+			break;
+		default:
+			ofono_error("Unexpected sms state %d", sms->tx_state);
+
+			return;
+		}
+
+		__ofono_history_sms_send_status(modem, &entry->uuid,
+								time(NULL), hs);
+	}
+
+	if (entry->flags & OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) {
+		struct message *m;
+
+		sms_tx_backup_free(sms->imsi, entry->id, entry->flags,
+					ofono_uuid_to_str(&entry->uuid));
+
+		m = g_hash_table_lookup(sms->messages, &entry->uuid);
+
+		if (m != NULL) {
+			message_set_state(m, sms->tx_state);
+			g_hash_table_remove(sms->messages, &entry->uuid);
+			message_emit_removed(m,
+					OFONO_MESSAGE_MANAGER_INTERFACE);
+			message_dbus_unregister(m);
+		}
+	}
+
+	tx_queue_entry_destroy(entry);
+}
+
 static void tx_finished(const struct ofono_error *error, int mr, void *data)
 {
 	struct ofono_sms *sms = data;
-	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	struct tx_queue_entry *entry = g_queue_peek_head(sms->txq);
 	gboolean ok = error->type == OFONO_ERROR_TYPE_NO_ERROR;
-	struct message *m = NULL;
 
-	DBG("tx_finished");
+	DBG("tx_finished %p", entry);
 
 	if (ok == FALSE) {
 		/* Retry again when back in online mode */
@@ -554,6 +608,8 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 		if (sms->registered == FALSE)
 			return;
 
+		sms->tx_state = MESSAGE_STATE_FAILED;
+
 		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY))
 			goto next_q;
 
@@ -591,47 +647,10 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 		return;
 	}
 
-next_q:
-	entry = g_queue_pop_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;
-
-		if (ok)
-			hs = OFONO_HISTORY_SMS_STATUS_SUBMITTED;
-		else
-			hs = OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED;
-
-		__ofono_history_sms_send_status(modem, &entry->uuid,
-						time(NULL), hs);
-	}
-
-	if (entry->flags & OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) {
-		enum message_state ms;
-
-		sms_tx_backup_free(sms->imsi, entry->id, entry->flags,
-					ofono_uuid_to_str(&entry->uuid));
-
-		if (ok)
-			ms = MESSAGE_STATE_SENT;
-		else
-			ms = MESSAGE_STATE_FAILED;
+	sms->tx_state = MESSAGE_STATE_SENT;
 
-		m = g_hash_table_lookup(sms->messages, &entry->uuid);
-
-		if (m != NULL) {
-			message_set_state(m, ms);
-			g_hash_table_remove(sms->messages, &entry->uuid);
-			message_emit_removed(m,
-					OFONO_MESSAGE_MANAGER_INTERFACE);
-			message_dbus_unregister(m);
-		}
-	}
-
-	tx_queue_entry_destroy(entry);
+next_q:
+	sms_tx_queue_remove_entry(sms, g_queue_peek_head_link(sms->txq));
 
 	if (sms->registered == FALSE)
 		return;
-- 
1.7.4

[PATCH v2 5/8] sms: allow message submission to be cancelled

[Lucas De Marchi]

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

Based on patch from Yang Gu <gyagp0@gmail.com>
---
 src/sms.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index dfec456..212a470 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -685,6 +685,8 @@ static gboolean tx_next(gpointer user_data)
 			|| (entry->num_pdus - entry->cur_pdu) > 1)
 		send_mms = 1;
 
+	sms->tx_state = MESSAGE_STATE_PENDING;
+
 	sms->driver->submit(sms, pdu->pdu, pdu->pdu_len, pdu->tpdu_len,
 				send_mms, tx_finished, sms);
 
@@ -970,6 +972,91 @@ static DBusMessage *sms_get_messages(DBusConnection *conn, DBusMessage *msg,
 	return reply;
 }
 
+static gboolean uuid_from_message_path(const char *path,
+						struct ofono_uuid *uuid)
+{
+	const char *uuidstr;
+	size_t len;
+
+	len = strlen(path);
+
+	if (len < OFONO_SHA1_UUID_LEN * 2)
+		return FALSE;
+
+	uuidstr = path + len - OFONO_SHA1_UUID_LEN * 2;
+
+	if (decode_hex_own_buf(uuidstr, -1, NULL, 0, uuid->uuid) == NULL)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gint entry_compare_by_uuid(gconstpointer a, gconstpointer b)
+{
+	const struct tx_queue_entry *entry = a;
+	const char *uuid = b;
+
+	return memcmp(&entry->uuid, uuid, sizeof(entry->uuid));
+}
+
+static DBusMessage *sms_cancel_message(DBusConnection *conn, DBusMessage *msg,
+								void *data)
+{
+	struct ofono_sms *sms = data;
+	char *path;
+	struct ofono_uuid uuid;
+	GList *l;
+	struct tx_queue_entry *entry;
+
+	if (sms->pending)
+		return __ofono_error_busy(msg);
+
+	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
+					DBUS_TYPE_INVALID) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	if (path[0] == '\0')
+		return __ofono_error_invalid_args(msg);
+
+	if (uuid_from_message_path(path, &uuid) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	l = g_queue_find_custom(sms->txq, uuid.uuid, entry_compare_by_uuid);
+
+	if (l == NULL)
+		return __ofono_error_not_found(msg);
+
+	entry = l->data;
+
+	if (entry == g_queue_peek_head(sms->txq)) {
+		/*
+		 * Fail if any pdu was already transmitted or if we are
+		 * waiting the answer from driver.
+		 */
+		if (entry->cur_pdu > 0 ||
+					sms->tx_state == MESSAGE_STATE_PENDING)
+			return __ofono_error_failed(msg);
+
+		/*
+		 * Make sure we don't call tx_next() if there are no entries
+		 * and that next entry doesn't have to wait a 'retry time'
+		 * from this one.
+		 */
+		if (sms->tx_source) {
+			g_source_remove(sms->tx_source);
+			sms->tx_source = 0;
+
+			if (g_queue_get_length(sms->txq) > 1)
+				sms->tx_source = g_timeout_add(0, tx_next, sms);
+		}
+	}
+
+	sms->tx_state = MESSAGE_STATE_CANCELLED;
+	sms_tx_queue_remove_entry(sms, l);
+
+	return dbus_message_new_method_return(msg);
+}
+
 static GDBusMethodTable sms_manager_methods[] = {
 	{ "GetProperties",    "",    "a{sv}",        sms_get_properties,
 						G_DBUS_METHOD_FLAG_ASYNC },
@@ -977,6 +1064,7 @@ static GDBusMethodTable sms_manager_methods[] = {
 						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "SendMessage",      "ss",  "o",             sms_send_message,
 						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "CancelMessage",    "o",   "",              sms_cancel_message },
 	{ "GetMessages",       "",    "a(oa{sv})",    sms_get_messages },
 	{ }
 };
-- 
1.7.4

[PATCH v2 6/8] doc: add CancelMessage to MessageManager

[Lucas De Marchi]

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

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

diff --git a/doc/messagemanager-api.txt b/doc/messagemanager-api.txt
index 0723e9c..29453c3 100644
--- a/doc/messagemanager-api.txt
+++ b/doc/messagemanager-api.txt
@@ -32,6 +32,18 @@ Methods		dict GetProperties()
 			Possible Errors: [service].Error.InvalidArguments
 					 [service].Error.DoesNotExist
 
+		void CancelMessage(object path)
+
+			Cancel a message that was previously sent. Only
+			messages that are waiting on queue can be cancelled and
+			it's not possible to cancel messages that already had
+			some parts delivered.
+
+			Possible Errors: [service].Error.InvalidArguments
+					 [service].Error.InProgress
+					 [service].Error.NotFound
+					 [service].Error.Failed
+
 		object SendMessage(string to, string text)
 
 			Send the message in text to the number in to.  If the
-- 
1.7.4

[PATCH v2 7/8] TODO: mark task 'cancel pending SMS' as done

[Lucas De Marchi]

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

---
 TODO |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/TODO b/TODO
index 13a6baa..8790209 100644
--- a/TODO
+++ b/TODO
@@ -31,15 +31,6 @@ SMS
   Priority: Low
   Complexity: C8
 
-- See / Cancel pending SMS messages over DBus.  When oFono sends SMS messages
-  the method call is only returned when the message has been submitted to the
-  network.  Instead we should return an object path and allow cancellation of
-  pending messages.
-
-  Priority: High
-  Complexity: C2
-  Owner: Yang Gu <yang.gu@intel.com>
-
 - Asynchronously acknowledge SMS DELIVER messages sent by the SMS driver
   to core using ofono_sms_deliver_notify().  This may require the struct
   ofono_sms_driver to be extended with one more function pointer like:
-- 
1.7.4

[PATCH v2 8/8] sms: remove check for impossible NULL condition

[Lucas De Marchi]

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

tx_next() must never be called with entry == NULL and currently it
was already being dereferenced before making this check. Thus just
remove it.
---
 src/sms.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 212a470..be7132a5 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -675,9 +675,6 @@ static gboolean tx_next(gpointer user_data)
 
 	sms->tx_source = 0;
 
-	if (entry == NULL)
-		return FALSE;
-
 	if (sms->registered == FALSE)
 		return FALSE;
 
-- 
1.7.4

Re: [PATCH v2 0/8] sms: Cancel pending message

[Denis Kenzior]

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

Hi Lucas,

On 02/04/2011 01:40 PM, Lucas De Marchi wrote:
> In this version I fixed some issues pointed out by Andrzej and added another
> patch, not so related, but that would conflict with these ones if applied to
> current master branch. Difference from previous version:
> 
> Tested with phonesim, slightly changing its plugin in oFono in order to hold
> messages for a certain period.

I applied this series and slightly changed the API in the process.
Thanks for doing this.

Can you have a quick look to make sure I didn't screw anything up?

Regards,
-Denis