[patch 00/20] SMS D-Bus support and misc small patches

[patch 00/20] SMS D-Bus support and misc small patches

[Inaky Perez-Gonzalez]

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

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

This (3rd? 4th?) version of the patchset builds the D-Bus support on
top of the new _txq_submit() callback mechanism.

Note there are still a couple of opens that need discussion:

 - the message ID is generated based on the contents of the message --
   thus, the current way doesn't work. We need the caller to
   _txq_submit() to generate it. It's been left out of the STK
   stc.c:handle_command_send_sms() because I am not sure what is the
   right way to do it -- need feedback on that.

 - The generation of the SMS message ID based on contents still has
   shortcomings: if we submit two messages with the same content and
   destination number, the ID is the same [sms.c:sms_msg_send()]. What
   other factor would make sense to add? time?

The following changes since commit 94344e967b4cd3edd65aa5254ef4b4f5dd037e69:
  Denis Kenzior (1):
        TODO: Major updates to STK related tasks

are available in the git repository at:

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

Patches follow for reviewing convenience.

Inaky Perez-Gonzalez (20):
      bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
      write_file: make transaction-safe
      manpage: explain debugging options to -d
      SMS: introduce message ID API
      introduce DECLARE_SMS_ADDR_STR()
      _assembly_encode_address: export and rename
      SMS: implement SHA256-based message IDs [incomplete]
      sms: document the org.ofono.SMSMessage D-Bus interface
      SMS: document handle_sms_status_report()
      sms_text_prepare: document @use_delivery_reports
      SMS: rename create_tx_queue_entry() to tx_queue_entry_new()
      struct tx_queue_entry: add a destructor
      SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
      SMS: introduce bare state machine and transitions
      SMS: introduce Wait-for-Status-Report state and infrastructure
      SMS: introduce a state change callback for TX messages
      SMS: export outgoing messages over D-Bus
      SMS: send D-Bus SMS-MSG::PropertyChanged signals when message changes status
      SMS: introduce sms_msg_cancel and its D-Bus wrapper
      SMS: Implement D-Bus SMS-MSG::GetProperties

 HACKING                        |   10 +
 Makefile.am                    |    5 +-
 doc/ofonod.8                   |    5 +-
 doc/sms-api.txt                |   49 ++++-
 src/bug.h                      |   50 ++++
 src/ofono.h                    |   42 +++-
 src/sms.c                      |  598 ++++++++++++++++++++++++++++++++++------
 src/smsutil.c                  |  206 ++++++++++++++-
 src/smsutil.h                  |  122 ++++++++
 src/stk.c                      |   24 ++-
 src/storage.c                  |   42 ++-
 test/test-sms-msg-cancel       |  173 ++++++++++++
 test/test-sms-msg-state-change |   24 ++
 unit/test-sms-msg-id.c         |  212 ++++++++++++++
 14 files changed, 1449 insertions(+), 113 deletions(-)
 create mode 100644 src/bug.h
 create mode 100755 test/test-sms-msg-cancel
 create mode 100755 test/test-sms-msg-state-change
 create mode 100644 unit/test-sms-msg-id.c

[patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

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

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

These have been stolen from the Linux kernel source; come pretty handy
to make build-time consistency checks and thus avoid run-time
surprises.
---
 src/bug.h     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.c |    1 +
 2 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 src/bug.h

diff --git a/src/bug.h b/src/bug.h
new file mode 100644
index 0000000..07c14b9
--- /dev/null
+++ b/src/bug.h
@@ -0,0 +1,50 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+/*
+ * Build time consistency checks
+ *
+ * Copied from the Linux kernel, linux/include/linux/kernel.h
+ * v2.6.35-rc; this file has no explicit license header, thus is
+ * covered by linux/COPYING which licenses it as GPL v2 only.
+ */
+
+/* Force a compilation error if condition is true */
+#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
+
+/* Force a compilation error if condition is constant and true */
+#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
+
+/* Force a compilation error if a constant expression is not a power of 2 */
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
+	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
+
+/* Force a compilation error if condition is true, but also produce a
+   result (of value 0 and type size_t), so the expression can be used
+   e.g. in a structure initializer (or where-ever else comma expressions
+   aren't permitted). */
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
+#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+
+/*
+ * End of code copied from linux/include/linux/kernel.h
+ * v2.6.35-rc.
+ */
diff --git a/src/smsutil.c b/src/smsutil.c
index e41c041..6c2087a 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -37,6 +37,7 @@
 #include "util.h"
 #include "storage.h"
 #include "smsutil.h"
+#include "bug.h"
 
 #define uninitialized_var(x) x = x
 
-- 
1.6.6.1

[patch 02/20] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2590 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 |   42 +++++++++++++++++++++++++++++++-----------
 1 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/storage.c b/src/storage.c
index cac5835..c88a8c8 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,36 @@ 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

[patch 03/20] manpage: explain debugging options to -d

[Inaky Perez-Gonzalez]

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

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

Modified HACKING and man page to have more formation on what are the
debugging options and how to enable them.
---
 HACKING      |   10 ++++++++++
 doc/ofonod.8 |    5 ++++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/HACKING b/HACKING
index ae420aa..e825185 100644
--- a/HACKING
+++ b/HACKING
@@ -81,3 +81,13 @@ automatically includes this option.
 
 For production installations or distribution packaging it is important that
 the "--enable-maintainer-mode" option is NOT used.
+
+Note multiple arguments to -d can be specified, colon, comma or space
+separated. The arguments are relative source code filenames for which
+debugging output should be enabled; output shell-style globs are
+accepted (e.g.: 'plugins/*:src/main.c').
+
+Other debugging settings that can be toggled:
+
+ - Environment variable OFONO_AT_DEBUG (set to 1): enable AT commands
+   debugging
diff --git a/doc/ofonod.8 b/doc/ofonod.8
index 474d7fb..7bb908c 100644
--- a/doc/ofonod.8
+++ b/doc/ofonod.8
@@ -18,7 +18,10 @@ is used to manage \fID-Bus\fP permissions for oFono.
 .SH OPTIONS
 .TP
 .B --debug, -d
-Enable debug information output.
+Enable debug information output. Note multiple arguments to -d can be
+specified, colon, comma or space separated. The arguments are relative
+source code filenames for which debugging output should be enabled;
+output shell-style globs are accepted (e.g.: "plugins/*:src/main.c").
 .TP
 .B --nodetach, -n
 Don't run as daemon in background.
-- 
1.6.6.1

[patch 04/20] SMS: introduce message ID API

[Inaky Perez-Gonzalez]

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

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

This adds a simple API to use for generating unique IDs for SMS
messages.

Note this adds missing #includes to src/smsutil.h; header files should
included from any other .c or .h file.
---
 Makefile.am            |    5 +-
 src/smsutil.c          |  191 +++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h          |   87 ++++++++++++++++++++
 unit/test-sms-msg-id.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 494 insertions(+), 1 deletions(-)
 create mode 100644 unit/test-sms-msg-id.c

diff --git a/Makefile.am b/Makefile.am
index e256841..5a371c2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -361,7 +361,7 @@ unit_objects =
 noinst_PROGRAMS = unit/test-common unit/test-util unit/test-idmap \
 					unit/test-sms unit/test-simutil \
 					unit/test-mux unit/test-caif \
-					unit/test-stkutil
+					unit/test-stkutil unit/test-sms-msg-id
 
 unit_test_common_SOURCES = unit/test-common.c src/common.c
 unit_test_common_LDADD = @GLIB_LIBS@
@@ -400,6 +400,9 @@ unit_test_caif_SOURCES = unit/test-caif.c $(gatchat_sources) \
 unit_test_caif_LDADD = @GLIB_LIBS@
 unit_objects += $(unit_test_caif_OBJECTS)
 
+unit_test_sms_msg_id_SOURCES = unit/test-sms-msg-id.c src/util.c src/smsutil.c src/storage.c
+unit_test_sms_msg_id_LDADD = @GLIB_LIBS@
+
 noinst_PROGRAMS += gatchat/gsmdial gatchat/test-server gatchat/test-qcdm
 
 gatchat_gsmdial_SOURCES = gatchat/gsmdial.c $(gatchat_sources)
diff --git a/src/smsutil.c b/src/smsutil.c
index 6c2087a..f93ea5b 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -3982,3 +3982,194 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)
 
 	return utf8;
 }
+
+static
+void sms_log_critical(const gchar *format, ...)
+{
+	va_list args;
+
+	va_start(args, format);
+	g_logv("SMS-MSG-ID", G_LOG_LEVEL_CRITICAL | G_LOG_FLAG_FATAL,
+	       format, args);
+	va_end(args);
+}
+
+const GChecksumType SMS_MSG_ID_CHECKSUM = G_CHECKSUM_SHA256;
+
+
+/*
+ * Internal function (also used for aiding in unit testing, hence not static)
+ */
+gsize __sms_msg_id_hash_length(void)
+{
+	return g_checksum_type_get_length(SMS_MSG_ID_CHECKSUM);
+}
+
+
+/**
+ * Initialize and reset a UUID's state
+ *
+ * \param msg_id Descriptor
+ */
+void sms_msg_id_init(struct sms_msg_id *msg_id)
+{
+	memset(msg_id->id, 0, sizeof(msg_id->id));
+	memset(&msg_id->checksum, 0, sizeof(msg_id->checksum));
+}
+
+
+/**
+ * Create a random UUID
+ *
+ * \param msg_id Descriptor
+ *
+ * \internal
+ *
+ * Crams the msg_id ID buffer with random 32-bit numbers; because we
+ * use a byte based buffer, we play the cast to guint32 trick (as that
+ * is what g_random_int() returns).
+ */
+void sms_msg_id_random(struct sms_msg_id *msg_id)
+{
+	guint32 *b;
+	unsigned cnt, top;
+
+	/* Fail building if the size of the ID block is not a full
+	 * multiple of a guint32's size. */
+	BUILD_BUG_ON(sizeof(msg_id->id) % sizeof(b[0]) != 0);
+
+	top = sizeof(msg_id->id) / sizeof(b[0]);
+	b = (void *) msg_id->id;
+	for (cnt = 0; cnt < top; cnt++)
+		b[cnt] = g_random_int();
+	/* mark hash is initialized */
+	msg_id->checksum = (void *) ~0;
+}
+
+
+/**
+ * Feed data to the hash for generation of a UUID
+ *
+ * Provides a data buffer to be added to the hash from which a UUID
+ * will be generated. When done providing buffers, call with a %NULL
+ * buffer to get the hash finalized and the UUID.
+ *
+ * \param msg_id Descriptor
+ * \param buf Pointer to data buffer. Call with %NULL to finalize the
+ *     hash and generate the UUID. Note that after calling with %NULL
+ *     you cannot call again with another buffer to generate another
+ *     UUID without calling sms_msg_id_init() first.
+ * \param buf_size size of the @buf buffer.
+ *
+ * NOTE:
+ *  - Will abort on OOM with an error message
+ *  - Will abort if called with %NULL as an argument to \param buf
+ *    without any non-NULL buffers were fed to in previous calls.
+ *  - In case of going over an error path before closing the UUID with
+ *    sms_msg_id_hash(%NULL), the hash has to be closed in order to
+ *    avoid memory leaks -- thus call sms_msg_id_hash(%NULL) in the
+ *    error path.
+ */
+void sms_msg_id_hash(struct sms_msg_id *msg_id,
+		     const void *buf, size_t buf_size)
+{
+	gsize digest_size = __sms_msg_id_hash_length();
+	unsigned char digest[digest_size];
+
+	if (msg_id->checksum == (void *) ~0) {
+		sms_log_critical("%s:%d: SW BUG: %s(!NULL) called "
+				 "on a closed hash\n",
+				 __FILE__, __LINE__, __func__);
+		return;
+	}
+	if (buf == NULL) {
+		if (msg_id->checksum == NULL) {
+			sms_log_critical("%s:%d: BUG: %s(NULL) called "
+					 "without feeding data first\n",
+					 __FILE__, __LINE__, __func__);
+			return;
+		}
+		g_checksum_get_digest(msg_id->checksum,
+				      digest, &digest_size);
+		memcpy(msg_id->id, digest,
+		       MIN(digest_size, sizeof(msg_id->id)));
+		if (digest_size < sizeof(msg_id->id))
+			memset(msg_id->id + digest_size, 0,
+			       sizeof(msg_id->id) - digest_size);
+		g_checksum_free(msg_id->checksum);
+		/* mark hash is initialized */
+		msg_id->checksum = (void *) ~0;
+	} else {
+		/* Word has it g_checksum_new(), which uses
+		 * g_slice_alloc() will abort() on allocation
+		 * failure. However, is not really documented
+		 * anywhere, so the extra check -- and we just abort
+		 * on OOM. */
+		if (msg_id->checksum == NULL) {
+			msg_id->checksum = g_checksum_new(SMS_MSG_ID_CHECKSUM);
+			if (msg_id->checksum == NULL)
+				sms_log_critical("%s:%d: OOM: can't allocate "
+						 "checksum",
+						 __FILE__, __LINE__);
+		}
+		g_checksum_update(msg_id->checksum, buf, buf_size);
+	}
+}
+
+
+/**
+ * Print a UUID to a string buffer
+ *
+ * Given a closed UUID, print it as a hexadecimal string to a provided
+ * buffer display (lowest nibble right).
+ *
+ * \param msg_id Descriptor
+ * \param strbuf String where to format the UUID
+ * \param strbuf_size Size of the string buffer.
+ *
+ * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or
+ *      sms_msg_id_hash(NULL) were called on them.
+ */
+void sms_msg_id_printf(const struct sms_msg_id *msg_id,
+		       char *strbuf, size_t strbuf_size)
+{
+	int cnt;
+	size_t written = 0;
+
+	if (msg_id->checksum != (void *) ~0) {
+		sms_log_critical("%s:%d: BUG: %s() called on an open hash\n",
+				 __FILE__, __LINE__, __func__);
+		return;
+	}
+	for (cnt = 0; cnt < SMS_MSG_ID_HASH_SIZE; cnt++)
+		written += snprintf(strbuf + written, strbuf_size - written,
+				    "%02x", msg_id->id[cnt] & 0xff);
+}
+
+
+/**
+ * Print a UUID to a string buffer
+ *
+ * Given a closed UUID, print it as a hexadecimal string to a provided
+ * buffer display (lowest nibble right).
+ *
+ * \param msg_id Descriptor
+ * \param strbuf String where to format the UUID
+ * \param strbuf_size Size of the string buffer.
+ *
+ * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or
+ *      sms_msg_id_hash(NULL) were called on them.
+ */
+void sms_msg_id_memcpy(void *buf, size_t buf_size,
+		       const struct sms_msg_id *msg_id)
+{
+	size_t copy_size = MIN(buf_size, sizeof(msg_id->id));
+	if (msg_id->checksum != (void *) ~0) {
+		sms_log_critical("%s:%d: BUG: %s() called on an open hash\n",
+				 __FILE__, __LINE__, __func__);
+		return;
+	}
+	memmove(buf, msg_id->id, MIN(buf_size, sizeof(msg_id->id)));
+	if (copy_size < buf_size)
+		memset(buf + copy_size, 0, buf_size - copy_size);
+}
diff --git a/src/smsutil.h b/src/smsutil.h
index 66ef6f8..fbe3c72 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -19,6 +19,12 @@
  *
  */
 
