2:02 a.m.
This patch set exports on MessageManager interface a method to cancel messages
that are pending on tx queue.
As the doc says, only messages that are currently waiting on message queue and
not being handled by the modem can be cancelled. Otherwise a race could occur.
First 4 patches are some refactors needed to accommodate this feature, that has
been added on 5th patch.
Lucas De Marchi (7):
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
TODO | 9 --
doc/messagemanager-api.txt | 10 +++
examples/history.c | 5 +
include/history.h | 1 +
src/message.c | 2 +
src/message.h | 3 +-
src/sms.c | 181 ++++++++++++++++++++++++++++++++++----------
7 files changed, 161 insertions(+), 50 deletions(-)
--
1.7.4
2:02 a.m.
New subject: [PATCH 1/7] history: add cancelled status
Based on patch from Yang Gu <gyagp0(a)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
2:02 a.m.
New subject: [PATCH 2/7] examples: handle cancelled history status
---
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
2:02 a.m.
New subject: [PATCH 3/7] message: add cancelled state
Based on patch from Yang Gu <gyagp0(a)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
2:02 a.m.
New subject: [PATCH 4/7] sms: factor out 'remove entry' from tx_finished()
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(a)gmail.com>
---
src/sms.c | 99 ++++++++++++++++++++++++++++++++++++------------------------
1 files changed, 59 insertions(+), 40 deletions(-)
diff --git a/src/sms.c b/src/sms.c
index 73e067e..7db91cb 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,65 @@ 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->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 +605,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 +644,13 @@ static void tx_finished(const struct ofono_error *error, int mr,
void *data)
return;
}
-next_q:
- entry = g_queue_pop_head(sms->txq);
+ sms->tx_state = MESSAGE_STATE_SENT;
+next_q:
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;
-
- 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);
+ sms_tx_queue_remove_entry(sms, g_queue_peek_head_link(sms->txq));
if (sms->registered == FALSE)
return;
--
1.7.4
2:02 a.m.
New subject: [PATCH 5/7] sms: allow message submission to be cancelled
Based on patch from Yang Gu <gyagp0(a)gmail.com>
---
src/sms.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/src/sms.c b/src/sms.c
index 7db91cb..91b0a31 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,85 @@ 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;
+
+ uuidstr = path + strlen(path) - 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_failed(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 +1058,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
2:58 a.m.
New subject: [PATCH 5/7] sms: allow message submission to be cancelled
Hi,
On 4 February 2011 16:02, Lucas De Marchi <lucas.demarchi(a)profusion.mobi> wrote:
Based on patch from Yang Gu <gyagp0(a)gmail.com>
---
src/sms.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/src/sms.c b/src/sms.c
index 7db91cb..91b0a31 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,85 @@ 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;
+
+ uuidstr = path + strlen(path) - OFONO_SHA1_UUID_LEN * 2;
+
+ if (decode_hex_own_buf(uuidstr, -1, NULL, 0, uuid->uuid) == NULL)
+ return FALSE;
I think you need to at least check that strlen(path) >=
OFONO_SHA1_UUID_LEN * 2 or we might segfault.
+
+ 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_failed(msg);
Maybe __ofono_error_not_found?
+
+ 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);
As you mentioned on IRC we may need to call the "submitted" callback
with ok == FALSE.
I also wonder if we need to check that (entry->flags &
OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) and update some property.
Best regards
3:04 a.m.
New subject: [PATCH 5/7] sms: allow message submission to be cancelled
On 4 February 2011 16:58, andrzej zaborowski <balrogg(a)gmail.com> wrote:
I also wonder if we need to check that (entry->flags &
OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) and update some property.
Nevermind, this is handled in 4/7.
Best regards
3:07 a.m.
New subject: [PATCH 5/7] sms: allow message submission to be cancelled
Hi Andrzej,
On Fri, Feb 4, 2011 at 1:58 PM, andrzej zaborowski <balrogg(a)gmail.com> wrote:
Hi,
On 4 February 2011 16:02, Lucas De Marchi <lucas.demarchi(a)profusion.mobi> wrote:
> Based on patch from Yang Gu <gyagp0(a)gmail.com>
> ---
> src/sms.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 82 insertions(+), 0 deletions(-)
>
> diff --git a/src/sms.c b/src/sms.c
> index 7db91cb..91b0a31 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,85 @@ 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;
> +
> + uuidstr = path + strlen(path) - OFONO_SHA1_UUID_LEN * 2;
> +
> + if (decode_hex_own_buf(uuidstr, -1, NULL, 0, uuid->uuid) == NULL)
> + return FALSE;
I think you need to at least check that strlen(path) >=
OFONO_SHA1_UUID_LEN * 2 or we might segfault.
Indeed. I'll fix this.
> +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_failed(msg);
Maybe __ofono_error_not_found?
ok.
> +
> + 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);
As you mentioned on IRC we may need to call the "submitted" callback
with ok == FALSE.
ok.
I also wonder if we need to check that (entry->flags &
OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) and update some property.
It's already handled inside sms_tx_queue_remove_entry().
thanks,
Lucas De Marchi
2:02 a.m.
New subject: [PATCH 6/7] doc: add CancelMessage to MessageManager
---
doc/messagemanager-api.txt | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/doc/messagemanager-api.txt b/doc/messagemanager-api.txt
index 0723e9c..3d66193 100644
--- a/doc/messagemanager-api.txt
+++ b/doc/messagemanager-api.txt
@@ -32,6 +32,16 @@ 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.Failed
+
object SendMessage(string to, string text)
Send the message in text to the number in to. If the
--
1.7.4
2:02 a.m.
New subject: [PATCH 7/7] TODO: mark task 'cancel pending SMS' as done
---
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(a)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
4244
days inactive
4244
days old
10 comments
2 participants
participants (2)
-
andrzej zaborowski -
Lucas De Marchi