+#include <time.h>
+#include <glib/gtypes.h>
+#include <glib/gslist.h>
+#include <glib/ghash.h>
+#include <glib/gchecksum.h>
+
 #define CBS_MAX_GSM_CHARS 93
 
 enum sms_type {
@@ -539,3 +545,84 @@ 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);
+
+enum {
+	/* Note this has to be a multiple of sizeof(guint32);
+	 * Compilation will assert-fail if this is not met. */
+	SMS_MSG_ID_HASH_SIZE = 16,
+};
+
+/**
+ * Unique identifiers
+ *
+ * Generate unique identifiers to map data. Can be initialized with a
+ * random identifier or by creating a hash based on data contents.
+ *
+ * \NOTE Never access the struct's contents directly; use the
+ *     sms_msg_id*() API
+ *
+ * Usage
+ *
+ * Declare and initialize:
+ *
+ * \code
+ *   struct sms_msg_id msg_id;
+ *
+ *   sms_msg_id_init(&msg_id);	// initialize to known state
+ * \endcode
+ *
+ * Initialize with a random ID
+ *
+ * \code
+ *   sms_msg_id_random(&msg_id);	// Create a random unique ID
+ * \endcode
+ *
+ * or initialize by hashing data buffers:
+ *
+ * \code
+ *   sms_msg_id_hash(&msg_id, buf1, buf1_size);
+ *   sms_msg_id_hash(&msg_id, buf2, buf2_size);
+ *   sms_msg_id_hash(&msg_id, buf3, buf3_size);
+ *   ...
+ *   sms_msg_id_hash(&msg_id, NULL, 0);
+ * \endcode
+ *
+ * Once the hash has been initialized, it can be accessed. Never
+ * access it before initializing the hash (it will trigger a software
+ * error and abort).
+ *
+ * Formating for printing:
+ *
+ * \code
+ *   DECLARE_SMS_MSG_ID_STRBUF(buf);
+ *   ...
+ *   sms_msg_id_printf(&msg_id, buf, sizeof(buf));
+ * \endcode
+ *
+ * Copying the hash data to a buffer
+ *
+ * \code
+ *   sms_msg_id_memcpy(buf, buf_size, &msg_id);
+ * \endcode
+ *
+ * For generating another ID (with sms_msg_id_random() or
+ * sms_msg_id_hash()), sms_msg_id_init() has to be called.
+ */
+
+struct sms_msg_id {
+	guint8 id[SMS_MSG_ID_HASH_SIZE];
+	GChecksum *checksum;
+};
+
+#define DECLARE_SMS_MSG_ID_STRBUF(strbuf)	\
+	char strbuf[SMS_MSG_ID_HASH_SIZE * 2 + 1]
+
+void sms_msg_id_init(struct sms_msg_id *);
+void sms_msg_id_random(struct sms_msg_id *);
+void sms_msg_id_hash(struct sms_msg_id *, const void *, size_t);
+
+void sms_msg_id_printf(const struct sms_msg_id *, char *, size_t);
+void sms_msg_id_memcpy(void *, size_t, const struct sms_msg_id *);
+
+/* for unit testing only */
+gsize __sms_msg_id_hash_length(void);
diff --git a/unit/test-sms-msg-id.c b/unit/test-sms-msg-id.c
new file mode 100644
index 0000000..f9dd5e2
--- /dev/null
+++ b/unit/test-sms-msg-id.c
@@ -0,0 +1,212 @@
+/*
+ * Exercise flows and corner cases of the SMS-MSG-ID API
+ *
+ * This unit attempts to prove the common code flow and corner cases
+ * of the SMS-MSG-ID API, as described by it's functions usage
+ * documentation.
+ *
+ * Relies on the fact that the SMS-MSG-ID code will call
+ * sms_log_critical() for every condition that warrants an abort. This
+ * will call g_logv() with a G_LOG_FLAG_FATAL flag, which can be
+ * intercepted at glib level by setting a fatal log handler
+ * [test_fatal_log()] with g_test_log_set_fatal_handler() and decide
+ * based on the return value if the program should abort (TRUE) or not
+ * (FALSE). Instead we set a flag to indicate the 'condition' has been
+ * detected.
+ */
+
+#include <smsutil.h>
+#include <stdio.h>
+#include <string.h>
+#include <glib/gtestutils.h>
+
+int fatal_error;
+
+gboolean test_fatal_log(const gchar *log_domain,
+			GLogLevelFlags log_level,
+			const gchar *message,
+			gpointer user_data)
+{
+	fatal_error = 1;
+	return FALSE;	/* Avoid program abort */
+}
+
+int main(void)
+{
+	int fail = 0;
+	struct sms_msg_id msg_id1, msg_id2;
+	size_t hash_length = __sms_msg_id_hash_length();
+	size_t id_length = SMS_MSG_ID_HASH_SIZE;
+	size_t cnt;
+	DECLARE_SMS_MSG_ID_STRBUF(str_id1);
+	DECLARE_SMS_MSG_ID_STRBUF(str_id2);
+	char blob1[SMS_MSG_ID_HASH_SIZE],
+		blob2[SMS_MSG_ID_HASH_SIZE],
+		blob_big[4 * hash_length];
+
+	printf("I: '**CRITICAL**' and 'aborting...' messages "
+	       "should be ignored\n");
+
+	/* mask fatal errors so they don't crash, but just notes it
+	 * happened @fatal_error -- this is called by
+	 * sms_log_critical() -> g_logv() [as it uses the
+	 * G_LOG_FLAG_FATAL indicator. */
+	fatal_error = 0;
+	g_test_log_set_fatal_handler(test_fatal_log, NULL);
+
+	sms_msg_id_init(&msg_id1);
+	sms_msg_id_init(&msg_id2);
+
+	/* should trap: hash not initialized */
+	sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1));
+	if (fatal_error == 1) {
+		printf("I: %d: sms_msg_id_printf() doesn't work on "
+		       "unitialized hash. OK\n", __LINE__);
+		fatal_error = 0;
+	} else {
+		fprintf(stderr, "E: %d: sms_msg_id_printf() didn't reject an "
+		       "unitialized hash. FAILURE\n", __LINE__);
+		fail = 1;
+	}
+	sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1);
+	if (fatal_error == 1) {
+		printf("I: %d: sms_msg_id_memcpy() doesn't work on "
+		       "unitialized hash. OK\n", __LINE__);
+		fatal_error = 0;
+	} else {
+		fprintf(stderr, "E: %d: sms_msg_id_memcpy() didn't reject an "
+		       "unitialized hash. FAILURE.\n", __LINE__);
+		fail = 1;
+	}
+
+	/* Make a random initialization */
+	sms_msg_id_random(&msg_id1);
+	sms_msg_id_random(&msg_id2);
+
+	/* Printf */
+	sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1));
+	sms_msg_id_printf(&msg_id2, str_id2, sizeof(str_id2));
+	/* should be different */
+	if (strcmp(str_id1, str_id2))
+		printf("I: %d: strings for 1 & 2 are different, OK\n",
+			__LINE__);
+	else {
+		fprintf(stderr, "E: %d: strings for 1 & 2 match. FAILURE.\n",
+			__LINE__);
+		fail = 1;
+	}
+	printf("I: %d: str1 %s, str2 %s\n", __LINE__, str_id1, str_id2);
+
+	/* Reinitialize to test hash -- we feed the strings as binary
+	 * data */
+	sms_msg_id_init(&msg_id1);
+	/* Test sms_msg_id_hash(NULL) fails if no data was fed */
+	sms_msg_id_hash(&msg_id1, NULL, 0);
+	if (fatal_error == 1) {
+		printf("I: %d: sms_msg_id_hash() rejects a non-fed "
+		       "hash. OK\n", __LINE__);
+		fatal_error = 0;
+	} else {
+		fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject a "
+		       "non-fed hash. FAILURE.\n", __LINE__);
+		fail = 1;
+	}
+	sms_msg_id_hash(&msg_id1, str_id1, sizeof(str_id1));
+	sms_msg_id_hash(&msg_id1, str_id2, sizeof(str_id2));
+	sms_msg_id_hash(&msg_id1, NULL, 0);
+	/* Should fail if we try to add more */
+	sms_msg_id_hash(&msg_id1, str_id1, sizeof(str_id1));
+	if (fatal_error == 1) {
+		printf("I: %d: sms_msg_id_hash() rejects a closed "
+		       "hash. OK\n", __LINE__);
+		fatal_error = 0;
+	} else {
+		fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject a "
+		       "closed hash. FAILURE.\n", __LINE__);
+		fail = 1;
+	}
+	/* ... or close */
+	sms_msg_id_hash(&msg_id1, NULL, 0);
+	if (fatal_error == 1) {
+		printf("I: %d: sms_msg_id_hash() rejects closing a closed "
+		       "hash. OK\n", __LINE__);
+		fatal_error = 0;
+	} else {
+		fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject "
+			" closing a closed hash. FAILURE.\n", __LINE__);
+		fail = 1;
+	}
+
+	sms_msg_id_init(&msg_id2);
+	sms_msg_id_hash(&msg_id2, str_id1, sizeof(str_id1));
+	sms_msg_id_hash(&msg_id2, str_id2, sizeof(str_id2));
+	sms_msg_id_hash(&msg_id2, NULL, 0);
+
+	/* extract the blobs, they should match as we generated the
+	 * hashes from identical buckets */
+	sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1);
+	sms_msg_id_memcpy(blob2, sizeof(blob2), &msg_id2);
+	if (memcmp(blob1, blob2, sizeof(blob1))) {
+		fprintf(stderr, "E: %d: hashes don't match on identical feed. "
+			"FAILURE.\n", __LINE__);
+		fail = 1;
+	} else
+		printf("I: %d: hashes match on identical feed. "
+			"OK.\n", __LINE__);
+	sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1));
+	sms_msg_id_printf(&msg_id2, str_id2, sizeof(str_id2));
+	/* should be the same */
+	if (!strcmp(str_id1, str_id2))
+		printf("I: %d: strings for 1 & 2 match. OK\n",
+			__LINE__);
+	else {
+		fprintf(stderr, "E: %d: strings for 1 & 2 differ. FAILURE.\n",
+			__LINE__);
+		fail = 1;
+	}
+	printf("I: %d: str1 %s, str2 %s\n", __LINE__, str_id1, str_id2);
+
+	/* if the blob is bigger than the hash, the rest should be
+	 * filled with zeroes */
+	memset(blob_big, 0xff, sizeof(blob_big));
+	sms_msg_id_memcpy(blob_big, sizeof(blob_big), &msg_id1);
+	for (cnt = hash_length; cnt < sizeof(blob_big); cnt++)
+		if (blob_big[cnt] != 0) {
+			fprintf(stderr, "E: %d: blob @ #%u not zero. "
+				"FAILURE.\n", __LINE__, cnt);
+			fail = 1;
+			break;
+		}
+	if (cnt >= sizeof(blob_big))
+		printf("I: %d: remainder blob is zero. OK\n", __LINE__);
+
+	/* if the blob is smaller than the hash, the remainder should
+	 * not be touched */
+	sms_msg_id_init(&msg_id1);
+	sms_msg_id_random(&msg_id1);
+
+	sms_msg_id_init(&msg_id2);
+	sms_msg_id_random(&msg_id2);
+
+	sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1);
+	/* Note we cut by four here! */
+	memcpy(blob1, blob2, sizeof(blob1));
+	sms_msg_id_memcpy(blob2, id_length / 4, &msg_id2);
+
+	for (cnt = id_length / 4; cnt < id_length; cnt++)
+		if (blob1[cnt] != blob2[cnt]) {
+			fprintf(stderr, "E: %d: blob1 @ #%u doesn't match "
+				"blob2. FAILURE.\n", __LINE__, cnt);
+			fail = 1;
+			break;
+		}
+	if (cnt >= id_length)
+		printf("I: %d: remainder blob is untouched. OK\n",
+		       __LINE__);
+
+	if (fail)
+		fprintf(stderr, "E: FAILURE.\n");
+	else
+		printf("I: OK\n");
+	return fail;
+}
-- 
1.6.6.1

[patch 05/20] introduce DECLARE_SMS_ADDR_STR()

[Inaky Perez-Gonzalez]

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

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

Introduce DECLARE_SMS_ADDR_STR(), which declares a string variable of
the right size for passing to sms_assembly_decode_address(). This way
we detach each client having to have the knowledge of what the right
size is, leaving that decission to the infrastructure
provider. Updated couple of sites in smsutil.c to use it vs a raw
declaration.
---
 src/smsutil.c |    4 ++--
 src/smsutil.h |    6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index f93ea5b..3f743ef 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2315,7 +2315,7 @@ static void sms_assembly_load(struct sms_assembly *assembly,
 				const struct dirent *dir)
 {
 	struct sms_address addr;
-	char straddr[25];
+	DECLARE_SMS_ADDR_STR(straddr);
 	guint16 ref;
 	guint8 max;
 	guint8 seq;
@@ -2392,7 +2392,7 @@ static gboolean sms_assembly_store(struct sms_assembly *assembly,
 {
 	unsigned char buf[177];
 	int len;
-	char straddr[25];
+	DECLARE_SMS_ADDR_STR(straddr);
 
 	if (!assembly->imsi)
 		return FALSE;
diff --git a/src/smsutil.h b/src/smsutil.h
index fbe3c72..59297a0 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -438,6 +438,12 @@ gboolean sms_decode_unpacked_stk_pdu(const unsigned char *pdu, int len,
 gboolean sms_encode(const struct sms *in, int *len, int *tpdu_len,
 			unsigned char *pdu);
 
+/*
+ * Length is based on the address being 12 hex characters plus a
+ * terminating NUL char. See sms_assembly_extract_address().
+ */
+#define DECLARE_SMS_ADDR_STR(a) char a[25]
+
 gboolean sms_decode_address_field(const unsigned char *pdu, int len,
 					int *offset, gboolean sc,
 					struct sms_address *out);
-- 
1.6.6.1

[patch 06/20] _assembly_encode_address: export and rename

[Inaky Perez-Gonzalez]

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

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

The new name better reflects the function's purpose.

We need to export it, as for generating unique message naming (for
persistence and D-Bus object naming), we'll be using the
address.
---
 src/smsutil.c |    7 +++----
 src/smsutil.h |    2 ++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 3f743ef..969c1b7 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2294,8 +2294,7 @@ static gboolean sms_assembly_extract_address(const char *straddr,
 	return sms_decode_address_field(pdu, len, &offset, FALSE, out);
 }
 
-static gboolean sms_assembly_encode_address(const struct sms_address *in,
-						char *straddr)
+gboolean sms_address_to_hex_string(const struct sms_address *in, char *straddr)
 {
 	unsigned char pdu[12];
 	int offset = 0;
@@ -2397,7 +2396,7 @@ static gboolean sms_assembly_store(struct sms_assembly *assembly,
 	if (!assembly->imsi)
 		return FALSE;
 
-	if (sms_assembly_encode_address(&node->addr, straddr) == FALSE)
+	if (sms_address_to_hex_string(&node->addr, straddr) == FALSE)
 		return FALSE;
 
 	len = sms_serialize(buf, sms);
@@ -2420,7 +2419,7 @@ static void sms_assembly_backup_free(struct sms_assembly *assembly,
 	if (!assembly->imsi)
 		return;
 
-	if (sms_assembly_encode_address(&node->addr, straddr) == FALSE)
+	if (sms_address_to_hex_string(&node->addr, straddr) == FALSE)
 		return;
 
 	for (seq = 0; seq < node->max_fragments; seq++) {
diff --git a/src/smsutil.h b/src/smsutil.h
index 59297a0..28b5604 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -506,6 +506,8 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					const struct sms_address *addr,
 					guint16 ref, guint8 max, guint8 seq);
 void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
+gboolean sms_address_to_hex_string(const struct sms_address *in,
+				   char *straddr);
 
 struct status_report_assembly *status_report_assembly_new(const char *imsi);
 void status_report_assembly_free(struct status_report_assembly *assembly);
-- 
1.6.6.1

[patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 7246 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 the same number also have a different ID.

Note that in order to feed the reference 16-bit ID to
set_ref_and_to(), the UUID has to be computed as early as
possible.
---
 src/ofono.h |    4 +-
 src/sms.c   |   68 +++++++++++++++++++++++++++++++++++++---------------------
 src/stk.c   |    4 +-
 3 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index aaa01d9..1b72381 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -185,8 +185,8 @@ 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,
+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);
 
diff --git a/src/sms.c b/src/sms.c
index 71e24c1..49f3e54 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;
@@ -590,7 +589,12 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	struct ofono_modem *modem;
 	unsigned int flags;
 	unsigned int msg_id;
-
+	struct sms_msg_id sms_msg_id;
+	DECLARE_SMS_MSG_ID_STRBUF(msg_id_str);
+	struct sms_address receiver;
+	DECLARE_SMS_ADDR_STR(receiver_str);
+	struct tx_queue_entry *sms_msg;
+	
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
 					DBUS_TYPE_INVALID))
@@ -598,6 +602,16 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	if (valid_phone_number_format(to) == FALSE)
 		return __ofono_error_invalid_format(msg);
+	/*
+	 * Instead of using the telephone number/address we got from
+	 * D-Bus, we do the reverse formatting, so we get something
+	 * that has been normalized--this is used later and we do it
+	 * here to simplify error handling.
+	 */
+	sms_address_from_string(&receiver, to);
+	if (sms_address_to_hex_string(&receiver, receiver_str)
+	    == FALSE)
+		return __ofono_error_failed(msg);
 
 	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
 					sms->use_delivery_reports);
@@ -605,22 +619,21 @@ 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;
-	}
+	sms_msg_id_init(&sms_msg_id);
+	sms_msg_id_hash(&sms_msg_id, text, strlen(text));
+	sms_msg_id_hash(&sms_msg_id, receiver_str, strlen(receiver_str));
+	sms_msg_id_hash(&sms_msg_id, NULL, 0);
+	sms_msg_id_memcpy(&msg_id, sizeof(msg_id), &sms_msg_id);
+	sms_msg_id_printf(&sms_msg_id, msg_id_str, sizeof(msg_id_str));
+	set_ref_and_to(msg_list, msg_id, ref_offset, to);
+	DBG("ref: %d, offset: %d", msg_id, ref_offset);
 
 	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,
+	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id, send_message_cb,
 						dbus_message_ref(msg),
 						send_message_destroy);
 
@@ -676,6 +689,9 @@ static void dispatch_text_message(struct ofono_sms *sms,
 	struct tm remote;
 	struct tm local;
 	const char *str = buf;
+	struct sms_msg_id sms_msg_id;
+	unsigned int id;
+	DECLARE_SMS_MSG_ID_STRBUF(msg_id_str);
 
 	if (!message)
 		return;
@@ -717,11 +733,18 @@ 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,
+	sms_msg_id_init(&sms_msg_id);
+	sms_msg_id_hash(&sms_msg_id, message, strlen(message));
+	sms_msg_id_hash(&sms_msg_id, addr->address, strlen(addr->address));
+	/* FIXME: add RX time? */
+	sms_msg_id_hash(&sms_msg_id, NULL, 0);
+	sms_msg_id_memcpy(&id, sizeof(id), &sms_msg_id);
+	sms_msg_id_printf(&sms_msg_id, msg_id_str, sizeof(msg_id_str));
+	ofono_debug("SMS RX: msg id is %s\n", msg_id_str);
+
+	if (cls != SMS_CLASS_0)
+		__ofono_history_sms_received(modem, id, str,
 						&remote, &local, message);
-		sms->next_msg_id += 1;
-	}
 }
 
 static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
@@ -1104,8 +1127,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 +1222,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,8 +1324,8 @@ 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,
+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)
 {
@@ -1320,7 +1338,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 +1349,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..400b033 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -338,8 +338,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,
+#warning FIXME: fix msg_id, missing
+	__ofono_sms_txq_submit(sms, &msg_list, 0, 0, send_sms_submit_cb,
 				stk->sms_submit_req, g_free);
 
 	stk->cancel_cmd = send_sms_cancel;
-- 
1.6.6.1

[patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2325 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..af4baae 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)
+		string SendMessage(string destination_number, 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{string} PendingMessages()
+
+                        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]/modemX/message_id
+
+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 TXState
+
+			Current transmission state.
+
-- 
1.6.6.1

[patch 09/20] 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 49f3e54..b53f880 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -887,6 +887,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

[patch 10/20] sms_text_prepare: document @use_delivery_reports

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/smsutil.c b/src/smsutil.c
index 969c1b7..3ca5979 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2831,6 +2831,9 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)
  * Returns a list of sms messages in order.  If ref_offset is given,
  * then the ref_offset contains the reference number offset or 0
  * if no concatenation took place.
+ *
+ * @use_delivery_reports: value for the Status-Report-Request field
+ *     (23.040 3.2.9, 9.2.2.2)
  */
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
 				gboolean use_16bit, int *ref_offset,
-- 
1.6.6.1

[patch 11/20] SMS: rename create_tx_queue_entry() to tx_queue_entry_new()

[Inaky Perez-Gonzalez]

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

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

This is for symmetry with tx_queue_entry_free()
---
 src/sms.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index b53f880..85353ff 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -522,7 +522,7 @@ static void set_ref_and_to(GSList *msg_list, guint16 ref, int offset,
 	}
 }
 
-static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)
+static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list)
 {
 	struct tx_queue_entry *entry = g_new0(struct tx_queue_entry, 1);
 	int i = 0;
@@ -574,7 +574,7 @@ static void send_message_destroy(void *data)
  *
  * 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
- * is created by create_tx_queue_entry() and g_queue_push_tail()
+ * 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.
  */
@@ -1338,7 +1338,7 @@ struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *li
 					ofono_sms_txq_submit_cb_t cb,
 					void *data, ofono_destroy_func destroy)
 {
-	struct tx_queue_entry *entry = create_tx_queue_entry(list);
+	struct tx_queue_entry *entry = tx_queue_entry_new(list);
 
 	if (flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR) {
 		struct sms *head = list->data;
-- 
1.6.6.1

[patch 12/20] struct tx_queue_entry: add a destructor

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1610 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 |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 85353ff..5f5df04 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -405,6 +405,23 @@ 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_free(gpointer _entry, gpointer unused)
+{
+	struct tx_queue_entry *entry = _entry;
+
+	if (entry->destroy)
+		entry->destroy(entry->data);
+	g_free(entry->pdus);
+	g_free(entry);
+}
+
 static void tx_finished(const struct ofono_error *error, int mr, void *data)
 {
 	struct ofono_sms *sms = data;
@@ -465,11 +482,7 @@ next_q:
 						time(NULL), hs);
 	}
 
-	if (entry->destroy)
-		entry->destroy(entry->data);
-
-	g_free(entry->pdus);
-	g_free(entry);
+	tx_queue_entry_destroy_free(entry, NULL);
 
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
@@ -1129,7 +1142,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_free, NULL);
 		g_queue_free(sms->txq);
 		sms->txq = NULL;
 	}
-- 
1.6.6.1

[patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'

[Inaky Perez-Gonzalez]

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

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

This will ease adding later on other fields that are D-Bus specific
and needed to export SMS messages in TX transit over D-Bus.
---
 src/sms.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 5f5df04..7bde919 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -90,6 +90,17 @@ struct tx_queue_entry {
 	ofono_destroy_func destroy;
 };
 
+/*
+ * Encapsulate information needed to export an SMS message over D-Bus.
+ *
+ * @dbus_path: path of the object in the D-Bus
+ * @dbus_msg: message this originated at
+ */
+struct sms_msg_dbus_data {
+	char *dbus_path;
+	DBusMessage *dbus_msg;
+};
+
 static const char *sms_bearer_to_string(int bearer)
 {
 	switch (bearer) {
@@ -560,7 +571,8 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list)
 static void send_message_cb(gboolean ok, void *data)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
-	DBusMessage *msg = data;
+	struct sms_msg_dbus_data *dbus_data = data;
+	DBusMessage *msg = dbus_data->dbus_msg;
 	DBusMessage *reply;
 
 	if (ok)
@@ -573,9 +585,11 @@ static void send_message_cb(gboolean ok, void *data)
 
 static void send_message_destroy(void *data)
 {
-	DBusMessage *msg = data;
+	struct sms_msg_dbus_data *dbus_data = data;
+	DBusMessage *msg = dbus_data->dbus_msg;
 
 	dbus_message_unref(msg);
+	g_free(dbus_data);
 }
 
 /*
@@ -590,6 +604,9 @@ static void send_message_destroy(void *data)
  * 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 DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 					void *data)
-- 
1.6.6.1

[patch 14/20] SMS: introduce bare state machine and transitions

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 8351 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   |  121 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 150 insertions(+), 4 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index 1b72381..51c2743 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(struct ofono_sms *sms, GSList *list,
diff --git a/src/sms.c b/src/sms.c
index 7bde919..de6eb7f 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -50,6 +50,23 @@ 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 +94,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;
@@ -417,11 +440,91 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *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'
  *
  * This releases resources allocated *inside* @entry and @entry
- * itself.
+ * itself. We blindly remove from the submission queue, in case it is
+ * still in any. We could do it with a 'checked' state change to
+ * INVALID if we modify that function.
  */
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
 {
@@ -430,6 +533,8 @@ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
 	if (entry->destroy)
 		entry->destroy(entry->data);
 	g_free(entry->pdus);
+	g_queue_remove(entry->sms_mgr->txq, entry);
+	entry->state = __OFONO_SMS_TX_ST_INVALID;
 	g_free(entry);
 }
 
@@ -443,8 +548,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;
 
@@ -457,6 +564,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;
 	}
 
@@ -476,7 +584,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);
@@ -488,10 +596,13 @@ next_q:
 			hs = OFONO_HISTORY_SMS_STATUS_SUBMITTED;
 		else
 			hs = OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED;
-
 		__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_free(entry, NULL);
 
@@ -1382,8 +1493,10 @@ struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *li
 	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

[patch 15/20] SMS: introduce Wait-for-Status-Report state and infrastructure

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 6217 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 |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index de6eb7f..1075f33 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -67,6 +67,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;
@@ -74,6 +79,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;
@@ -487,10 +493,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(
@@ -522,8 +537,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
  * Destroy/release the contents of a 'struct tx_queue_entry'
  *
  * This releases resources allocated *inside* @entry and @entry
- * itself. We blindly remove from the submission queue, in case it is
- * still in any. We could do it with a 'checked' state change to
+ * itself. We blindly remove from both submission queues, in case it
+ * is still in any. We could do it with a 'checked' state change to
  * INVALID if we modify that function.
  */
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
@@ -534,6 +549,7 @@ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
 		entry->destroy(entry->data);
 	g_free(entry->pdus);
 	g_queue_remove(entry->sms_mgr->txq, entry);
+	g_queue_remove(entry->sms_mgr->tx_wsrq, entry);
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
 	g_free(entry);
 }
@@ -586,6 +602,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 next_q:
 	entry = g_queue_peek_head(sms->txq);
 
+#warning FIXME: cb should not be called on WSR?
 	if (entry->cb)
 		entry->cb(ok, entry->data);
 
@@ -599,12 +616,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_free(entry, NULL);
+	} else {
 		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
-
-	tx_queue_entry_destroy_free(entry, NULL);
+		tx_queue_entry_destroy_free(entry, NULL);
+	}
 
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
@@ -1030,11 +1051,31 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 
 
 /*
+ * 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 _msg_id)
+{
+	unsigned msg_id = (unsigned) _msg_id;
+	struct tx_queue_entry *sms_msg = _sms_msg;
+
+	if (sms_msg->msg_id != msg_id)
+		return;
+	ofono_debug("SMS: ACKED %p msg_id match %x", sms_msg, msg_id);
+	g_queue_remove(sms_msg->sms_mgr->tx_wsrq, sms_msg);
+	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_DONE);
+	tx_queue_entry_destroy_free(sms_msg, NULL);
+}
+
+
+/*
  * 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,
@@ -1048,6 +1089,9 @@ static void handle_sms_status_report(struct ofono_sms *sms,
 						&delivered) == FALSE)
 		return;
 
+	g_queue_foreach(sms->tx_wsrq, __sms_find_destroy_by_msg_id,
+			(void *) msg_id);
+
 	__ofono_history_sms_send_status(modem, msg_id, time(NULL),
 			delivered ? OFONO_HISTORY_SMS_STATUS_DELIVERED :
 			OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);
@@ -1275,6 +1319,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_free, 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);
@@ -1329,6 +1380,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

[patch 16/20] SMS: introduce a state change callback for TX messages

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 5847 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 takes 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   |   36 +++++++++++++++++++++++++-----------
 src/stk.c   |   22 ++++++++++++++++++----
 3 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index 51c2743..d5ac6f5 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -216,11 +216,12 @@ 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 1075f33..668a645 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -114,7 +114,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;
 };
@@ -530,6 +530,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);
 }
 
 
@@ -602,10 +604,6 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 next_q:
 	entry = g_queue_peek_head(sms->txq);
 
-#warning FIXME: cb should not be called on WSR?
-	if (entry->cb)
-		entry->cb(ok, entry->data);
-
 	if (entry->flags & OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY) {
 		enum ofono_history_sms_status hs;
 
@@ -700,12 +698,27 @@ 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();
 	struct sms_msg_dbus_data *dbus_data = data;
 	DBusMessage *msg = dbus_data->dbus_msg;
 	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);
@@ -795,9 +808,10 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	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_cb,
-						dbus_message_ref(msg),
-						send_message_destroy);
+	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
+					 send_message_stch_cb,
+					 dbus_message_ref(msg),
+					 send_message_destroy);
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
@@ -1528,7 +1542,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);
@@ -1542,7 +1556,7 @@ struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *li
 
 	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 400b033..c253aa2 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -287,13 +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");
 
 	if (req->cancelled) {
@@ -339,8 +353,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;
 #warning FIXME: fix msg_id, missing
-	__ofono_sms_txq_submit(sms, &msg_list, 0, 0, send_sms_submit_cb,
-				stk->sms_submit_req, g_free);
+	__ofono_sms_txq_submit(sms, &msg_list, 0, 0, send_sms_stch_cb,
+			       stk->sms_submit_req, g_free);
 
 	stk->cancel_cmd = send_sms_cancel;
 
-- 
1.6.6.1

[patch 17/20] SMS: export outgoing messages over D-Bus

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 11175 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 needed in a 'struct
  sms_msg_dbus_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     |  192 +++++++++++++++++++++++++++++++++++++++------------------
 src/smsutil.h |   12 ++++
 2 files changed, 145 insertions(+), 59 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 668a645..4569cf3 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"
 
@@ -123,11 +127,9 @@ struct tx_queue_entry {
  * Encapsulate information needed to export an SMS message over D-Bus.
  *
  * @dbus_path: path of the object in the D-Bus
- * @dbus_msg: message this originated at
  */
 struct sms_msg_dbus_data {
 	char *dbus_path;
-	DBusMessage *dbus_msg;
 };
 
 static const char *sms_bearer_to_string(int bearer)
@@ -536,6 +538,26 @@ 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
@@ -554,6 +576,7 @@ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
 	g_queue_remove(entry->sms_mgr->tx_wsrq, 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_finished(const struct ofono_error *error, int mr, void *data)
@@ -694,55 +717,35 @@ 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)
-{
-	DBusConnection *conn = ofono_dbus_get_connection();
-	struct sms_msg_dbus_data *dbus_data = data;
-	DBusMessage *msg = dbus_data->dbus_msg;
-	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);
-	else
-		reply = __ofono_error_failed(msg);
-
-	g_dbus_send_message(conn, reply);
-}
-
-static void send_message_destroy(void *data)
+static void sms_msg_dbus_destroy(void *_dbus_data)
 {
-	struct sms_msg_dbus_data *dbus_data = data;
-	DBusMessage *msg = dbus_data->dbus_msg;
+	struct sms_msg_dbus_data *dbus_data = _dbus_data;
 
-	dbus_message_unref(msg);
+	if (dbus_data == NULL)
+		return;
+	g_dbus_unregister_interface(ofono_dbus_get_connection(),
+				    dbus_data->dbus_path, SMS_MSG_INTERFACE);
+	g_free(dbus_data->dbus_path);
+	memset(dbus_data, 0, sizeof(*dbus_data));
 	g_free(dbus_data);
 }
 
+
 /*
- * 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
@@ -753,12 +756,12 @@ static void send_message_destroy(void *data)
  * @sms is the main SMS driver struct, @entry and @msg_list represent
  * the current message being processed.
  */
-static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
-					void *data)
+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;
@@ -770,13 +773,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	DECLARE_SMS_ADDR_STR(receiver_str);
 	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;
 	/*
 	 * Instead of using the telephone number/address we got from
 	 * D-Bus, we do the reverse formatting, so we get something
@@ -786,13 +784,13 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	sms_address_from_string(&receiver, to);
 	if (sms_address_to_hex_string(&receiver, receiver_str)
 	    == FALSE)
-		return __ofono_error_failed(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;
 
 	sms_msg_id_init(&sms_msg_id);
 	sms_msg_id_hash(&sms_msg_id, text, strlen(text));
@@ -803,23 +801,100 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	set_ref_and_to(msg_list, msg_id, ref_offset, to);
 	DBG("ref: %d, offset: %d", msg_id, ref_offset);
 
-	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;
+	struct sms_msg_dbus_data *dbus_data;
+	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);
+
+	dbus_data = g_new0(struct sms_msg_dbus_data, 1);
+	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY,
+			       NULL,
+			       sms_msg_dbus_destroy,
+			       dbus_data);
+	if (sms_msg == NULL)
+		goto error_sms_msg_send;
+	/* We can only fill out these data when we have the
+	 * information we need off @sms_msg; this is all released by @
+	 * sms_msg_dbus_destroy() */
+	dbus_data->dbus_path = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
+					       sms_msg->msg_id,
+					       sms_msg->num_pdus);
+
+	if (!g_dbus_register_interface(ofono_dbus_get_connection(),
+				       dbus_data->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_data->dbus_path,
+			    SMS_MSG_INTERFACE);
+		goto error_dbus_register_interface;
+	}
+	ofono_debug("%s():%d: sms %p @ %s: MSG registered",
+		    __func__, __LINE__, sms_msg, dbus_data->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_data->dbus_path,
+				 DBUS_TYPE_INVALID);
+	return reply;
+
+
+error_dbus_new_method_return:
+	g_dbus_unregister_interface(ofono_dbus_get_connection(),
+				    dbus_data->dbus_path, SMS_MSG_INTERFACE);
+error_dbus_register_interface:
+	g_free(dbus_data->dbus_path);
+	/*
+	 * 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_free(sms_msg, NULL);
+error_sms_msg_send:
+	g_free(dbus_data);
+	return __ofono_error_invalid_format(msg);
 }
 
 static GDBusMethodTable sms_manager_methods[] = {
@@ -827,8 +902,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 28b5604..20b24f0 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -215,6 +215,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;
@@ -634,3 +640,9 @@ void sms_msg_id_memcpy(void *, size_t, const struct sms_msg_id *);
 
 /* for unit testing only */
 gsize __sms_msg_id_hash_length(void);
+
+/*
+ * UNIQUE-ID_#PDUS -- note we use underscores as to generate a name
+ * suitable to be a D-Bus name.
+ */
+#define SMS_MSG_NAME_FMT "%s/%04x_%i"
-- 
1.6.6.1

[patch 18/20] SMS: send D-Bus SMS-MSG::PropertyChanged signals when message changes status

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 3224 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 +++++++++++++++++++++++++++++++++++-
 test/test-sms-msg-state-change |   24 ++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletions(-)
 create mode 100755 test/test-sms-msg-state-change

diff --git a/src/sms.c b/src/sms.c
index 4569cf3..c0b2e00 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -820,6 +820,40 @@ struct tx_queue_entry *sms_msg_send(
 
 
 /*
+ * Send a PropertyChange signal when the state changes
+ *
+ * D-Bus typing is "ss" for variable/value.
+ */
+static
+void sms_msg_stch_dbus_cb(void *_dbus_data,
+			  enum ofono_sms_tx_state new_state)
+{
+	struct sms_msg_dbus_data *dbus_data = _dbus_data;
+	DBusConnection *dbus_conn = ofono_dbus_get_connection();
+	DBusMessage *dbus_signal;
+	DBusMessageIter iter;
+	const char *property = "TXState";
+	const char *new_state_str;
+
+	/*
+	 * This will be called in the first transition, when all the
+	 * D-Bus stuff is not yet in place.
+	 */
+	if (dbus_data->dbus_path == NULL)
+		return;
+	dbus_signal = dbus_message_new_signal(
+		dbus_data->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
@@ -846,7 +880,7 @@ static DBusMessage *dbus_sms_msg_send(DBusConnection *conn, DBusMessage *msg,
 
 	dbus_data = g_new0(struct sms_msg_dbus_data, 1);
 	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY,
-			       NULL,
+			       sms_msg_stch_dbus_cb,
 			       sms_msg_dbus_destroy,
 			       dbus_data);
 	if (sms_msg == NULL)
diff --git a/test/test-sms-msg-state-change b/test/test-sms-msg-state-change
new file mode 100755
index 0000000..18486bf
--- /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

[patch 19/20] SMS: introduce sms_msg_cancel and its D-Bus wrapper

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 12523 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                |   63 +++++++++++++++++
 src/smsutil.h            |   15 ++++
 test/test-sms-msg-cancel |  173 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+), 0 deletions(-)
 create mode 100755 test/test-sms-msg-cancel

diff --git a/src/sms.c b/src/sms.c
index c0b2e00..97c9eab 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -538,6 +538,24 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *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)
+{
+	gboolean do_network_cancel = FALSE;
+	struct tx_queue_entry *sms_msg = data;
+	if (!dbus_message_get_args(msg, NULL,
+				   DBUS_TYPE_BOOLEAN, &do_network_cancel))
+		return __ofono_error_invalid_args(msg);
+	sms_msg_cancel(sms_msg,
+		       do_network_cancel ? SMS_MSG_CANCEL_IN_NETWORK : 0);
+	return dbus_message_new_method_return(msg);
+}
+
+
+/*
  * D-Bus SMS Message interface
  *
  * NOTE: the sms_msg_{methods,signals} tables should be const, but
@@ -546,6 +564,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 
 static
 GDBusMethodTable sms_msg_methods[] = {
+	{ "Cancel",		DBUS_TYPE_BOOLEAN_AS_STRING,	"",
+	  dbus_sms_msg_cancel, 0 },
 	{ }
 };
 
@@ -587,6 +607,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)) {
@@ -819,6 +844,44 @@ struct tx_queue_entry *sms_msg_send(
 }
 
 
+/**
+ * Cancel a pending SMS message
+ *
+ * @sms_msg: message to cancel
+ * @flags: modifies for the action
+ *
+ * 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.
+ */
+void sms_msg_cancel(struct tx_queue_entry *sms_msg,
+		    enum sms_msg_cancel_flags flags)
+{
+	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);
+	if (flags & SMS_MSG_CANCEL_IN_NETWORK)
+		ofono_warn("%s(): SMS_MSG_CANCEL_IN_NETWORK support "
+			   "not yet implemented\n", __func__);
+	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELLED);
+	tx_queue_entry_destroy_free(sms_msg, NULL);
+}
+
+
 /*
  * Send a PropertyChange signal when the state changes
  *
diff --git a/src/smsutil.h b/src/smsutil.h
index 20b24f0..fce4278 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -560,6 +560,21 @@ gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges);
 
 char *ussd_decode(int dcs, int len, const unsigned char *data);
 
+/*
+ * SMS Message API
+ */
+
+enum sms_msg_cancel_flags {
+	/**
+	 * The cancelled message will be reported to the network so it
+	 * tries to cancel it if in transit.
+	 */
+	SMS_MSG_CANCEL_IN_NETWORK = 0x1,
+};
+
+struct tx_queue_entry;
+void sms_msg_cancel(struct tx_queue_entry *sms_msg, enum sms_msg_cancel_flags);
+
 enum {
 	/* Note this has to be a multiple of sizeof(guint32);
 	 * Compilation will assert-fail if this is not met. */
diff --git a/test/test-sms-msg-cancel b/test/test-sms-msg-cancel
new file mode 100755
index 0000000..24cc7e2
--- /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(0)
+                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 != "TXState":
+                        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

[patch 20/20] SMS: Implement D-Bus SMS-MSG::GetProperties

[Inaky Perez-Gonzalez]

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

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

Currently this only returns the state of the SMS message.
---
 src/sms.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 97c9eab..5be2e39 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -537,6 +537,29 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 }
 
 
+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, "TXState", DBUS_TYPE_STRING, &str);
+	dbus_message_iter_close_container(&iter, &dict);
+	return reply;
+}
+
+
 /*
  * Note that the D-Bus specific cleanups are taken care by the
  * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send().
@@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(
 
 static
 GDBusMethodTable sms_msg_methods[] = {
+	{ "GetProperties",	"",	"a{sv}",
+	  dbus_sms_msg_get_properties, G_DBUS_METHOD_FLAG_ASYNC },
 	{ "Cancel",		DBUS_TYPE_BOOLEAN_AS_STRING,	"",
 	  dbus_sms_msg_cancel, 0 },
 	{ }
-- 
1.6.6.1

Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> These have been stolen from the Linux kernel source; come pretty handy
> to make build-time consistency checks and thus avoid run-time
> surprises.
> ---
>  src/bug.h     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/smsutil.c |    1 +
>  2 files changed, 51 insertions(+), 0 deletions(-)
>  create mode 100644 src/bug.h
> 

First of all, please refer to doc/coding-style.txt Section M5.

Second, please actually check that your code passes make distcheck:

  CCLD   unit/test-idmap
  CC     unit/test-sms.o
  CC     src/smsutil.o
../src/smsutil.c:40:17: error: bug.h: No such file or directory
make[2]: *** [src/smsutil.o] Error 1
make[1]: *** [all] Error 2
make: *** [distcheck] Error 1

> diff --git a/src/smsutil.c b/src/smsutil.c
> index e41c041..6c2087a 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -37,6 +37,7 @@
>  #include "util.h"
>  #include "storage.h"
>  #include "smsutil.h"
> +#include "bug.h"

Why are you including this without actually using it?

>  
>  #define uninitialized_var(x) x = x
>  

Regards,
-Denis

Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 14:41 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > These have been stolen from the Linux kernel source; come pretty handy
> > to make build-time consistency checks and thus avoid run-time
> > surprises.
> > ---
> >  src/bug.h     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/smsutil.c |    1 +
> >  2 files changed, 51 insertions(+), 0 deletions(-)
> >  create mode 100644 src/bug.h
> > 
> 
> First of all, please refer to doc/coding-style.txt Section M5.

Ops, miss, reworked them in the whole patchset

> Second, please actually check that your code passes make distcheck:
> 
>   CCLD   unit/test-idmap
>   CC     unit/test-sms.o
>   CC     src/smsutil.o
> ../src/smsutil.c:40:17: error: bug.h: No such file or directory
> make[2]: *** [src/smsutil.o] Error 1
> make[1]: *** [all] Error 2
> make: *** [distcheck] Error 1

another FAIL. However, I am hitting another one in missing
ofono/types.h, even after having pulled the tip as of today. Is that one
known? Anyway, fixing this one.

> > diff --git a/src/smsutil.c b/src/smsutil.c
> > index e41c041..6c2087a 100644
> > --- a/src/smsutil.c
> > +++ b/src/smsutil.c
> > @@ -37,6 +37,7 @@
> >  #include "util.h"
> >  #include "storage.h"
> >  #include "smsutil.h"
> > +#include "bug.h"
> 
> Why are you including this without actually using it?

It's used in a follow commit -- I mispliced that, sorry. Reworking it
for the next submission.

Re: [patch 02/20] write_file: make transaction-safe

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 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.
> ---
>  src/storage.c |   42 +++++++++++++++++++++++++++++++-----------
>  1 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/src/storage.c b/src/storage.c
> index cac5835..c88a8c8 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,36 @@ 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;

Please do me a favor and add an empty line here.

> +	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. */

Please follow comment conventions per doc/coding-style.txt Section M2.

> +	unlink(path);

There should be an empty line here per doc/coding-style.txt Section M1.

> +	/* conserve @r's value from 'write' */
> +	if (link(tmp_path, path) == -1)
> +		r = -1;

Another empty line here (before and after if/while/do/for blocks)

> +error_write:
> +	unlink(tmp_path);
> +error_mkstemp_full:
> +error_create_dirs:
> +	g_free(tmp_path);
>  	g_free(path);
>  	return r;
>  }

Regards,
-Denis

Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Denis Kenzior]

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

Hi Inaky,

>> Second, please actually check that your code passes make distcheck:
>>
>>   CCLD   unit/test-idmap
>>   CC     unit/test-sms.o
>>   CC     src/smsutil.o
>> ../src/smsutil.c:40:17: error: bug.h: No such file or directory
>> make[2]: *** [src/smsutil.o] Error 1
>> make[1]: *** [all] Error 2
>> make: *** [distcheck] Error 1
> 
> another FAIL. However, I am hitting another one in missing
> ofono/types.h, even after having pulled the tip as of today. Is that one
> known? Anyway, fixing this one.
> 

Make distcheck passes fine on $HEAD for me.

Regards,
-Denis

Re: [patch 03/20] manpage: explain debugging options to -d

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> Modified HACKING and man page to have more formation on what are the
> debugging options and how to enable them.
> ---
>  HACKING      |   10 ++++++++++
>  doc/ofonod.8 |    5 ++++-
>  2 files changed, 14 insertions(+), 1 deletions(-)
> 

This one has been applied, thanks.

Regards,
-Denis

Re: [patch 05/20] introduce DECLARE_SMS_ADDR_STR()

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> Introduce DECLARE_SMS_ADDR_STR(), which declares a string variable of
> the right size for passing to sms_assembly_decode_address(). This way
> we detach each client having to have the knowledge of what the right
> size is, leaving that decission to the infrastructure
> provider. Updated couple of sites in smsutil.c to use it vs a raw
> declaration.
> ---
>  src/smsutil.c |    4 ++--
>  src/smsutil.h |    6 ++++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

This one has been applied, thanks.

Regards,
-Denis

Re: [patch 06/20] _assembly_encode_address: export and rename

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> The new name better reflects the function's purpose.
> 
> We need to export it, as for generating unique message naming (for
> persistence and D-Bus object naming), we'll be using the
> address.
> ---
>  src/smsutil.c |    7 +++----
>  src/smsutil.h |    2 ++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 

Applied with a very small tweak to the commit header.  Thanks.

Regards,
-Denis

Re: [patch 02/20] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 14:57 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/23/2010 03:59 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.

all fixed, thanks

Re: [patch 10/20] sms_text_prepare: document @use_delivery_reports

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> ---
>  src/smsutil.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 

This one has been applied with a minor tweak to the commit message.

Regards,
-Denis

Re: [patch 11/20] SMS: rename create_tx_queue_entry() to tx_queue_entry_new()

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> This is for symmetry with tx_queue_entry_free()
> ---
>  src/sms.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 

Applied with a minor tweak to the commit header.

Regards,
-Denis

Re: [patch 12/20] struct tx_queue_entry: add a destructor

[Denis Kenzior]

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

Hi Inaky,

> +
> +/*
> + * 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_free(gpointer _entry, gpointer unused)

Can't this be simply tx_queue_entry_free?  And feel free to take struct
tx_queue_entry * here.  Casting the destructor in g_foo_foreach is
acceptable.

> +{
> +	struct tx_queue_entry *entry = _entry;
> +
> +	if (entry->destroy)
> +		entry->destroy(entry->data);

An empty line after if/while/for blocks please.  I will update the
coding-style document with this rule shortly.

> +	g_free(entry->pdus);
> +	g_free(entry);
> +}
> +

Regards,
-Denis

Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> 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..af4baae 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)
> +		string SendMessage(string destination_number, string text)

Actually keep string to, I like that better.  The returned type should
be 'object' (the object path of the new message) not 'string'.

>  
> -			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.

Try to use tabs and not spaces for indentation.

> +
> +			Possible Errors: [service].Error.InvalidArguments
> +
> +                array{string} PendingMessages()

array{object} here, and using Messages is better.

> +
> +                        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

oFono API is full CamelCase, so this should be SmsMessage.

> +Object path	[variable prefix]/modemX/message_id

Please follow the conventions for Object Path, e.g. see
doc/voicecall-api.txt

> +
> +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 TXState
> +
> +			Current transmission state.
> +

Rename this to State and include the possible state values please.

Regards,
-Denis

Re: [patch 12/20] struct tx_queue_entry: add a destructor

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 16:06 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +
> > +/*
> > + * 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_free(gpointer _entry, gpointer unused)
> 
> Can't this be simply tx_queue_entry_free?  And feel free to take struct

Sure, I'll rename that.

> tx_queue_entry * here.  Casting the destructor in g_foo_foreach is
> acceptable.

Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts
are evil and should be avoided for they muck the code by hiding warnings
when you change things and miss updating all the sites ... 

> > +{
> > +	struct tx_queue_entry *entry = _entry;
> > +
> > +	if (entry->destroy)
> > +		entry->destroy(entry->data);
> 
> An empty line after if/while/for blocks please.  I will update the
> coding-style document with this rule shortly.

Will do, thanks

Re: [patch 12/20] struct tx_queue_entry: add a destructor

[Denis Kenzior]

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

Hi Inaky,

>> tx_queue_entry * here.  Casting the destructor in g_foo_foreach is
>> acceptable.
> 
> Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts
> are evil and should be avoided for they muck the code by hiding warnings
> when you change things and miss updating all the sites ... 
> 

I fully agree, but the extra silly NULL argument makes this worse than
the cast.  And to be clear, g_foo_foreach is about the only place we
explicitly allow casts.  If you feel you need to be pedantic, then write
a GFunc specifically to do the foreach bit.

e.g. tx_queue_entry_free(struct tx_queue_entry *);

txq_foreach_free(gpointer foo, gpointer user)
{
  struct tx_queue_entry *bar = foo;
  tx_queue_entry_free(bar);
}

Regards,
-Denis

Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 16:11 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> > 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..af4baae 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)
> > +		string SendMessage(string destination_number, string text)
> 
> Actually keep string to, I like that better.  The returned type should
> be 'object' (the object path of the new message) not 'string'.

Changed

> >  
> > -			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.
> 
> Try to use tabs and not spaces for indentation.

Changed to tab (x8)

> > +
> > +			Possible Errors: [service].Error.InvalidArguments
> > +
> > +                array{string} PendingMessages()
> 
> array{object} here, and using Messages is better.

Done

> > +
> > +                        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
> 
> oFono API is full CamelCase, so this should be SmsMessage.

I disagree. It would be like that if SMS was a word, but it is an
acronym. It makes it quite confusing, if you ask me. If that's still how
you like it, I'll change it, but I think it is wrong to capitalize an
acronym.

> > +Object path	[variable prefix]/modemX/message_id
> 
> Please follow the conventions for Object Path, e.g. see
> doc/voicecall-api.txt

Fixed

> > +
> > +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 TXState
> > +
> > +			Current transmission state.
> > +
> 
> Rename this to State and include the possible state values please.

Done

Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface

[Denis Kenzior]

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

Hi Inaky,

>>> +SMS / Messaging interface
>>> +=========================
>>> +
>>> +Service		org.ofono
>>> +Interface	org.ofono.SMSMessage
>>
>> oFono API is full CamelCase, so this should be SmsMessage.
> 
> I disagree. It would be like that if SMS was a word, but it is an
> acronym. It makes it quite confusing, if you ask me. If that's still how
> you like it, I'll change it, but I think it is wrong to capitalize an
> acronym.
> 

Your objection has been noted.  However, pure camel case looks nicer and
that is how most of the API already is (e.g. SmsManager, SimManager,
UnlockPin, etc)  So please change it or name it simply 'Message'.

Regards,
-Denis

Re: [patch 12/20] struct tx_queue_entry: add a destructor

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 16:14 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> >> tx_queue_entry * here.  Casting the destructor in g_foo_foreach is
> >> acceptable.
> > 
> > Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts
> > are evil and should be avoided for they muck the code by hiding warnings
> > when you change things and miss updating all the sites ... 
> > 
> 
> I fully agree, but the extra silly NULL argument makes this worse than
> the cast.  And to be clear, g_foo_foreach is about the only place we
> explicitly allow casts.  If you feel you need to be pedantic, then write
> a GFunc specifically to do the foreach bit.
> 
> e.g. tx_queue_entry_free(struct tx_queue_entry *);
> 
> txq_foreach_free(gpointer foo, gpointer user)
> {
>   struct tx_queue_entry *bar = foo;
>   tx_queue_entry_free(bar);
> }
> 

Done

Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface

[Inaky Perez-Gonzalez]

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

On Mon, 2010-07-26 at 11:05 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> >>> +SMS / Messaging interface
> >>> +=========================
> >>> +
> >>> +Service		org.ofono
> >>> +Interface	org.ofono.SMSMessage
> >>
> >> oFono API is full CamelCase, so this should be SmsMessage.
> > 
> > I disagree. It would be like that if SMS was a word, but it is an
> > acronym. It makes it quite confusing, if you ask me. If that's still how
> > you like it, I'll change it, but I think it is wrong to capitalize an
> > acronym.
> > 
> 
> Your objection has been noted.  However, pure camel case looks nicer and
> that is how most of the API already is (e.g. SmsManager, SimManager,
> UnlockPin, etc)  So please change it or name it simply 'Message'.

Changed to SmsMessage as discussed over IRC.

Re: [patch 12/20] struct tx_queue_entry: add a destructor

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 16:06 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +
> > +/*
> > + * 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_free(gpointer _entry, gpointer unused)
> 
> Can't this be simply tx_queue_entry_free?  And feel free to take struct
> tx_queue_entry * here.  Casting the destructor in g_foo_foreach is
> acceptable.

Renamed to tx_queue_entry_destroy() -- kept destroy vs free, as it has a
clearer meaning as to what the function does.

> > +{
> > +	struct tx_queue_entry *entry = _entry;
> > +
> > +	if (entry->destroy)
> > +		entry->destroy(entry->data);
> 
> An empty line after if/while/for blocks please.  I will update the
> coding-style document with this rule shortly.

Done.

Re: [patch 11/20] SMS: rename create_tx_queue_entry() to tx_queue_entry_new()

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-23 at 16:02 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote:
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > This is for symmetry with tx_queue_entry_free()
> > ---
> >  src/sms.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> 
> Applied with a minor tweak to the commit header.

Please *don't* do that; remember the minor extra work it causes you
multiplies by ten for the person that has to sort out the conflict in
his git trees and figure out the kind of change you did.

Re: [patch 04/20] SMS: introduce message ID API

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 03:59 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.
> 
> Note this adds missing #includes to src/smsutil.h; header files should
> included from any other .c or .h file.
> ---
>  Makefile.am            |    5 +-
>  src/smsutil.c          |  191 +++++++++++++++++++++++++++++++++++++++++++
>  src/smsutil.h          |   87 ++++++++++++++++++++
>  unit/test-sms-msg-id.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++

Any time you change files in multiple directories, send multiple patches
for each directory.  E.g. src/smsutil.[ch] in patch 1, Makefile.am and
unit/test-sms-msg-id.c in patch 2.

>  4 files changed, 494 insertions(+), 1 deletions(-)
>  create mode 100644 unit/test-sms-msg-id.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index e256841..5a371c2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -361,7 +361,7 @@ unit_objects =
>  noinst_PROGRAMS = unit/test-common unit/test-util unit/test-idmap \
>  					unit/test-sms unit/test-simutil \
>  					unit/test-mux unit/test-caif \
> -					unit/test-stkutil
> +					unit/test-stkutil unit/test-sms-msg-id
>  
>  unit_test_common_SOURCES = unit/test-common.c src/common.c
>  unit_test_common_LDADD = @GLIB_LIBS@
> @@ -400,6 +400,9 @@ unit_test_caif_SOURCES = unit/test-caif.c $(gatchat_sources) \
>  unit_test_caif_LDADD = @GLIB_LIBS@
>  unit_objects += $(unit_test_caif_OBJECTS)
>  
> +unit_test_sms_msg_id_SOURCES = unit/test-sms-msg-id.c src/util.c src/smsutil.c src/storage.c
> +unit_test_sms_msg_id_LDADD = @GLIB_LIBS@
> +
>  noinst_PROGRAMS += gatchat/gsmdial gatchat/test-server gatchat/test-qcdm
>  
>  gatchat_gsmdial_SOURCES = gatchat/gsmdial.c $(gatchat_sources)
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 6c2087a..f93ea5b 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -3982,3 +3982,194 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)
>  
>  	return utf8;
>  }
> +
> +static
> +void sms_log_critical(const gchar *format, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, format);
> +	g_logv("SMS-MSG-ID", G_LOG_LEVEL_CRITICAL | G_LOG_FLAG_FATAL,
> +	       format, args);
> +	va_end(args);
> +}
> +

Please refrain from doing this inside utility code.  There should be no
printf / logs in smsutil.c

> +const GChecksumType SMS_MSG_ID_CHECKSUM = G_CHECKSUM_SHA256;
> +
> +
> +/*
> + * Internal function (also used for aiding in unit testing, hence not static)
> + */
> +gsize __sms_msg_id_hash_length(void)
> +{
> +	return g_checksum_type_get_length(SMS_MSG_ID_CHECKSUM);
> +}
> +
> +

What is up with the double empty lines?

> +/**
> + * Initialize and reset a UUID's state
> + *
> + * \param msg_id Descriptor
> + */
> +void sms_msg_id_init(struct sms_msg_id *msg_id)
> +{
> +	memset(msg_id->id, 0, sizeof(msg_id->id));
> +	memset(&msg_id->checksum, 0, sizeof(msg_id->checksum));
> +}
> +
> +
> +/**
> + * Create a random UUID
> + *
> + * \param msg_id Descriptor
> + *
> + * \internal
> + *
> + * Crams the msg_id ID buffer with random 32-bit numbers; because we
> + * use a byte based buffer, we play the cast to guint32 trick (as that
> + * is what g_random_int() returns).
> + */
> +void sms_msg_id_random(struct sms_msg_id *msg_id)
> +{
> +	guint32 *b;
> +	unsigned cnt, top;
> +
> +	/* Fail building if the size of the ID block is not a full
> +	 * multiple of a guint32's size. */
> +	BUILD_BUG_ON(sizeof(msg_id->id) % sizeof(b[0]) != 0);
> +
> +	top = sizeof(msg_id->id) / sizeof(b[0]);
> +	b = (void *) msg_id->id;
> +	for (cnt = 0; cnt < top; cnt++)
> +		b[cnt] = g_random_int();
> +	/* mark hash is initialized */
> +	msg_id->checksum = (void *) ~0;
> +}

I really question the utility of this function.  We should always be
able to generate a message id based on the contents.

> +
> +
> +/**
> + * Feed data to the hash for generation of a UUID
> + *
> + * Provides a data buffer to be added to the hash from which a UUID
> + * will be generated. When done providing buffers, call with a %NULL
> + * buffer to get the hash finalized and the UUID.
> + *
> + * \param msg_id Descriptor
> + * \param buf Pointer to data buffer. Call with %NULL to finalize the
> + *     hash and generate the UUID. Note that after calling with %NULL
> + *     you cannot call again with another buffer to generate another
> + *     UUID without calling sms_msg_id_init() first.
> + * \param buf_size size of the @buf buffer.
> + *
> + * NOTE:
> + *  - Will abort on OOM with an error message
> + *  - Will abort if called with %NULL as an argument to \param buf
> + *    without any non-NULL buffers were fed to in previous calls.
> + *  - In case of going over an error path before closing the UUID with
> + *    sms_msg_id_hash(%NULL), the hash has to be closed in order to
> + *    avoid memory leaks -- thus call sms_msg_id_hash(%NULL) in the
> + *    error path.
> + */
> +void sms_msg_id_hash(struct sms_msg_id *msg_id,
> +		     const void *buf, size_t buf_size)
> +{
> +	gsize digest_size = __sms_msg_id_hash_length();
> +	unsigned char digest[digest_size];
> +
> +	if (msg_id->checksum == (void *) ~0) {
> +		sms_log_critical("%s:%d: SW BUG: %s(!NULL) called "
> +				 "on a closed hash\n",
> +				 __FILE__, __LINE__, __func__);
> +		return;
> +	}
> +	if (buf == NULL) {
> +		if (msg_id->checksum == NULL) {
> +			sms_log_critical("%s:%d: BUG: %s(NULL) called "
> +					 "without feeding data first\n",
> +					 __FILE__, __LINE__, __func__);
> +			return;
> +		}
> +		g_checksum_get_digest(msg_id->checksum,
> +				      digest, &digest_size);
> +		memcpy(msg_id->id, digest,
> +		       MIN(digest_size, sizeof(msg_id->id)));
> +		if (digest_size < sizeof(msg_id->id))
> +			memset(msg_id->id + digest_size, 0,
> +			       sizeof(msg_id->id) - digest_size);
> +		g_checksum_free(msg_id->checksum);
> +		/* mark hash is initialized */
> +		msg_id->checksum = (void *) ~0;
> +	} else {
> +		/* Word has it g_checksum_new(), which uses
> +		 * g_slice_alloc() will abort() on allocation
> +		 * failure. However, is not really documented
> +		 * anywhere, so the extra check -- and we just abort
> +		 * on OOM. */
> +		if (msg_id->checksum == NULL) {
> +			msg_id->checksum = g_checksum_new(SMS_MSG_ID_CHECKSUM);
> +			if (msg_id->checksum == NULL)
> +				sms_log_critical("%s:%d: OOM: can't allocate "
> +						 "checksum",
> +						 __FILE__, __LINE__);
> +		}
> +		g_checksum_update(msg_id->checksum, buf, buf_size);
> +	}
> +}
> +
> +
> +/**
> + * Print a UUID to a string buffer
> + *
> + * Given a closed UUID, print it as a hexadecimal string to a provided
> + * buffer display (lowest nibble right).
> + *
> + * \param msg_id Descriptor
> + * \param strbuf String where to format the UUID
> + * \param strbuf_size Size of the string buffer.
> + *
> + * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or
> + *      sms_msg_id_hash(NULL) were called on them.
> + */
> +void sms_msg_id_printf(const struct sms_msg_id *msg_id,
> +		       char *strbuf, size_t strbuf_size)
> +{
> +	int cnt;
> +	size_t written = 0;
> +
> +	if (msg_id->checksum != (void *) ~0) {
> +		sms_log_critical("%s:%d: BUG: %s() called on an open hash\n",
> +				 __FILE__, __LINE__, __func__);
> +		return;
> +	}
> +	for (cnt = 0; cnt < SMS_MSG_ID_HASH_SIZE; cnt++)
> +		written += snprintf(strbuf + written, strbuf_size - written,
> +				    "%02x", msg_id->id[cnt] & 0xff);
> +}
> +
> +
> +/**
> + * Print a UUID to a string buffer
> + *
> + * Given a closed UUID, print it as a hexadecimal string to a provided
> + * buffer display (lowest nibble right).
> + *
> + * \param msg_id Descriptor
> + * \param strbuf String where to format the UUID
> + * \param strbuf_size Size of the string buffer.
> + *
> + * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or
> + *      sms_msg_id_hash(NULL) were called on them.
> + */
> +void sms_msg_id_memcpy(void *buf, size_t buf_size,
> +		       const struct sms_msg_id *msg_id)
> +{
> +	size_t copy_size = MIN(buf_size, sizeof(msg_id->id));
> +	if (msg_id->checksum != (void *) ~0) {
> +		sms_log_critical("%s:%d: BUG: %s() called on an open hash\n",
> +				 __FILE__, __LINE__, __func__);
> +		return;
> +	}
> +	memmove(buf, msg_id->id, MIN(buf_size, sizeof(msg_id->id)));
> +	if (copy_size < buf_size)
> +		memset(buf + copy_size, 0, buf_size - copy_size);
> +}

Exposing all this API as public leads to lots of totally unnecessary
code.  The philosophy should be the opposite, e.g. exposing as little as
possible to the end user.

The API needs to be shrunk down to maybe 2-3 simple functions and most
of the extra pedantic checks removed.

> diff --git a/src/smsutil.h b/src/smsutil.h
> index 66ef6f8..fbe3c72 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -19,6 +19,12 @@
>   *
>   */
>  
> +#include <time.h>
> +#include <glib/gtypes.h>
> +#include <glib/gslist.h>
> +#include <glib/ghash.h>
> +#include <glib/gchecksum.h>
> +
>  #define CBS_MAX_GSM_CHARS 93
>  
>  enum sms_type {
> @@ -539,3 +545,84 @@ 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);
> +
> +enum {
> +	/* Note this has to be a multiple of sizeof(guint32);
> +	 * Compilation will assert-fail if this is not met. */
> +	SMS_MSG_ID_HASH_SIZE = 16,
> +};
> +
> +/**
> + * Unique identifiers
> + *
> + * Generate unique identifiers to map data. Can be initialized with a
> + * random identifier or by creating a hash based on data contents.
> + *
> + * \NOTE Never access the struct's contents directly; use the
> + *     sms_msg_id*() API
> + *
> + * Usage
> + *
> + * Declare and initialize:
> + *
> + * \code
> + *   struct sms_msg_id msg_id;
> + *
> + *   sms_msg_id_init(&msg_id);	// initialize to known state
> + * \endcode
> + *
> + * Initialize with a random ID
> + *
> + * \code
> + *   sms_msg_id_random(&msg_id);	// Create a random unique ID
> + * \endcode
> + *
> + * or initialize by hashing data buffers:
> + *
> + * \code
> + *   sms_msg_id_hash(&msg_id, buf1, buf1_size);
> + *   sms_msg_id_hash(&msg_id, buf2, buf2_size);
> + *   sms_msg_id_hash(&msg_id, buf3, buf3_size);
> + *   ...
> + *   sms_msg_id_hash(&msg_id, NULL, 0);
> + * \endcode
> + *
> + * Once the hash has been initialized, it can be accessed. Never
> + * access it before initializing the hash (it will trigger a software
> + * error and abort).
> + *
> + * Formating for printing:
> + *
> + * \code
> + *   DECLARE_SMS_MSG_ID_STRBUF(buf);
> + *   ...
> + *   sms_msg_id_printf(&msg_id, buf, sizeof(buf));
> + * \endcode
> + *
> + * Copying the hash data to a buffer
> + *
> + * \code
> + *   sms_msg_id_memcpy(buf, buf_size, &msg_id);
> + * \endcode
> + *
> + * For generating another ID (with sms_msg_id_random() or
> + * sms_msg_id_hash()), sms_msg_id_init() has to be called.
> + */
> +
> +struct sms_msg_id {
> +	guint8 id[SMS_MSG_ID_HASH_SIZE];
> +	GChecksum *checksum;
> +};
> +
> +#define DECLARE_SMS_MSG_ID_STRBUF(strbuf)	\
> +	char strbuf[SMS_MSG_ID_HASH_SIZE * 2 + 1]
> +
> +void sms_msg_id_init(struct sms_msg_id *);
> +void sms_msg_id_random(struct sms_msg_id *);
> +void sms_msg_id_hash(struct sms_msg_id *, const void *, size_t);
> +
> +void sms_msg_id_printf(const struct sms_msg_id *, char *, size_t);
> +void sms_msg_id_memcpy(void *, size_t, const struct sms_msg_id *);
> +
> +/* for unit testing only */
> +gsize __sms_msg_id_hash_length(void);

I think this is just way too complicated ...

I suggest something like:

struct sms_uuid {
	uint8_t id[hash_size];
};

// oFono is not multithreaded, so static return is fine
const char *sms_uuid_to_string(const struct ofono_uuid *uuid);
int sms_uuid_from_sms_list(GSList *list,
				struct ofono_uuid *out_uuid);

Regards,
-Denis

Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Denis Kenzior]

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

Hi Inaky,

> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> -					unsigned int flags,
> +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);

I disagree with this, you're modifying an ofono internal API function to
return a structure which is opaque and cannot be manipulated.  I'd
rather see you returning a struct sms_uuid...

> +	/*
> +	 * Instead of using the telephone number/address we got from
> +	 * D-Bus, we do the reverse formatting, so we get something
> +	 * that has been normalized--this is used later and we do it
> +	 * here to simplify error handling.
> +	 */
> +	sms_address_from_string(&receiver, to);
> +	if (sms_address_to_hex_string(&receiver, receiver_str)
> +	    == FALSE)
> +		return __ofono_error_failed(msg);

Please avoid mixing tabs and spaces for indentation.  And in this case
it is more readale to break up the function than the equals line.  E.g.

if (sms_address_to_hex_string(&receiver,
				receiver_str == FALSE)
	return __ofono_error_failed(msg);

Regards,
-Denis

Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'

[Denis Kenzior]

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

Hi Inaky,

> +/*
> + * Encapsulate information needed to export an SMS message over D-Bus.
> + *
> + * @dbus_path: path of the object in the D-Bus
> + * @dbus_msg: message this originated at
> + */
> +struct sms_msg_dbus_data {
> +	char *dbus_path;
> +	DBusMessage *dbus_msg;
> +};
> +

I don't really see a point for this structure.  When you send an SMS,
assuming the arguments and format were valid, you return straight away.
 Thus SendMessage should be marked as not being ASYNC and storing of
DBusMessage is unnecessary.  Rest can be handled by txq_submit and the
dbus path can be easily generated based on the UUID.

>  /*
> @@ -590,6 +604,9 @@ static void send_message_destroy(void *data)
>   * 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 DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  					void *data)

This chunk seems totally unrelated...

Regards,
-Denis

Re: [patch 19/20] SMS: introduce sms_msg_cancel and its D-Bus wrapper

[Denis Kenzior]

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

Hi Inaky,

On 07/23/2010 04:00 PM, Inaky Perez-Gonzalez wrote:
> 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                |   63 +++++++++++++++++
>  src/smsutil.h            |   15 ++++
>  test/test-sms-msg-cancel |  173 ++++++++++++++++++++++++++++++++++++++++++++++

Multiple patches please.  One for each directory.

>  3 files changed, 251 insertions(+), 0 deletions(-)
>  create mode 100755 test/test-sms-msg-cancel
> 
> diff --git a/src/sms.c b/src/sms.c
> index c0b2e00..97c9eab 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -538,6 +538,24 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *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)
> +{
> +	gboolean do_network_cancel = FALSE;
> +	struct tx_queue_entry *sms_msg = data;
> +	if (!dbus_message_get_args(msg, NULL,
> +				   DBUS_TYPE_BOOLEAN, &do_network_cancel))
> +		return __ofono_error_invalid_args(msg);
> +	sms_msg_cancel(sms_msg,
> +		       do_network_cancel ? SMS_MSG_CANCEL_IN_NETWORK : 0);
> +	return dbus_message_new_method_return(msg);
> +}
> +
> +

You're ignoring about every part of the coding standard in this chunk...

> +/**
> + * Cancel a pending SMS message
> + *
> + * @sms_msg: message to cancel
> + * @flags: modifies for the action
> + *
> + * 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.
> + */
> +void sms_msg_cancel(struct tx_queue_entry *sms_msg,
> +		    enum sms_msg_cancel_flags flags)
> +{
> +	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);
> +	if (flags & SMS_MSG_CANCEL_IN_NETWORK)
> +		ofono_warn("%s(): SMS_MSG_CANCEL_IN_NETWORK support "
> +			   "not yet implemented\n", __func__);
> +	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELLED);
> +	tx_queue_entry_destroy_free(sms_msg, NULL);
> +}
> +
> +

And again ignoring the coding standard...

Regards,
-Denis

Re: [patch 20/20] SMS: Implement D-Bus SMS-MSG::GetProperties

[Denis Kenzior]

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

Hi Inaky,

> +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;

Empty line here

> +	dbus_message_iter_init_append(reply, &iter);
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					 OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					 &dict);

Empty line here

> +	str = ofono_sms_tx_state_to_string(sms_msg->state);
> +	ofono_dbus_dict_append(&dict, "TXState", DBUS_TYPE_STRING, &str);

Empty line here
> +	dbus_message_iter_close_container(&iter, &dict);

And one more here

Also, you might want to think about what other properties would be
useful here.  E.g. perhaps queue time, submission time, To address,
contents?

> +	return reply;
> +}
> +
> +

No double empty lines allowed.

>  /*
>   * Note that the D-Bus specific cleanups are taken care by the
>   * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send().
> @@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(
>  
>  static
>  GDBusMethodTable sms_msg_methods[] = {
> +	{ "GetProperties",	"",	"a{sv}",
> +	  dbus_sms_msg_get_properties, G_DBUS_METHOD_FLAG_ASYNC },

Why exactly is this method ASYNC?

>  	{ "Cancel",		DBUS_TYPE_BOOLEAN_AS_STRING,	"",
>  	  dbus_sms_msg_cancel, 0 },
>  	{ }

Regards,
-Denis

Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

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

On Tue, 2010-07-27 at 10:03 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> > -					unsigned int flags,
> > +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);
> 
> I disagree with this, you're modifying an ofono internal API function to
> return a structure which is opaque and cannot be manipulated.  I'd
> rather see you returning a struct sms_uuid...

I need a handle to operate on. The D-Bus wrappers need something they
can use to dispose of the structure (call tx_queue_entry_destroy() on
the error path) and to pass to sms_msg_cancel(). The structure is not
manipulated (except for accessing the UUID and #PDUs, which should
probably be coded out to a helper).

This could be solved by having at the entry point of those functions or
wrappers of a lookup function that looks for the UUID in the list of
messages, but that would be adding more code and wasting time for no
benefit. I disagree on adding unnecessary steps.

> > +	/*
> > +	 * Instead of using the telephone number/address we got from
> > +	 * D-Bus, we do the reverse formatting, so we get something
> > +	 * that has been normalized--this is used later and we do it
> > +	 * here to simplify error handling.
> > +	 */
> > +	sms_address_from_string(&receiver, to);
> > +	if (sms_address_to_hex_string(&receiver, receiver_str)
> > +	    == FALSE)
> > +		return __ofono_error_failed(msg);
> 
> Please avoid mixing tabs and spaces for indentation.  And in this case
> it is more readale to break up the function than the equals line.  E.g.
> 
> if (sms_address_to_hex_string(&receiver,
> 				receiver_str == FALSE)
> 	return __ofono_error_failed(msg);

This is truly a matter of personal opinion; I for one cannot see how
this is more readable than the other form :)

Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Denis Kenzior]

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

On 07/29/2010 04:26 PM, Inaky Perez-Gonzalez wrote:
> On Tue, 2010-07-27 at 10:03 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>>> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>>> -					unsigned int flags,
>>> +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);
>>
>> I disagree with this, you're modifying an ofono internal API function to
>> return a structure which is opaque and cannot be manipulated.  I'd
>> rather see you returning a struct sms_uuid...
> 
> I need a handle to operate on. The D-Bus wrappers need something they
> can use to dispose of the structure (call tx_queue_entry_destroy() on
> the error path) and to pass to sms_msg_cancel(). The structure is not
> manipulated (except for accessing the UUID and #PDUs, which should
> probably be coded out to a helper).
> 
> This could be solved by having at the entry point of those functions or
> wrappers of a lookup function that looks for the UUID in the list of
> messages, but that would be adding more code and wasting time for no
> benefit. I disagree on adding unnecessary steps.
> 

I need to look at this in detail.  Can you simply grab the tx_entry from
the back of the queue for now?  I'm really against modifying txq_submit
right now.

>>> +	/*
>>> +	 * Instead of using the telephone number/address we got from
>>> +	 * D-Bus, we do the reverse formatting, so we get something
>>> +	 * that has been normalized--this is used later and we do it
>>> +	 * here to simplify error handling.
>>> +	 */
>>> +	sms_address_from_string(&receiver, to);
>>> +	if (sms_address_to_hex_string(&receiver, receiver_str)
>>> +	    == FALSE)
>>> +		return __ofono_error_failed(msg);
>>
>> Please avoid mixing tabs and spaces for indentation.  And in this case
>> it is more readale to break up the function than the equals line.  E.g.
>>
>> if (sms_address_to_hex_string(&receiver,
>> 				receiver_str == FALSE)
>> 	return __ofono_error_failed(msg);
> 
> This is truly a matter of personal opinion; I for one cannot see how
> this is more readable than the other form :)
> 

All coding styles are a matter of personal taste.  For oFono we prefer
the above mentioned style.

Regards,
-Denis

Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'

[Inaky Perez-Gonzalez]

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

On Tue, 2010-07-27 at 10:08 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +/*
> > + * Encapsulate information needed to export an SMS message over D-Bus.
> > + *
> > + * @dbus_path: path of the object in the D-Bus
> > + * @dbus_msg: message this originated at
> > + */
> > +struct sms_msg_dbus_data {
> > +	char *dbus_path;
> > +	DBusMessage *dbus_msg;
> > +};
> > +
> 
> I don't really see a point for this structure.  When you send an SMS,
> assuming the arguments and format were valid, you return straight away.
>  Thus SendMessage should be marked as not being ASYNC and storing of
> DBusMessage is unnecessary.  Rest can be handled by txq_submit and the
> dbus path can be easily generated based on the UUID.

First it serves as transitional storage -- later in the commit sequences
the dbus_msg is removed as it is, as you say, unneeded once all the code
is made sync. If at the end only one member is left (dbus_msg)

In our discussions to plan this stuff you said you wanted the D-Bus
unique name to contain the UUID and the number of PDUs
(/modemX/UUID_PDUs). That means I need to access the SMS object if I
want to generate the name on the fly. I need a backpointer for that.

Even if we remove the _PDUs requirement:

To generate on the fly whenever is needed we need to have access to who
is the 'parent' in:

- the destructor function, which is not going to be easy

- the PropertyChanged signal generator

which means that instead of storing a name I have to store, for example
the 'struct ofono_sms' that is the parent, get its name, figure out how
to pull the name of the SMS object from a UUID *and* then generate a
name *and* do the actual work.

That is not effective and adds more code. Generating the name once and
storing it is the simplest solution. If you want it out of the structure
that encapsulates it, well I can do that as a final follow up patch once
dbus_msg is no longer needed. It's syntactic sugar to enforce
encapsulation.

> >  /*
> > @@ -590,6 +604,9 @@ static void send_message_destroy(void *data)
> >   * 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 DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  					void *data)
> 
> This chunk seems totally unrelated...

Yep, it is -- this should be a separate doc commit. Thanks for catching
it.

Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'

[Denis Kenzior]

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

Hi Inaky,

On 07/29/2010 04:47 PM, Inaky Perez-Gonzalez wrote:
> On Tue, 2010-07-27 at 10:08 -0700, Denis Kenzior wrote:
>> Hi Inaky,
>> 
>>> +/* + * Encapsulate information needed to export an SMS message 
>>> over D-Bus. + * + * @dbus_path: path of the object in the D-Bus
>>> + * @dbus_msg: message this originated at + */ +struct 
>>> sms_msg_dbus_data { +	char *dbus_path; +	DBusMessage *dbus_msg; 
>>> +}; +
>> 
>> I don't really see a point for this structure.  When you send an 
>> SMS, assuming the arguments and format were valid, you return 
>> straight away. Thus SendMessage should be marked as not being
>> ASYNC and storing of DBusMessage is unnecessary.  Rest can be
>> handled by txq_submit and the dbus path can be easily generated
>> based on the UUID.
> 
> First it serves as transitional storage -- later in the commit 
> sequences the dbus_msg is removed as it is, as you say, unneeded
> once all the code is made sync. If at the end only one member is
> left (dbus_msg)

Don't get me wrong, if a structure is necessary in the end, so be it.
However, during the review I only see a structure being introduced that
is totally unnecessary.  You can simply pass dbus_path around as
userdata.

If/when this structure does become necessary, feel free to
introduce it.

> 
> In our discussions to plan this stuff you said you wanted the D-Bus 
> unique name to contain the UUID and the number of PDUs 
> (/modemX/UUID_PDUs). That means I need to access the SMS object if I
>  want to generate the name on the fly. I need a backpointer for
> that.
> 
> Even if we remove the _PDUs requirement:

There is no such requirement.  If I gave you this idea, then I must have
not been clear enough.  I meant that PDUs would be used to generate the
UUID.

Regards,
-Denis

Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'

[Inaky Perez-Gonzalez]

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

On Thu, 2010-07-29 at 15:17 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/29/2010 04:47 PM, Inaky Perez-Gonzalez wrote:
> > On Tue, 2010-07-27 at 10:08 -0700, Denis Kenzior wrote:
> >> Hi Inaky,
> >> 
> >>> +/* + * Encapsulate information needed to export an SMS message 
> >>> over D-Bus. + * + * @dbus_path: path of the object in the D-Bus
> >>> + * @dbus_msg: message this originated at + */ +struct 
> >>> sms_msg_dbus_data { +	char *dbus_path; +	DBusMessage *dbus_msg; 
> >>> +}; +
> >> 
> >> I don't really see a point for this structure.  When you send an 
> >> SMS, assuming the arguments and format were valid, you return 
> >> straight away. Thus SendMessage should be marked as not being
> >> ASYNC and storing of DBusMessage is unnecessary.  Rest can be
> >> handled by txq_submit and the dbus path can be easily generated
> >> based on the UUID.
> > 
> > First it serves as transitional storage -- later in the commit 
> > sequences the dbus_msg is removed as it is, as you say, unneeded
> > once all the code is made sync. If at the end only one member is
> > left (dbus_msg)
> 
> Don't get me wrong, if a structure is necessary in the end, so be it.
> However, during the review I only see a structure being introduced that
> is totally unnecessary.  You can simply pass dbus_path around as
> userdata.

Agreed -- at the end, once everything is cleaned up only dbus_path is
necessary, and I agree it could be passed instead of having the struct.
However, for all the intermediate commits not to break (and thus break
bisectability), having that there is a must. 

However, with so much reorganization in the code that has happened
during this review process, it is probably possible to do with out it.
I'll re-audit all that to see how it can be killed.

> If/when this structure does become necessary, feel free to
> introduce it.
> 
> > 
> > In our discussions to plan this stuff you said you wanted the D-Bus 
> > unique name to contain the UUID and the number of PDUs 
> > (/modemX/UUID_PDUs). That means I need to access the SMS object if I
> >  want to generate the name on the fly. I need a backpointer for
> > that.
> > 
> > Even if we remove the _PDUs requirement:
> 
> There is no such requirement.  If I gave you this idea, then I must have
> not been clear enough.  I meant that PDUs would be used to generate the
> UUID.

That simplifies things further, thanks.

Re: [patch 19/20] SMS: introduce sms_msg_cancel and its D-Bus wrapper

[Inaky Perez-Gonzalez]

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

On Tue, 2010-07-27 at 10:16 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/23/2010 04:00 PM, Inaky Perez-Gonzalez wrote:
> > 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                |   63 +++++++++++++++++
> >  src/smsutil.h            |   15 ++++
> >  test/test-sms-msg-cancel |  173 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> Multiple patches please.  One for each directory.
> ...
>

> You're ignoring about every part of the coding standard in this chunk...
> 
> ...

> And again ignoring the coding standard...

All feedback for this fixed, thanks.

Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

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

On Thu, 2010-07-29 at 14:37 -0700, Denis Kenzior wrote: 
> On 07/29/2010 04:26 PM, Inaky Perez-Gonzalez wrote:
> > On Tue, 2010-07-27 at 10:03 -0700, Denis Kenzior wrote: 
> >> Hi Inaky,
> >>
> >>> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> >>> -					unsigned int flags,
> >>> +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);
> >>
> >> I disagree with this, you're modifying an ofono internal API function to
> >> return a structure which is opaque and cannot be manipulated.  I'd
> >> rather see you returning a struct sms_uuid...
> > 
> > I need a handle to operate on. The D-Bus wrappers need something they
> > can use to dispose of the structure (call tx_queue_entry_destroy() on
> > the error path) and to pass to sms_msg_cancel(). The structure is not
> > manipulated (except for accessing the UUID and #PDUs, which should
> > probably be coded out to a helper).
> > 
> > This could be solved by having at the entry point of those functions or
> > wrappers of a lookup function that looks for the UUID in the list of
> > messages, but that would be adding more code and wasting time for no
> > benefit. I disagree on adding unnecessary steps.
> > 
> 
> I need to look at this in detail.  Can you simply grab the tx_entry from
> the back of the queue for now?

Yes, that could be done, but it is way too dirty. Assumes *a lot* of
knowledge about internal implementation details. A no-no.

>   I'm really against modifying txq_submit right now.

In any case it has to be modified to add the state change callback. 

It has only *one* user asides from the SMS code and the changes don't
impact said user negatively at all, so it shouldn't be such a big deal.

Re: [patch 20/20] SMS: Implement D-Bus SMS-MSG::GetProperties

[Inaky Perez-Gonzalez]

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

On Tue, 2010-07-27 at 10:18 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +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;
> 
> Empty line here
... 
> Empty line here
... 
> Empty line here
... 
> And one more here

Done

> Also, you might want to think about what other properties would be
> useful here.  E.g. perhaps queue time, submission time, To address,
> contents?

None are needed so far

Contents will be quite messy, as there is only binary encoded data at
this point and we might not even know how to decode it (let's say a
binary list of PDUs vs a text message).

Other immediate ones are submission and delivery time, which also means
we'd have to have a (property changed) signal for them and that'd
require adding another callback in the core code [I am talking
_txq_submit()]. I am going to leave it out for now because I don't want
to add more complexity -- it's easy to add them later, but will require
more changes.

> No double empty lines allowed.

fixed

> >  /*
> >   * Note that the D-Bus specific cleanups are taken care by the
> >   * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send().
> > @@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(
> >  
> >  static
> >  GDBusMethodTable sms_msg_methods[] = {
> > +	{ "GetProperties",	"",	"a{sv}",
> > +	  dbus_sms_msg_get_properties, G_DBUS_METHOD_FLAG_ASYNC },
> 
> Why exactly is this method ASYNC?

Hmm, that was cut & paste from the manager's get_properties and it is
obviously wrong -- fixed thanks