[SMS D-Bus 00/23] Exports SMS over D-Bus and mis cleanups

[SMS D-Bus 00/23] Exports SMS over D-Bus and mis cleanups

[Inaky Perez-Gonzalez]

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

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

Hi All

This patchset is the current state of my tree that changes the D-Bus
interface for SMS:

 - adds object based management of SMS messages

 - adds a cancelation operation for in-transit / pending messages

 - holds messages waiting for acknoledgement (delivery report) -- this
   is still not fully integrated with the code that was commited last
   days (thus once the delivery arrives the message is not
   automatically cleaned up as "confirmed").

 - generates truly unique SMS message IDs using hashing of contents
   and receiver address

 - miscelaeous small cleanups / additions, carryover from a previous
   submit that got neither not acked or no resolution was agreed upon

Please review and suggest what else needs to be done / changed.

Thx,


The following changes since commit 1fedd096a0ba2ce8625a9e4d1c2ce25bb8f6dfe4:
  Marcel Holtmann (1):
        Check sanity the MNC length value from the SIM card

are available in the git repository at:

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

Patches follow for reviewing convenience.

Inaky Perez-Gonzalez (23):
      documentation: add note about referencing standards
      util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
      smutil.h: add missing header file dependencies
      write_file: make transaction-safe
      doc: explain debugging options to -d, add a pointer in -h to manpage
      SMS: introduce message ID API
      introduce DECLARE_SMS_ADDR_STR()
      export sms_assembly_encode_address
      SMS: implement SHA256-based message IDs [incomplete]
      sms: add doc about the extensions D-Bus API (not yet implemented)
      struct tx_queue_entry: add fields and destructor
      SMS: produce a unique, persistent name for in-transit messages
      SMS: introduce bare state machine and transitions
      SMS: export outgoing messages over D-Bus (skeleton)
      SMS: split sms_send_message() into a D-Bus front end and an internal API
      SMS: introduce wait-for-ack state and infrastructure
      SMS: introduce sms_msg_cancel and its D-Bus wrapper
      SMS: rename tx_queue_entry->msg to ->dbus_msg for clarity
      SMS: Implement D-Bus SMS-MSG::GetProperties
      SMS: send D-Bus SMS-MSG::ProperyChanged signals when message changes status
      SMS: make D-Bus SendMessage and Cancel fully synchronous
      SMS: set the SRR bit in outgoing PDUs if WFA is requested
      sms_text_prepare: document @use_delivery_reports

 HACKING                        |   10 +
 Makefile.am                    |    5 +-
 doc/ofonod.8                   |    5 +-
 doc/sms-api.txt                |   94 ++++++++
 doc/standards.txt              |    8 +
 src/main.c                     |    4 +-
 src/sms.c                      |  512 +++++++++++++++++++++++++++++++++++-----
 src/smsutil.c                  |  202 ++++++++++++++++-
 src/smsutil.h                  |  125 ++++++++++
 src/storage.c                  |   42 +++-
 src/util.h                     |   28 +++
 test/test-sms-msg-state-change |   24 ++
 unit/test-sms-msg-id.c         |  212 +++++++++++++++++
 13 files changed, 1199 insertions(+), 72 deletions(-)
 create mode 100644 doc/standards.txt
 create mode 100755 test/test-sms-msg-state-change
 create mode 100644 unit/test-sms-msg-id.c

[SMS D-Bus 01/23] documentation: add note about referencing standards

[Inaky Perez-Gonzalez]

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

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

---
 doc/standards.txt |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
 create mode 100644 doc/standards.txt

diff --git a/doc/standards.txt b/doc/standards.txt
new file mode 100644
index 0000000..c4b68eb
--- /dev/null
+++ b/doc/standards.txt
@@ -0,0 +1,8 @@
+Referencing standards in the source
+===================================
+
+When referencing standard documents use raw numbers xx.xxx for 3GPP
+documents or xxx.xxx for ETSI document (eg: 23.040). If needing to
+point to an specific section/subsection, explicitly say "Section foo"
+
+3GPP specs can be found in http://3gpp.org/ftp/Specs.
-- 
1.6.6.1

[SMS D-Bus 02/23] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1742 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/util.h |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/src/util.h b/src/util.h
index 9da81aa..b972120 100644
--- a/src/util.h
+++ b/src/util.h
@@ -80,3 +80,31 @@ char *sim_string_to_utf8(const unsigned char *buffer, int length);
 
 unsigned char *utf8_to_sim_string(const char *utf,
 					int max_length, int *out_length);
+/*
+ * Build time consistency checks
+ *
+ * Stolen from the Linux kernel 2.6.35-rc; as most taken from the
+ * Linux kernel, this is licensed GPL v2 or newer.
+ */
+
+/* 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 stolen from the Linux kernel 2.6.35-rc. From now, code
+ * is GPL v2 only.
+ */
-- 
1.6.6.1

[SMS D-Bus 03/23] smutil.h: add missing header file dependencies

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/smsutil.h b/src/smsutil.h
index 66ef6f8..baa3eca 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -18,6 +18,14 @@
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  *
  */
+#ifndef __smsutil_h__
+#define __smsutil_h__
+
+#include <time.h>
+#include <glib/gtypes.h>
+#include <glib/gslist.h>
+#include <glib/gmessages.h>
+#include <glib/gchecksum.h>
 
 #define CBS_MAX_GSM_CHARS 93
 
@@ -539,3 +547,4 @@ 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);
+#endif /* #ifndef __smsutil_h__ */
-- 
1.6.6.1

[SMS D-Bus 04/23] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2594 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..b8451de 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

[SMS D-Bus 05/23] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2254 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 ++++-
 src/main.c   |    4 +++-
 3 files changed, 17 insertions(+), 2 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.
diff --git a/src/main.c b/src/main.c
index d8df2f2..b395cfa 100644
--- a/src/main.c
+++ b/src/main.c
@@ -110,7 +110,9 @@ static gboolean parse_debug(const char *key, const char *value,
 static GOptionEntry options[] = {
 	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
 				G_OPTION_ARG_CALLBACK, parse_debug,
-				"Specify debug options to enable", "DEBUG" },
+			   "Specify debug options to enable (see the "
+			   "man page for ofonod(8) for more information).",
+			   "DEBUG" },
 	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
 				G_OPTION_ARG_NONE, &option_detach,
 				"Don't run as daemon in background" },
-- 
1.6.6.1

[SMS D-Bus 06/23] SMS: introduce message ID API

[Inaky Perez-Gonzalez]

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

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

This adds a simple API to use for generating unique IDs for SMS
messages.
---
 Makefile.am            |    5 +-
 src/smsutil.c          |  191 +++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h          |   82 +++++++++++++++++++
 unit/test-sms-msg-id.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 489 insertions(+), 1 deletions(-)
 create mode 100644 unit/test-sms-msg-id.c

diff --git a/Makefile.am b/Makefile.am
index 96116a5..2d7cf9c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -363,7 +363,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@
@@ -402,6 +402,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 14dc43a..96c5001 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -3981,3 +3981,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 baa3eca..9f43c0b 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -547,4 +547,86 @@ 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);
+
 #endif /* #ifndef __smsutil_h__ */
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@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

[SMS D-Bus 07/23] introduce DECLARE_SMS_ADDR_STR()

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1681 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 |    3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 96c5001..78fabf5 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2314,7 +2314,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;
@@ -2391,7 +2391,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 9f43c0b..1c272bc 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -440,6 +440,9 @@ 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);
 
+enum { SMS_ADDR_STR_SIZE = 25 };
+#define DECLARE_SMS_ADDR_STR(a) char a[SMS_ADDR_STR_SIZE]
+
 gboolean sms_decode_address_field(const unsigned char *pdu, int len,
 					int *offset, gboolean sc,
 					struct sms_address *out);
-- 
1.6.6.1

[SMS D-Bus 08/23] export sms_assembly_encode_address

[Inaky Perez-Gonzalez]

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

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

For generating unique message naming (for persistence and D-Bus object
naming), we'll be using the address. Instead of repeating the
functions, we reuse the same infrastructure used by the SMS assembly
code.
---
 src/smsutil.c |    4 ++--
 src/smsutil.h |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 78fabf5..cdf28f9 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2293,8 +2293,8 @@ 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_assembly_encode_address(const struct sms_address *in,
+				     char *straddr)
 {
 	unsigned char pdu[12];
 	int offset = 0;
diff --git a/src/smsutil.h b/src/smsutil.h
index 1c272bc..0d47385 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -505,6 +505,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_assembly_encode_address(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

[SMS D-Bus 09/23] SMS: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 5176 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/sms.c |   64 +++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index b5c0614..66a0be3 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;
@@ -450,9 +449,13 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	const char *to;
 	const char *text;
 	GSList *msg_list;
-	int ref_offset;
+	int ref_offset, ref;
 	struct tx_queue_entry *entry;
 	struct ofono_modem *modem;
+	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);
 
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
@@ -462,31 +465,40 @@ 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_assembly_encode_address(&receiver, receiver_str)
+	    == FALSE)
+		return __ofono_error_failed(msg);
+
 	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
 					sms->use_delivery_reports);
 
 	if (!msg_list)
 		return __ofono_error_invalid_format(msg);
 
-	DBG("ref: %d, offset: %d", sms->ref, ref_offset);
+	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(&ref, sizeof(ref), &sms_msg_id);
+	sms_msg_id_printf(&sms_msg_id, msg_id_str, sizeof(msg_id_str));
 
-	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
+	set_ref_and_to(msg_list, ref, ref_offset, to);
 	entry = create_tx_queue_entry(msg_list);
-
-	sms_address_from_string(&entry->receiver, to);
+	ofono_debug("SMS: %p: ref %08x, offset %d, msg id: %s\n",
+		    entry, ref, ref_offset, msg_id_str);
+	entry->msg_id = ref;
+	entry->receiver = receiver;
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
-
-	if (ref_offset != 0) {
-		if (sms->ref == 65536)
-			sms->ref = 1;
-		else
-			sms->ref = sms->ref + 1;
-	}
-
 	entry->msg = dbus_message_ref(msg);
-	entry->msg_id = sms->next_msg_id++;
 	entry->status_report = sms->use_delivery_reports;
 
 	g_queue_push_tail(sms->txq, entry);
@@ -544,6 +556,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;
@@ -585,11 +600,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)
@@ -972,8 +994,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",
@@ -1065,8 +1085,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);
 	sms->use_delivery_reports =
-- 
1.6.6.1

[SMS D-Bus 10/23] sms: add doc about the extensions D-Bus API (not yet implemented)

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/doc/sms-api.txt b/doc/sms-api.txt
index baa4aa1..a23486a 100644
--- a/doc/sms-api.txt
+++ b/doc/sms-api.txt
@@ -46,3 +46,97 @@ Signals		PropertyChanged(string name, variant value)
 Properties	string ServiceCenterAddress
 
 			Contains the number of the SMS service center.
+
+
+
+SMS / Messaging interface
+=========================
+
+Requirements:
+
+ - provide an interface for management of text messages over cell
+   networks
+
+ - maintain messaging data while "in transit", meaning, while Ofono
+   owns it since the time it is submitted for delivery until it is
+   deemed delivered to the network or had failure (undeliverable /
+   undelivered).
+
+ - has to be persistent across daemon invocations (needs offline db)
+
+ - provides methods to submit a message for delivery, query status,
+   cancel delivery, as well as signals on status change
+
+
+High level interface
+
+  MSGHANDLE := objectpath
+
+      Each MSGHANDLE is a message object "in transit" (aka: owned by
+      oFono). So it becomes a D-Bus object name that supports the
+      org.ofono.Message interface.
+
+  MSGDATA := struct / dict
+
+      Data for the message. Still TBD what exactly will come in here,
+      other than the obvious destination and text data.
+
+org.ofono.SmsManager                  [interface]
+
+    This interface hangs off the modem object in oFono.
+
+    Marcel suggests in the future we could have an "any" modem that
+    orutes routes things automatically. Dennis says that in any case,
+    most of the times we have a single modem object dangling around.
+
+    MSGHANDLE SendMessage(MSGDATA)    [method]
+
+        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.
+
+    PendingMessages                   [property]
+
+        List of MSGHANDLEs which are currently pending/in-transit
+
+    GetProperties()	   	      [method]
+
+        Get manager's properties; one of them being the list of
+        pending messages.
+
+    ProperyChanged()                  [signal]
+
+        A property has changed.
+
+    IncomingMessage()                 [signal]
+
+        Message arrivedl; class != CLASS_0 FIXME
+
+    ImmediateMessage()                [signal]
+
+        Message arrived; class == CLASS_0 FIXME
+
+
+org.ofono.Message                     [interface]
+
+    MSGDATA GetProperties()           [method]
+
+        Returns all the data associated to a message, as well as its
+        current status.
+
+    Cancel()                          [method]
+
+        Cancel oFono's handling of the message, wether is submitting
+        it, retrying submissions, etc.
+
+    PropertyChanged()                  [signal]
+
+        Some property in the message has changed. If status change,
+        provide old and new status -- to ease up state machine
+        processing.
+
+	Dennis: subscribers can subscribe to signals from all object
+	that support an interface, so it's easy to subscribe to get
+	the signals from all the objects without having to subscribe
+	to each object.
-- 
1.6.6.1

[SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

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

Add a name field that will be used to contain a permanent name for
each SMS message in-transit, to be used for persistent storage and for
D-Bus object naming.

The persist name will be used to denote if persistence has to be
implemented in this in-transit SMS or not.

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

diff --git a/src/sms.c b/src/sms.c
index 66a0be3..d4d08fa 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -76,6 +76,7 @@ struct pending_pdu {
 	int pdu_len;
 };
 
+
 struct tx_queue_entry {
 	struct pending_pdu *pdus;
 	unsigned char num_pdus;
@@ -291,6 +292,25 @@ 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 *only* releases resources allocated *inside* @entry. The
+ * caller still has to explicitly free @entry (if needed) itself.
+ */
+static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
+{
+	g_free(entry->pdus);
+}
+
+static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
+{
+	struct tx_queue_entry *entry = _entry;
+	tx_queue_entry_destroy(entry);
+	g_free(entry);
+}
+
 static void tx_finished(const struct ofono_error *error, int mr, void *data)
 {
 	struct ofono_sms *sms = data;
@@ -320,7 +340,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED);
 
-		g_free(entry->pdus);
+		tx_queue_entry_destroy(entry);
 		g_free(entry);
 
 		if (g_queue_peek_head(sms->txq)) {
@@ -353,7 +373,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
 
-	g_free(entry->pdus);
+	tx_queue_entry_destroy(entry);
 	g_free(entry);
 
 	if (g_queue_peek_head(sms->txq)) {
@@ -987,7 +1007,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

[SMS D-Bus 12/23] SMS: produce a unique, persistent name for in-transit messages

[Inaky Perez-Gonzalez]

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

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

This name will be used for persistence and D-Bus object naming. It is
not very humanly readable, but most dependable to avoid collisions.
---
 src/sms.c     |   12 ++++++++++++
 src/smsutil.h |    6 ++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index d4d08fa..87ea926 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -77,6 +77,9 @@ struct pending_pdu {
 };
 
 
+/*
+ * @name: Name for the SMS message object (used by D-Bus)
+ */
 struct tx_queue_entry {
 	struct pending_pdu *pdus;
 	unsigned char num_pdus;
@@ -86,6 +89,7 @@ struct tx_queue_entry {
 	DBusMessage *msg;
 	gboolean status_report;
 	struct sms_address receiver;
+	char *name;
 };
 
 static void set_sca(struct ofono_sms *sms,
@@ -302,6 +306,7 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg,
 static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 {
 	g_free(entry->pdus);
+	g_free(entry->name);
 }
 
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
@@ -461,6 +466,9 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)
  * is created by create_tx_queue_entry() 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)
@@ -476,6 +484,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	DECLARE_SMS_MSG_ID_STRBUF(msg_id_str);
 	struct sms_address receiver;
 	DECLARE_SMS_ADDR_STR(receiver_str);
+	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,
@@ -520,6 +529,9 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	g_slist_free(msg_list);
 	entry->msg = dbus_message_ref(msg);
 	entry->status_report = sms->use_delivery_reports;
+	entry->name = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
+				      msg_id_str, entry->num_pdus);
+	ofono_debug("sms/entry %p name %s\n", entry, entry->name);
 
 	g_queue_push_tail(sms->txq, entry);
 
diff --git a/src/smsutil.h b/src/smsutil.h
index 0d47385..b87dffb 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -634,4 +634,10 @@ 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/%s_%i"
+
 #endif /* #ifndef __smsutil_h__ */
-- 
1.6.6.1

[SMS D-Bus 13/23] SMS: introduce bare state machine and transitions

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 6196 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/sms.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 107 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 87ea926..0bdfc45 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -50,6 +50,35 @@ static gboolean tx_next(gpointer user_data);
 
 static GSList *g_drivers = NULL;
 
+/*
+ * 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 DONE   | CANCELLED  EXPIRED
+ * UNINITIALIZED -    A     N    N    N    N    N
+ * QUEUED        N    -     A    A    N    A    N
+ * DONE          A    N     -    N    N    N    N
+ * CANCELING     N    N     N    -    A    A    A
+ * CANCELLED     A    N     N    N    -    N    N
+ * FAILED        A    N     N    N    N    -    N
+ * EXPIRED       A    N     N    N    N    N    -
+ */
+enum ofono_sms_tx_state {
+	OFONO_SMS_TX_ST_UNINITIALIZED,
+	OFONO_SMS_TX_ST_QUEUED,
+	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,
+};
+
 struct ofono_sms {
 	int flags;
 	DBusMessage *pending;
@@ -79,6 +108,7 @@ struct pending_pdu {
 
 /*
  * @name: Name for the SMS message object (used by D-Bus)
+ * @state: Current state of the (in-transit) SMS
  */
 struct tx_queue_entry {
 	struct pending_pdu *pdus;
@@ -90,6 +120,7 @@ struct tx_queue_entry {
 	gboolean status_report;
 	struct sms_address receiver;
 	char *name;
+	enum ofono_sms_tx_state state;
 };
 
 static void set_sca(struct ofono_sms *sms,
@@ -297,6 +328,77 @@ 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
+ *
+ * This is just syntatic 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;
+
+	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);
+		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'
  *
@@ -307,6 +409,7 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 {
 	g_free(entry->pdus);
 	g_free(entry->name);
+	entry->state = __OFONO_SMS_TX_ST_INVALID;
 }
 
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
@@ -337,6 +440,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_CANCELING);
 		entry = g_queue_pop_head(sms->txq);
 		__ofono_dbus_pending_reply(&entry->msg,
 					__ofono_error_failed(entry->msg));
@@ -345,6 +449,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED);
 
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED);
 		tx_queue_entry_destroy(entry);
 		g_free(entry);
 
@@ -378,6 +483,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
 
+	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE);
 	tx_queue_entry_destroy(entry);
 	g_free(entry);
 
@@ -534,6 +640,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	ofono_debug("sms/entry %p name %s\n", entry, entry->name);
 
 	g_queue_push_tail(sms->txq, entry);
+	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_QUEUED);
 
 	modem = __ofono_atom_get_modem(sms->atom);
 	__ofono_history_sms_send_pending(modem, entry->msg_id, to,
-- 
1.6.6.1

[SMS D-Bus 14/23] SMS: export outgoing messages over D-Bus (skeleton)

[Inaky Perez-Gonzalez]

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

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

This creates two frunctions, sms_msg_[un]register() which will
add/remove a D-Bus object for each SMS message when in transit.

Future changes make sms_msg_register() need information that is not
available at the time create_tx_queue_entry() is called, thus why
registration happens later.
---
 src/sms.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 57 insertions(+), 1 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 0bdfc45..49b9c4c 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"
 
@@ -108,6 +112,7 @@ struct pending_pdu {
 
 /*
  * @name: Name for the SMS message object (used by D-Bus)
+ * @dbus_path: D-Bus path for this object
  * @state: Current state of the (in-transit) SMS
  */
 struct tx_queue_entry {
@@ -120,6 +125,7 @@ struct tx_queue_entry {
 	gboolean status_report;
 	struct sms_address receiver;
 	char *name;
+	char *dbus_path;
 	enum ofono_sms_tx_state state;
 };
 
@@ -400,6 +406,55 @@ 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"		},
+	{ }
+};
+
+
+static
+void sms_msg_register(struct tx_queue_entry *sms_msg)
+{
+	g_assert(sms_msg->name != NULL);
+	sms_msg->dbus_path = g_strdup(sms_msg->name);
+	if (!g_dbus_register_interface(ofono_dbus_get_connection(),
+				       sms_msg->dbus_path, SMS_MSG_INTERFACE,
+				       sms_msg_methods, sms_msg_signals,
+				       NULL, sms_msg, NULL)) {
+		ofono_error("%s: Could not create %s interface",
+			    sms_msg->dbus_path, SMS_MSG_INTERFACE);
+		g_free(sms_msg->dbus_path);
+	}
+	ofono_debug("%s: %d: sms %p: MSG registered @ %s",
+		    __FILE__, __LINE__, sms_msg, sms_msg->dbus_path);
+}
+
+
+static
+void sms_msg_unregister(struct tx_queue_entry *sms_msg)
+{
+	g_dbus_unregister_interface(ofono_dbus_get_connection(),
+				    sms_msg->dbus_path, SMS_MSG_INTERFACE);
+	g_free(sms_msg->dbus_path);
+	ofono_debug("%s: %d: sms %p: MSG unregistered",
+		    __FILE__, __LINE__, sms_msg);
+}
+
+
+/*
  * Destroy/release the contents of a 'struct tx_queue_entry'
  *
  * This *only* releases resources allocated *inside* @entry. The
@@ -407,6 +462,7 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
  */
 static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 {
+	sms_msg_unregister(entry);
 	g_free(entry->pdus);
 	g_free(entry->name);
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
@@ -556,7 +612,6 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)
 		DBG("pdu_len: %d, tpdu_len: %d",
 				pdu->pdu_len, pdu->tpdu_len);
 	}
-
 	return entry;
 }
 
@@ -638,6 +693,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	entry->name = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
 				      msg_id_str, entry->num_pdus);
 	ofono_debug("sms/entry %p name %s\n", entry, entry->name);
+	sms_msg_register(entry);
 
 	g_queue_push_tail(sms->txq, entry);
 	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_QUEUED);
-- 
1.6.6.1

[SMS D-Bus 15/23] SMS: split sms_send_message() into a D-Bus front end and an internal API

[Inaky Perez-Gonzalez]

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

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

sms_send_message() is unfolded into:

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

- dbus_SendMessage(), which adapts sms_msg_send() to be a D-Bus call

This is done to allow plugins to use the same infrastructure as D-Bus
clients to send SMS messages.
---
 src/sms.c     |   66 +++++++++++++++++++++++++++++++++++++++++---------------
 src/smsutil.h |    6 +++++
 2 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 49b9c4c..0182ac8 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -466,6 +466,10 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 	g_free(entry->pdus);
 	g_free(entry->name);
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
+	if (entry->msg) {
+		dbus_message_unref(entry->msg);
+		entry->msg = NULL;
+	}
 }
 
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
@@ -615,8 +619,9 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)
 	return entry;
 }
 
+
 /*
- * 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)
@@ -631,12 +636,10 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)
  * @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 flags)
 {
-	struct ofono_sms *sms = data;
-	const char *to;
-	const char *text;
 	GSList *msg_list;
 	int ref_offset, ref;
 	struct tx_queue_entry *entry;
@@ -647,13 +650,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	DECLARE_SMS_ADDR_STR(receiver_str);
 	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);
-
 	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
@@ -664,13 +662,13 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	sms_address_from_string(&receiver, to);
 	if (sms_assembly_encode_address(&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);
 
 	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));
@@ -688,7 +686,6 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
-	entry->msg = dbus_message_ref(msg);
 	entry->status_report = sms->use_delivery_reports;
 	entry->name = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
 				      msg_id_str, entry->num_pdus);
@@ -699,21 +696,54 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_QUEUED);
 
 	modem = __ofono_atom_get_modem(sms->atom);
-	__ofono_history_sms_send_pending(modem, entry->msg_id, to,
-						time(NULL), text);
+	if (flags & SMS_MSG_SEND_HISTORY)
+		__ofono_history_sms_send_pending(modem, entry->msg_id, to,
+						 time(NULL), text);
 
 	if (g_queue_get_length(sms->txq) == 1)
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 
+	return entry;
+}
+
+
+/*
+ * 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_SendMessage(DBusConnection *conn, DBusMessage *msg,
+				     void *data)
+{
+	struct ofono_sms *sms = data;
+	const char *to;
+	const char *text;
+	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);
+
+	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY);
+	if (sms_msg == NULL)
+		return __ofono_error_invalid_format(msg);
+	/* Unreferenced upon tx_queue_entry_destroy() */
+	sms_msg->msg = dbus_message_ref(msg);
 	return NULL;
 }
 
+
 static GDBusMethodTable sms_manager_methods[] = {
 	{ "GetProperties",	"",	"a{sv}",	sms_get_properties,
 							G_DBUS_METHOD_FLAG_ASYNC },
 	{ "SetProperty",	"sv",	"",		sms_set_property,
 							G_DBUS_METHOD_FLAG_ASYNC },
-	{ "SendMessage",	"ss",	"",		sms_send_message,
+	{ "SendMessage",	"ss",	"",		dbus_SendMessage,
 							G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
diff --git a/src/smsutil.h b/src/smsutil.h
index b87dffb..40df71f 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -217,6 +217,12 @@ enum cbs_geo_scope {
 	CBS_GEO_SCOPE_CELL_NORMAL
 };
 
+/* Flags to sms_msg_send() */
+enum sms_msg_send_flags {
+	/* Record/dont this message to the history database */
+	SMS_MSG_SEND_HISTORY = 0x01,
+};
+
 struct sms_address {
 	enum sms_number_type number_type;
 	enum sms_numbering_plan numbering_plan;
-- 
1.6.6.1

[SMS D-Bus 16/23] SMS: introduce wait-for-ack state and infrastructure

[Inaky Perez-Gonzalez]

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

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

This introduces basic infrastructure to support wait-for-ack in SMS
messages. It consists of a state definition in the state transtition
machine, a wait-for-ack queue (ofono_sms->wfaq) and a flag setting in
which a message is tagged as 'requests wait for ack'.

This does not implement the network bits, just the basic
infrastructure bits.
---
 src/sms.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 0182ac8..97c7429 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -62,19 +62,21 @@ static GSList *g_drivers = NULL;
  *
  * Allowed transition table (Allowed, Not-allowed) from left to right:
  *
- *          UNINITIALIZED     CANCELING  FAILED
- *               | QUEUED DONE   | CANCELLED  EXPIRED
- * UNINITIALIZED -    A     N    N    N    N    N
- * QUEUED        N    -     A    A    N    A    N
- * DONE          A    N     -    N    N    N    N
- * CANCELING     N    N     N    -    A    A    A
- * CANCELLED     A    N     N    N    -    N    N
- * FAILED        A    N     N    N    N    -    N
- * EXPIRED       A    N     N    N    N    N    -
+ *          UNINITIALIZED         CANCELING  FAILED
+ *               | QUEUED WFA DONE   | CANCELLED  EXPIRED
+ * UNINITIALIZED -    A    N    N    N    N    N    N
+ * QUEUED        N    -    A    A    A    N    A    N
+ * WFA           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_WFA,
 	OFONO_SMS_TX_ST_DONE,
 	OFONO_SMS_TX_ST_CANCELING,
 	OFONO_SMS_TX_ST_CANCELLED,
@@ -83,6 +85,12 @@ enum ofono_sms_tx_state {
 	__OFONO_SMS_TX_ST_INVALID,
 };
 
+
+/**
+ * @wfaq: Waiting-For-Acknoledgement queue; messages in this queue
+ *     have been delivered but are waiting to be acknoledged by the
+ *     network.
+ */
 struct ofono_sms {
 	int flags;
 	DBusMessage *pending;
@@ -90,6 +98,7 @@ struct ofono_sms {
 	struct sms_assembly *assembly;
 	guint ref;
 	GQueue *txq;
+	GQueue *tx_wfaq;
 	gint tx_source;
 	struct ofono_message_waiting *mw;
 	unsigned int mw_watch;
@@ -113,9 +122,13 @@ struct pending_pdu {
 /*
  * @name: Name for the SMS message object (used by D-Bus)
  * @dbus_path: D-Bus path for this object
+ * @sms_mgr: SMS manager / driver object
  * @state: Current state of the (in-transit) SMS
+ * @status_report: message reception/delivery should be acknoledged by
+ *     the network.
  */
 struct tx_queue_entry {
+	struct ofono_sms *sms_mgr;
 	struct pending_pdu *pdus;
 	unsigned char num_pdus;
 	unsigned char cur_pdu;
@@ -375,9 +388,17 @@ 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_WFA
+			| 1 << OFONO_SMS_TX_ST_DONE
+			| 1 << OFONO_SMS_TX_ST_CANCELING);
+		break;
+	case OFONO_SMS_TX_ST_WFA:
+		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);
+			| 1 << OFONO_SMS_TX_ST_FAILED
+			| 1 << OFONO_SMS_TX_ST_EXPIRED);
 		break;
 	case OFONO_SMS_TX_ST_CANCELING:
 		ofono_sms_tx_state_check(
@@ -543,9 +564,14 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
 
-	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE);
-	tx_queue_entry_destroy(entry);
-	g_free(entry);
+	if (entry->status_report) {
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_WFA);
+		g_queue_push_tail(sms->tx_wfaq, entry);
+	} else {
+		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE);
+		tx_queue_entry_destroy(entry);
+		g_free(entry);
+	}
 
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
@@ -683,6 +709,7 @@ struct tx_queue_entry *sms_msg_send(
 		    entry, ref, ref_offset, msg_id_str);
 	entry->msg_id = ref;
 	entry->receiver = receiver;
+	entry->sms_mgr = sms;
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
@@ -1217,6 +1244,13 @@ static void sms_remove(struct ofono_atom *atom)
 		sms->txq = NULL;
 	}
 
+	if (sms->tx_wfaq) {
+		g_queue_foreach(sms->tx_wfaq,
+				tx_queue_entry_destroy_free, NULL);
+		g_queue_free(sms->tx_wfaq);
+		sms->tx_wfaq = NULL;
+	}
+
 	if (sms->settings) {
 		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
 					"NextReference", sms->ref);
@@ -1269,6 +1303,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_wfaq = g_queue_new();
 	sms->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_SMS,
 						sms_remove, sms);
 
-- 
1.6.6.1

[SMS D-Bus 17/23] SMS: introduce sms_msg_cancel and its D-Bus wrapper

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 3783 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.
---
 src/sms.c     |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h |   15 +++++++++++++++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 97c7429..faaacbe 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -426,6 +426,24 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,
 }
 
 
+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);
+	sms_msg->msg = dbus_message_ref(msg);
+	__ofono_dbus_pending_reply(
+		&sms_msg->msg,
+		dbus_message_new_method_return(sms_msg->msg));
+	return NULL;
+}
+
+
 /*
  * D-Bus SMS Message interface
  *
@@ -435,6 +453,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, G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
@@ -734,6 +754,43 @@ 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.
+ *
+ * \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_WFA)
+		g_queue_remove(sms->tx_wfaq, 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(sms_msg);
+}
+
+
 /*
  * D-Bus: Send a SMS text message
  *
diff --git a/src/smsutil.h b/src/smsutil.h
index 40df71f..ad1e560 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -559,6 +559,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. */
-- 
1.6.6.1

[SMS D-Bus 18/23] SMS: rename tx_queue_entry->msg to ->dbus_msg for clarity

[Inaky Perez-Gonzalez]

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

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

'msg' gets too confusing: is it the SMS message, a D-Bus message?
---
 src/sms.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index faaacbe..4ad9e61 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -134,7 +134,7 @@ struct tx_queue_entry {
 	unsigned char cur_pdu;
 	unsigned int msg_id;
 	unsigned int retry;
-	DBusMessage *msg;
+	DBusMessage *dbus_msg;
 	gboolean status_report;
 	struct sms_address receiver;
 	char *name;
@@ -436,10 +436,10 @@ static DBusMessage *dbus_sms_msg_cancel(
 		return __ofono_error_invalid_args(msg);
 	sms_msg_cancel(sms_msg,
 		       do_network_cancel ? SMS_MSG_CANCEL_IN_NETWORK : 0);
-	sms_msg->msg = dbus_message_ref(msg);
+	sms_msg->dbus_msg = dbus_message_ref(msg);
 	__ofono_dbus_pending_reply(
-		&sms_msg->msg,
-		dbus_message_new_method_return(sms_msg->msg));
+		&sms_msg->dbus_msg,
+		dbus_message_new_method_return(sms_msg->dbus_msg));
 	return NULL;
 }
 
@@ -507,9 +507,9 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 	g_free(entry->pdus);
 	g_free(entry->name);
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
-	if (entry->msg) {
-		dbus_message_unref(entry->msg);
-		entry->msg = NULL;
+	if (entry->dbus_msg) {
+		dbus_message_unref(entry->dbus_msg);
+		entry->dbus_msg = NULL;
 	}
 }
 
@@ -543,8 +543,8 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 
 		ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_CANCELING);
 		entry = g_queue_pop_head(sms->txq);
-		__ofono_dbus_pending_reply(&entry->msg,
-					__ofono_error_failed(entry->msg));
+		__ofono_dbus_pending_reply(&entry->dbus_msg,
+					__ofono_error_failed(entry->dbus_msg));
 
 		__ofono_history_sms_send_status(modem, entry->msg_id,
 					time(NULL),
@@ -578,8 +578,9 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	}
 
 	entry = g_queue_pop_head(sms->txq);
-	__ofono_dbus_pending_reply(&entry->msg,
-				dbus_message_new_method_return(entry->msg));
+	__ofono_dbus_pending_reply(
+		&entry->dbus_msg,
+		dbus_message_new_method_return(entry->dbus_msg));
 	__ofono_history_sms_send_status(modem, entry->msg_id,
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
@@ -817,7 +818,7 @@ static DBusMessage *dbus_SendMessage(DBusConnection *conn, DBusMessage *msg,
 	if (sms_msg == NULL)
 		return __ofono_error_invalid_format(msg);
 	/* Unreferenced upon tx_queue_entry_destroy() */
-	sms_msg->msg = dbus_message_ref(msg);
+	sms_msg->dbus_msg = dbus_message_ref(msg);
 	return NULL;
 }
 
-- 
1.6.6.1

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

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/sms.c b/src/sms.c
index 4ad9e61..77904ad 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -86,6 +86,24 @@ enum ofono_sms_tx_state {
 };
 
 
+static
+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_WFA:		return "WFA";
+	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";
+	}
+}
+
+
 /**
  * @wfaq: Waiting-For-Acknoledgement queue; messages in this queue
  *     have been delivered but are waiting to be acknoledged by the
@@ -426,6 +444,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;
+}
+
+
 static DBusMessage *dbus_sms_msg_cancel(
 	DBusConnection * conn, DBusMessage *msg, void *data)
 {
@@ -453,6 +494,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, G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
-- 
1.6.6.1

[SMS D-Bus 20/23] SMS: send D-Bus SMS-MSG::ProperyChanged signals when message changes status

[Inaky Perez-Gonzalez]

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

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

This hooks __sms_msg_state_change_dbus_signal() into
__ofono_sms_tx_state_set(), 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.

With this, we can remove, in tx_finished() the asynchornous
finalization of the call (via sms_msg->dbus_msg) and make the call
synchronous (follow up commit).

The rest of the sites that were calling __ofono_sms_tx_state_set() get
(by definition) to send the signal.
---
 src/sms.c                      |   33 +++++++++++++++++++++++++++------
 test/test-sms-msg-state-change |   24 ++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 6 deletions(-)
 create mode 100755 test/test-sms-msg-state-change

diff --git a/src/sms.c b/src/sms.c
index 77904ad..11d1f33 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -380,6 +380,32 @@ static void ofono_sms_tx_state_check(const char *file, unsigned line,
 
 
 /*
+ * Send a PropertyChange signal when the state changes
+ *
+ * D-Bus typing is "ss" for variable/value.
+ */
+static
+void __sms_msg_state_change_dbus_signal(struct tx_queue_entry *sms_msg)
+{
+	DBusConnection *dbus_conn = ofono_dbus_get_connection();
+	DBusMessage *dbus_signal;
+	DBusMessageIter iter;
+	const char *property = "TXState";
+	const char *new_state_str;
+
+	dbus_signal = dbus_message_new_signal(
+		sms_msg->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(sms_msg->state);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &new_state_str);
+	g_dbus_send_message(dbus_conn, dbus_signal);
+}
+
+
+/*
  * Set a pending SMS's state
  *
  * This is just syntatic sugar that validates that the transition is
@@ -441,6 +467,7 @@ 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;
+	__sms_msg_state_change_dbus_signal(entry);
 }
 
 
@@ -584,10 +611,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_CANCELING);
 		entry = g_queue_pop_head(sms->txq);
-		__ofono_dbus_pending_reply(&entry->dbus_msg,
-					__ofono_error_failed(entry->dbus_msg));
 
 		__ofono_history_sms_send_status(modem, entry->msg_id,
 					time(NULL),
@@ -621,9 +645,6 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	}
 
 	entry = g_queue_pop_head(sms->txq);
-	__ofono_dbus_pending_reply(
-		&entry->dbus_msg,
-		dbus_message_new_method_return(entry->dbus_msg));
 	__ofono_history_sms_send_status(modem, entry->msg_id,
 					time(NULL),
 					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
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

[SMS D-Bus 21/23] SMS: make D-Bus SendMessage and Cancel fully synchronous

[Inaky Perez-Gonzalez]

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

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

Up until now, SMSManager::SendMessage() and SMSMessage::Cancel() where
D-Bus asynchronous. They would perform the D-Bus method return reply
inside the callbacks, as they needed to wait for the operation to
complete.

Relying on previous commits that make the SMS message object send
D-Bus updates when changing state, this makes Sendmessage and Cancel
return inmediately. SendMessage() will return an string with the
SMS Message object name and Cancel() will just return.

This renders all the D-Bus message bookeeping in 'struct
tx_queue_entry' uneeded.
---
 src/sms.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 11d1f33..96315b2 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -504,11 +504,7 @@ static DBusMessage *dbus_sms_msg_cancel(
 		return __ofono_error_invalid_args(msg);
 	sms_msg_cancel(sms_msg,
 		       do_network_cancel ? SMS_MSG_CANCEL_IN_NETWORK : 0);
-	sms_msg->dbus_msg = dbus_message_ref(msg);
-	__ofono_dbus_pending_reply(
-		&sms_msg->dbus_msg,
-		dbus_message_new_method_return(sms_msg->dbus_msg));
-	return NULL;
+	return dbus_message_new_method_return(msg);
 }
 
 
@@ -522,9 +518,9 @@ static DBusMessage *dbus_sms_msg_cancel(
 static
 GDBusMethodTable sms_msg_methods[] = {
 	{ "GetProperties",	"",	"a{sv}",
-	  dbus_sms_msg_get_properties, G_DBUS_METHOD_FLAG_ASYNC },
+	  dbus_sms_msg_get_properties, 0 },
 	{ "Cancel",		DBUS_TYPE_BOOLEAN_AS_STRING,	"",
-	  dbus_sms_msg_cancel, G_DBUS_METHOD_FLAG_ASYNC },
+	  dbus_sms_msg_cancel, 0 },
 	{ }
 };
 
@@ -577,10 +573,6 @@ static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
 	g_free(entry->pdus);
 	g_free(entry->name);
 	entry->state = __OFONO_SMS_TX_ST_INVALID;
-	if (entry->dbus_msg) {
-		dbus_message_unref(entry->dbus_msg);
-		entry->dbus_msg = NULL;
-	}
 }
 
 static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
@@ -872,6 +864,7 @@ static DBusMessage *dbus_SendMessage(DBusConnection *conn, DBusMessage *msg,
 	const char *to;
 	const char *text;
 	struct tx_queue_entry *sms_msg;
+	DBusMessage *reply;
 
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
@@ -881,9 +874,13 @@ static DBusMessage *dbus_SendMessage(DBusConnection *conn, DBusMessage *msg,
 	sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY);
 	if (sms_msg == NULL)
 		return __ofono_error_invalid_format(msg);
-	/* Unreferenced upon tx_queue_entry_destroy() */
-	sms_msg->dbus_msg = dbus_message_ref(msg);
-	return NULL;
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+	dbus_message_append_args(reply,
+				 DBUS_TYPE_STRING, &sms_msg->dbus_path,
+				 DBUS_TYPE_INVALID);
+	return reply;
 }
 
 
-- 
1.6.6.1

[SMS D-Bus 22/23] SMS: set the SRR bit in outgoing PDUs if WFA is requested

[Inaky Perez-Gonzalez]

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

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

When calling sms_text_prepare(), pass a new argument (srr) that
denotes if the TP-SRR bit in the PDU should be set to request an
Status Report from the Service Center.
---
 src/sms.c     |   13 ++++++++++---
 src/smsutil.h |    2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 96315b2..1b35fa9 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -752,6 +752,7 @@ struct tx_queue_entry *sms_msg_send(
 	struct sms_address receiver;
 	DECLARE_SMS_ADDR_STR(receiver_str);
 	const char *sms_path = __ofono_atom_get_path(sms->atom);
+	gboolean status_report;
 
 	if (valid_phone_number_format(to) == FALSE)
 		return NULL;
@@ -767,8 +768,14 @@ struct tx_queue_entry *sms_msg_send(
 	    == FALSE)
 		return NULL;
 
-	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
-					sms->use_delivery_reports);
+	/*
+	 * Request submit report? Set it in sms->submit for the
+	 * encoding code to decide how to encode it, take the default
+	 * from the SMS manager object.
+	 */
+	status_report = flags & SMS_MSG_SEND_STATUS_REPORT ?
+		TRUE : sms->use_delivery_reports;
+	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, status_report);
 
 	if (!msg_list)
 		return NULL;
@@ -790,7 +797,7 @@ struct tx_queue_entry *sms_msg_send(
 
 	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
 	g_slist_free(msg_list);
-	entry->status_report = sms->use_delivery_reports;
+	entry->status_report = status_report;
 	entry->name = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
 				      msg_id_str, entry->num_pdus);
 	ofono_debug("sms/entry %p name %s\n", entry, entry->name);
diff --git a/src/smsutil.h b/src/smsutil.h
index ad1e560..bf23f0c 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -569,6 +569,8 @@ enum sms_msg_cancel_flags {
 	 * tries to cancel it if in transit.
 	 */
 	SMS_MSG_CANCEL_IN_NETWORK = 0x1,
+	/* Request and wait for receipt acknowledgement from the network */
+	SMS_MSG_SEND_STATUS_REPORT = 0x02,
 };
 
 struct tx_queue_entry;
-- 
1.6.6.1

[SMS D-Bus 23/23] 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 cdf28f9..54278ad 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

Re: [SMS D-Bus 02/23] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Marcel Holtmann]

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

Hi Inaky,

> 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/util.h |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)

I still think that using include/bug.h is way better. And that way we
could more easily sync this with ConnMan. Since this might come in handy
for ConnMan as well.

Regards

Marcel

Re: [SMS D-Bus 03/23] smutil.h: add missing header file dependencies

[Marcel Holtmann]

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

Hi Inaky,

>  src/smsutil.h |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/src/smsutil.h b/src/smsutil.h
> index 66ef6f8..baa3eca 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -18,6 +18,14 @@
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   *
>   */
> +#ifndef __smsutil_h__
> +#define __smsutil_h__

for internal headers I don't want the circular inclusion protection. It
buys us nothing. And leaving it out will warn us when we do circular
inclusion. That is a clear indication that something is a bit too
complicated if it is needed.

Regards

Marcel

Re: [SMS D-Bus 02/23] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

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

Hey Marcel

On Fri, 2010-06-25 at 16:46 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > 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/util.h |   28 ++++++++++++++++++++++++++++
> >  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> I still think that using include/bug.h is way better. And that way we
> could more easily sync this with ConnMan. Since this might come in handy
> for ConnMan as well.

I don't have a problem with it, but I am still waiting for Denis' take
on it.

Re: [SMS D-Bus 03/23] smutil.h: add missing header file dependencies

[Inaky Perez-Gonzalez]

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

On Sat, 2010-06-26 at 01:48 +0200, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> >  src/smsutil.h |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/smsutil.h b/src/smsutil.h
> > index 66ef6f8..baa3eca 100644
> > --- a/src/smsutil.h
> > +++ b/src/smsutil.h
> > @@ -18,6 +18,14 @@
> >   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> >   *
> >   */
> > +#ifndef __smsutil_h__
> > +#define __smsutil_h__
> 
> for internal headers I don't want the circular inclusion protection. It
> buys us nothing. And leaving it out will warn us when we do circular
> inclusion. That is a clear indication that something is a bit too
> complicated if it is needed.

Yeah, I just forgot to drop it.

Re: [SMS D-Bus 02/23] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

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

On Mon, 2010-06-28 at 10:01 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > 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/util.h |   28 ++++++++++++++++++++++++++++
> >  1 files changed, 28 insertions(+), 0 deletions(-)
> > 
> 
> This stuff really doesn't belong in util.h.  That file is ancient and if I were 
> to do it again, I'd rename it into textutil.h or gsmutil.h.  I only really 
> want to keep text conversions there.
> 
> I suggest putting this stuff into include/bug.h per Marcel's earlier email.

Okey dokey, will do so.

Re: [SMS D-Bus 02/23] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Denis Kenzior]

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

Hi Inaky,

> 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/util.h |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 

This stuff really doesn't belong in util.h.  That file is ancient and if I were 
to do it again, I'd rename it into textutil.h or gsmutil.h.  I only really 
want to keep text conversions there.

I suggest putting this stuff into include/bug.h per Marcel's earlier email.

Regards,
-Denis

Re: [SMS D-Bus 14/23] SMS: export outgoing messages over D-Bus (skeleton)

[Inaky Perez-Gonzalez]

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

On Fri, 2010-06-25 at 16:15 -0700, Inaky Perez-Gonzalez wrote: 
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> This creates two frunctions, sms_msg_[un]register() which will
> add/remove a D-Bus object for each SMS message when in transit.
> 
> Future changes make sms_msg_register() need information that is not
> available at the time create_tx_queue_entry() is called, thus why
> registration happens later.

Found a few rough corners here when integrating the final parts of
status reports into the SMS state-machine. A re-submit will have this
incremental patch merged:

diff --git a/src/sms.c b/src/sms.c
index 1b35fa9..27a3d64 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -532,8 +532,15 @@ GDBusSignalTable sms_msg_signals[] = {
 };
 

+/**
+ *
+ * @returns 0 if ok, 0 on error
+ *
+ * On success, sms_msg->dbus_path is non-NULL; this is needed as the
+ * cleanup path on error are not easy to break up.
+ */
 static
-void sms_msg_register(struct tx_queue_entry *sms_msg)
+int sms_msg_register(struct tx_queue_entry *sms_msg)
 {
 	g_assert(sms_msg->name != NULL);
 	sms_msg->dbus_path = g_strdup(sms_msg->name);
@@ -544,20 +551,26 @@ void sms_msg_register(struct tx_queue_entry *sms_msg)
 		ofono_error("%s: Could not create %s interface",
 			    sms_msg->dbus_path, SMS_MSG_INTERFACE);
 		g_free(sms_msg->dbus_path);
+		sms_msg->dbus_path = NULL;
+		return 1;
+	} else {
+		ofono_debug("%s: %d: sms %p @ %s: MSG registered",
+			    __FILE__, __LINE__, sms_msg, sms_msg->dbus_path);
+		return 0;
 	}
-	ofono_debug("%s: %d: sms %p: MSG registered @ %s",
-		    __FILE__, __LINE__, sms_msg, sms_msg->dbus_path);
 }
 

 static
 void sms_msg_unregister(struct tx_queue_entry *sms_msg)
 {
+	if (sms_msg->dbus_path == NULL)
+		return;
+	ofono_debug("%s: %d: sms %p @ %s: MSG unregistered",
+		    __FILE__, __LINE__, sms_msg, sms_msg->dbus_path);
 	g_dbus_unregister_interface(ofono_dbus_get_connection(),
 				    sms_msg->dbus_path, SMS_MSG_INTERFACE);
 	g_free(sms_msg->dbus_path);
-	ofono_debug("%s: %d: sms %p: MSG unregistered",
-		    __FILE__, __LINE__, sms_msg);
 }
 

@@ -801,7 +814,10 @@ struct tx_queue_entry *sms_msg_send(
 	entry->name = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path,
 				      msg_id_str, entry->num_pdus);
 	ofono_debug("sms/entry %p name %s\n", entry, entry->name);
-	sms_msg_register(entry);
+	if (sms_msg_register(entry)) {
+		tx_queue_entry_destroy_free(entry, NULL);
+		return NULL;
+	}
 
 	g_queue_push_tail(sms->txq, entry);
 	ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_QUEUED);

Re: [SMS D-Bus 22/23] SMS: set the SRR bit in outgoing PDUs if WFA is requested

[Inaky Perez-Gonzalez]

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

The final patch in this series for integrating the status report into
D-Bus message state machine is added here -- don't want to resubmit the
whole thing as this is still an RFC.

From 2a22f2cb4328c54fb5cb544056541de63667588c Mon Sep 17 00:00:00 2001
From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Date: Mon, 28 Jun 2010 16:18:27 -0700
Subject: [PATCH] SMS: hookup status reports to message status machine

When a message is waiting for a delivery report, it gets added to an
special queue and it will show in D-Bus. Make it so that when the
message's delivery is acknowledged, the message's status machine is
updated, and thus, D-Bus signals for it are delivered.
---
 src/sms.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 27a3d64..bc0f80f 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -1148,6 +1148,21 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
+
+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_wfaq, sms_msg);
+	ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_DONE);
+	tx_queue_entry_destroy_free(sms_msg, NULL);
+}
+
+
 static void handle_sms_status_report(struct ofono_sms *sms,
 						const struct sms *incoming)
 {
@@ -1159,6 +1174,9 @@ static void handle_sms_status_report(struct ofono_sms *sms,
 						&delivered) == FALSE)
 		return;
 
+	g_queue_foreach(sms->tx_wfaq, __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);

Re: [SMS D-Bus 01/23] documentation: add note about referencing standards

[Denis Kenzior]

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

Hi Inaky,

On 06/25/2010 06:15 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> ---
>  doc/standards.txt |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>  create mode 100644 doc/standards.txt

This one has been applied, thanks.

Regards,
-Denis

Re: [SMS D-Bus 04/23] write_file: make transaction-safe

[Denis Kenzior]

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

Hi Inaky,

On 06/25/2010 06:15 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.

Patch is fine, but:

Applying: write_file: make transaction-safe
/home/denkenz/ofono-master/.git/rebase-apply/patch:13: trailing whitespace.
/*
/home/denkenz/ofono-master/.git/rebase-apply/patch:22: trailing whitespace.
 */
/home/denkenz/ofono-master/.git/rebase-apply/patch:75: trailing whitespace.
	g_free(tmp_path);		
fatal: 3 lines add whitespace errors.

Regards,
-Denis

Re: [SMS D-Bus 05/23] doc: explain debugging options to -d, add a pointer in -h to manpage

[Denis Kenzior]

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

Hi Inaky,

Please remember not to go over 50 characters on your git commit
description header and more than 72 characters on your git commit
description body.

> @@ -110,7 +110,9 @@ static gboolean parse_debug(const char *key, const char *value,
>  static GOptionEntry options[] = {
>  	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
>  				G_OPTION_ARG_CALLBACK, parse_debug,
> -				"Specify debug options to enable", "DEBUG" },
> +			   "Specify debug options to enable (see the "
> +			   "man page for ofonod(8) for more information).",

I still think mentioning the man page is a bit redundant.

> +			   "DEBUG" },
>  	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
>  				G_OPTION_ARG_NONE, &option_detach,
>  				"Don't run as daemon in background" },

Regards,
-Denis

Re: [SMS D-Bus 05/23] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-02 at 13:43 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> Please remember not to go over 50 characters on your git commit
> description header and more than 72 characters on your git commit
> description body.
> 
> > @@ -110,7 +110,9 @@ static gboolean parse_debug(const char *key, const char *value,
> >  static GOptionEntry options[] = {
> >  	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
> >  				G_OPTION_ARG_CALLBACK, parse_debug,
> > -				"Specify debug options to enable", "DEBUG" },
> > +			   "Specify debug options to enable (see the "
> > +			   "man page for ofonod(8) for more information).",
> 
> I still think mentioning the man page is a bit redundant.
> 
> > +			   "DEBUG" },
> >  	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
> >  				G_OPTION_ARG_NONE, &option_detach,
> >  				"Don't run as daemon in background" },
> 
> Regards,
> -Denis

Let's just drop this one then.

Re: [SMS D-Bus 05/23] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-02 at 13:43 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> Please remember not to go over 50 characters on your git commit
> description header and more than 72 characters on your git commit
> description body.
> 
> > @@ -110,7 +110,9 @@ static gboolean parse_debug(const char *key, const char *value,
> >  static GOptionEntry options[] = {
> >  	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
> >  				G_OPTION_ARG_CALLBACK, parse_debug,
> > -				"Specify debug options to enable", "DEBUG" },
> > +			   "Specify debug options to enable (see the "
> > +			   "man page for ofonod(8) for more information).",
> 
> I still think mentioning the man page is a bit redundant.
> 
> > +			   "DEBUG" },
> >  	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
> >  				G_OPTION_ARG_NONE, &option_detach,
> >  				"Don't run as daemon in background" },
> 
> Regards,
> -Denis

Bah, hit enter too soon, sorry -- meant drop the .c code adding that,
but keep the additions to the man page. I'll redo that one.

Re: [SMS D-Bus 04/23] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-02 at 13:39 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 06/25/2010 06:15 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.
> 
> Patch is fine, but:
> 
> Applying: write_file: make transaction-safe
> /home/denkenz/ofono-master/.git/rebase-apply/patch:13: trailing whitespace.
> /*

Yup, sorry, I missed those. They have been cleaned up already -- next
resubmit will have it fixed.

Re: [SMS D-Bus 07/23] introduce DECLARE_SMS_ADDR_STR()

[Denis Kenzior]

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

Hi Inaky,

> +enum { SMS_ADDR_STR_SIZE = 25 };
> +#define DECLARE_SMS_ADDR_STR(a) char a[SMS_ADDR_STR_SIZE]
> +
>  gboolean sms_decode_address_field(const unsigned char *pdu, int len,
>  					int *offset, gboolean sc,
>  					struct sms_address *out);

Sounds fine to me, but can we get rid of the enum declaration? simply
char a[25] seems to be enough.  Might want to add a comment explaining
why it is 25.  Namely that sms address is encoded into 12 bytes, stored
in hex format and space for a null.

Regards,
-Denis

Re: [SMS D-Bus 08/23] export sms_assembly_encode_address

[Denis Kenzior]

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

Hi Inaky,

>  void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
> +gboolean sms_assembly_encode_address(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);

Can we rename this into sms_address_to_hex_string?  That seems to
reflect the intent a bit better.

Regards,
-Denis

Re: [SMS D-Bus 10/23] sms: add doc about the extensions D-Bus API (not yet implemented)

[Denis Kenzior]

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

Hi Inaky,

> +
> +
> +
> +SMS / Messaging interface
> +=========================
> +
> +Requirements:
> +
> + - provide an interface for management of text messages over cell
> +   networks
> +
> + - maintain messaging data while "in transit", meaning, while Ofono
> +   owns it since the time it is submitted for delivery until it is
> +   deemed delivered to the network or had failure (undeliverable /
> +   undelivered).
> +
> + - has to be persistent across daemon invocations (needs offline db)
> +
> + - provides methods to submit a message for delivery, query status,
> +   cancel delivery, as well as signals on status change
> +
> +
> +High level interface
> +
> +  MSGHANDLE := objectpath
> +
> +      Each MSGHANDLE is a message object "in transit" (aka: owned by
> +      oFono). So it becomes a D-Bus object name that supports the
> +      org.ofono.Message interface.
> +
> +  MSGDATA := struct / dict
> +
> +      Data for the message. Still TBD what exactly will come in here,
> +      other than the obvious destination and text data.
> +

Please put this type of stuff into doc/overview.txt or doc/sms.txt.  I'd
like to stick to the API conventions in doc/foo-api.txt.

> +org.ofono.SmsManager                  [interface]
> +
> +    This interface hangs off the modem object in oFono.
> +
> +    Marcel suggests in the future we could have an "any" modem that
> +    orutes routes things automatically. Dennis says that in any case,
> +    most of the times we have a single modem object dangling around.
> +
> +    MSGHANDLE SendMessage(MSGDATA)    [method]
> +
> +        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.
> +
> +    PendingMessages                   [property]
> +
> +        List of MSGHANDLEs which are currently pending/in-transit
> +
> +    GetProperties()	   	      [method]
> +
> +        Get manager's properties; one of them being the list of
> +        pending messages.
> +
> +    ProperyChanged()                  [signal]
> +
> +        A property has changed.
> +
> +    IncomingMessage()                 [signal]
> +
> +        Message arrivedl; class != CLASS_0 FIXME
> +
> +    ImmediateMessage()                [signal]
> +
> +        Message arrived; class == CLASS_0 FIXME
> +
> +
> +org.ofono.Message                     [interface]
> +
> +    MSGDATA GetProperties()           [method]
> +
> +        Returns all the data associated to a message, as well as its
> +        current status.
> +
> +    Cancel()                          [method]
> +
> +        Cancel oFono's handling of the message, wether is submitting
> +        it, retrying submissions, etc.
> +
> +    PropertyChanged()                  [signal]
> +
> +        Some property in the message has changed. If status change,
> +        provide old and new status -- to ease up state machine
> +        processing.
> +

Please convert this to proper doc/sms-api.txt style and remove the old API.

> +	Dennis: subscribers can subscribe to signals from all object
> +	that support an interface, so it's easy to subscribe to get
> +	the signals from all the objects without having to subscribe
> +	to each object.

This is a task that needs to be added to the TODO for multiple
interfaces.  In particular NetworkOperator, VoiceCall, PrimaryContext, etc.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

Hi Inaky,

<snip>

> +
> +/*
> + * Destroy/release the contents of a 'struct tx_queue_entry'
> + *
> + * This *only* releases resources allocated *inside* @entry. The
> + * caller still has to explicitly free @entry (if needed) itself.
> + */
> +static void tx_queue_entry_destroy(struct tx_queue_entry *entry)
> +{
> +	g_free(entry->pdus);
> +}
> +
> +static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused)
> +{
> +	struct tx_queue_entry *entry = _entry;
> +	tx_queue_entry_destroy(entry);
> +	g_free(entry);
> +}
> +

>  static void tx_finished(const struct ofono_error *error, int mr, void *data)
>  {
>  	struct ofono_sms *sms = data;
> @@ -320,7 +340,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
>  					time(NULL),
>  					OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED);
>  
> -		g_free(entry->pdus);
> +		tx_queue_entry_destroy(entry);
>  		g_free(entry);

It seems we can use tx_queue_entry_destroy_free here...

>  
>  		if (g_queue_peek_head(sms->txq)) {
> @@ -353,7 +373,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
>  					time(NULL),
>  					OFONO_HISTORY_SMS_STATUS_SUBMITTED);
>  
> -	g_free(entry->pdus);
> +	tx_queue_entry_destroy(entry);
>  	g_free(entry);

Same comment as above.

>  
>  	if (g_queue_peek_head(sms->txq)) {
> @@ -987,7 +1007,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;
>  	}

Can we simply unify the two functions and simply call it
tx_queue_entry_free?  For symmetry renaming create_tx_queue_entry to
tx_queue_entry_new would be nice.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 16:04 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> <snip>
> 
> ....

> >  	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;
> >  	}
> 
> Can we simply unify the two functions and simply call it
> tx_queue_entry_free?  For symmetry renaming create_tx_queue_entry to
> tx_queue_entry_new would be nice.

Makes sense -- thought it cannot be done, but I realized
sms_msg_cancel() needs a _destroy_free(), not just a _destroy(), so that
settles that last one. Will do the _create() rename too.

Re: [SMS D-Bus 07/23] introduce DECLARE_SMS_ADDR_STR()

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 15:54 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +enum { SMS_ADDR_STR_SIZE = 25 };
> > +#define DECLARE_SMS_ADDR_STR(a) char a[SMS_ADDR_STR_SIZE]
> > +
> >  gboolean sms_decode_address_field(const unsigned char *pdu, int len,
> >  					int *offset, gboolean sc,
> >  					struct sms_address *out);
> 
> Sounds fine to me, but can we get rid of the enum declaration? simply
> char a[25] seems to be enough.  Might want to add a comment explaining
> why it is 25.  Namely that sms address is encoded into 12 bytes, stored
> in hex format and space for a null.

Ack, will do.

Re: [SMS D-Bus 08/23] export sms_assembly_encode_address

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 15:57 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> >  void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
> > +gboolean sms_assembly_encode_address(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);
> 
> Can we rename this into sms_address_to_hex_string?  That seems to
> reflect the intent a bit better.

Ok -- this will then touch a few more other sites in the code.

Re: [SMS D-Bus 10/23] sms: add doc about the extensions D-Bus API (not yet implemented)

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 16:01 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +
> > +
.... 
> > +	Dennis: subscribers can subscribe to signals from all object
> > +	that support an interface, so it's easy to subscribe to get
> > +	the signals from all the objects without having to subscribe
> > +	to each object.
> 
> This is a task that needs to be added to the TODO for multiple
> interfaces.  In particular NetworkOperator, VoiceCall, PrimaryContext, etc.
> 
> Regards,
> -Denis

K, will report all these too.

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 16:32 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> ....
> > settles that last one. Will do the _create() rename too.
> > 
> > 
> 
> Good, can you resubmit this one separately soonish?  I'm touching this
> area of the code because of Andrew's changes for STK Send SMS command
> handling.

I'll resubmit the whole series of stuff you haven't merged yet with all
the feedback incorporated. Should be done tomorrow the latest.

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

Hi Inaky,

On 07/07/2010 06:24 PM, Inaky Perez-Gonzalez wrote:
> On Wed, 2010-07-07 at 16:04 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>> <snip>
>>
>> ....
> 
>>>  	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;
>>>  	}
>>
>> Can we simply unify the two functions and simply call it
>> tx_queue_entry_free?  For symmetry renaming create_tx_queue_entry to
>> tx_queue_entry_new would be nice.
> 
> Makes sense -- thought it cannot be done, but I realized
> sms_msg_cancel() needs a _destroy_free(), not just a _destroy(), so that
> settles that last one. Will do the _create() rename too.
> 
> 

Good, can you resubmit this one separately soonish?  I'm touching this
area of the code because of Andrew's changes for STK Send SMS command
handling.

Regards,
-Denis

Re: [SMS D-Bus 08/23] export sms_assembly_encode_address

[Denis Kenzior]

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

Hi Inaky,

On 07/07/2010 06:28 PM, Inaky Perez-Gonzalez wrote:
> On Wed, 2010-07-07 at 15:57 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>>>  void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
>>> +gboolean sms_assembly_encode_address(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);
>>
>> Can we rename this into sms_address_to_hex_string?  That seems to
>> reflect the intent a bit better.
> 
> Ok -- this will then touch a few more other sites in the code.
> 
> 

That is fine.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

Hi Inaky,

On 07/07/2010 06:31 PM, Inaky Perez-Gonzalez wrote:
> On Wed, 2010-07-07 at 16:32 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>> ....
>>> settles that last one. Will do the _create() rename too.
>>>
>>>
>>
>> Good, can you resubmit this one separately soonish?  I'm touching this
>> area of the code because of Andrew's changes for STK Send SMS command
>> handling.
> 
> I'll resubmit the whole series of stuff you haven't merged yet with all
> the feedback incorporated. Should be done tomorrow the latest.
> 

The reason I'm asking is that I was about to do almost the exact same
set of changes.  Tomorrow is fine though.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 16:39 -0700, Denis Kenzior wrote: 
> ...
> >> Good, can you resubmit this one separately soonish?  I'm touching this
> >> area of the code because of Andrew's changes for STK Send SMS command
> >> handling.
> > 
> > I'll resubmit the whole series of stuff you haven't merged yet with all
> > the feedback incorporated. Should be done tomorrow the latest.
> > 
> 
> The reason I'm asking is that I was about to do almost the exact same
> set of changes.  Tomorrow is fine though.

Dennis, I am running behind on this one. The commits have been
regenerated with the feedback you gave -- however, I picked up a bug
when hitting cancel when the message is in _QUEUED state and I still
haven't been able to tackle it. Quite hard to reproduce as the
transition out of _QUEUED happens quite fast; currently working on a
python script to automate the test.

If you want, I can submit with the buglet, but I'd rather *not* do that.

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Thu, 2010-07-08 at 16:38 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/08/2010 06:28 PM, Inaky Perez-Gonzalez wrote:
> > On Wed, 2010-07-07 at 16:39 -0700, Denis Kenzior wrote: 
> >> ...
> >>>> Good, can you resubmit this one separately soonish?  I'm touching this
> >>>> area of the code because of Andrew's changes for STK Send SMS command
> >>>> handling.
> >>>
> >>> I'll resubmit the whole series of stuff you haven't merged yet with all
> >>> the feedback incorporated. Should be done tomorrow the latest.
> >>>
> >>
> >> The reason I'm asking is that I was about to do almost the exact same
> >> set of changes.  Tomorrow is fine though.
> > 
> > Dennis, I am running behind on this one. The commits have been
> > regenerated with the feedback you gave -- however, I picked up a bug
> > when hitting cancel when the message is in _QUEUED state and I still
> > haven't been able to tackle it. Quite hard to reproduce as the
> > transition out of _QUEUED happens quite fast; currently working on a
> > python script to automate the test.
> > 
> > If you want, I can submit with the buglet, but I'd rather *not* do that.
> > 
> 
> No worries, take your time.  Just as a heads up, I pushed some changes
> to src/sms.c today.

Ack, thx. I'll do a rebase before pushing once I am done chasing that
bugger.

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

Hi Inaky,

On 07/08/2010 06:28 PM, Inaky Perez-Gonzalez wrote:
> On Wed, 2010-07-07 at 16:39 -0700, Denis Kenzior wrote: 
>> ...
>>>> Good, can you resubmit this one separately soonish?  I'm touching this
>>>> area of the code because of Andrew's changes for STK Send SMS command
>>>> handling.
>>>
>>> I'll resubmit the whole series of stuff you haven't merged yet with all
>>> the feedback incorporated. Should be done tomorrow the latest.
>>>
>>
>> The reason I'm asking is that I was about to do almost the exact same
>> set of changes.  Tomorrow is fine though.
> 
> Dennis, I am running behind on this one. The commits have been
> regenerated with the feedback you gave -- however, I picked up a bug
> when hitting cancel when the message is in _QUEUED state and I still
> haven't been able to tackle it. Quite hard to reproduce as the
> transition out of _QUEUED happens quite fast; currently working on a
> python script to automate the test.
> 
> If you want, I can submit with the buglet, but I'd rather *not* do that.
> 

No worries, take your time.  Just as a heads up, I pushed some changes
to src/sms.c today.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Thu, 2010-07-08 at 16:38 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/08/2010 06:28 PM, Inaky Perez-Gonzalez wrote:
> > On Wed, 2010-07-07 at 16:39 -0700, Denis Kenzior wrote: 
> >> ...
> >>>> Good, can you resubmit this one separately soonish?  I'm touching this
> >>>> area of the code because of Andrew's changes for STK Send SMS command
> >>>> handling.
> >>>
> >>> I'll resubmit the whole series of stuff you haven't merged yet with all
> >>> the feedback incorporated. Should be done tomorrow the latest.
> >>>
> >>
> >> The reason I'm asking is that I was about to do almost the exact same
> >> set of changes.  Tomorrow is fine though.
> > 
> > Dennis, I am running behind on this one. The commits have been
> > regenerated with the feedback you gave -- however, I picked up a bug
> > when hitting cancel when the message is in _QUEUED state and I still
> > haven't been able to tackle it. Quite hard to reproduce as the
> > transition out of _QUEUED happens quite fast; currently working on a
> > python script to automate the test.
> > 
> > If you want, I can submit with the buglet, but I'd rather *not* do that.
> > 
> 
> No worries, take your time.  Just as a heads up, I pushed some changes
> to src/sms.c today.

Grrr -- found the bug and deal with it. However, when I tried to pull
your tree to rebase on top, I got weird conflicts on stkutil.c -- don't
have time to diagnose now, but just wanted to ask: did you do a rebase?

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

Inaky,

On 07/08/2010 07:03 PM, Inaky Perez-Gonzalez wrote:
> On Thu, 2010-07-08 at 16:38 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>> On 07/08/2010 06:28 PM, Inaky Perez-Gonzalez wrote:
>>> On Wed, 2010-07-07 at 16:39 -0700, Denis Kenzior wrote: 
>>>> ...
>>>>>> Good, can you resubmit this one separately soonish?  I'm touching this
>>>>>> area of the code because of Andrew's changes for STK Send SMS command
>>>>>> handling.
>>>>>
>>>>> I'll resubmit the whole series of stuff you haven't merged yet with all
>>>>> the feedback incorporated. Should be done tomorrow the latest.
>>>>>
>>>>
>>>> The reason I'm asking is that I was about to do almost the exact same
>>>> set of changes.  Tomorrow is fine though.
>>>
>>> Dennis, I am running behind on this one. The commits have been
>>> regenerated with the feedback you gave -- however, I picked up a bug
>>> when hitting cancel when the message is in _QUEUED state and I still
>>> haven't been able to tackle it. Quite hard to reproduce as the
>>> transition out of _QUEUED happens quite fast; currently working on a
>>> python script to automate the test.
>>>
>>> If you want, I can submit with the buglet, but I'd rather *not* do that.
>>>
>>
>> No worries, take your time.  Just as a heads up, I pushed some changes
>> to src/sms.c today.
> 
> Grrr -- found the bug and deal with it. However, when I tried to pull
> your tree to rebase on top, I got weird conflicts on stkutil.c -- don't
> have time to diagnose now, but just wanted to ask: did you do a rebase?
> 
> 

Yep.

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Thu, 2010-07-08 at 17:22 -0700, Denis Kenzior wrote: 
> Inaky,
> 
> ....
> >>
> >> No worries, take your time.  Just as a heads up, I pushed some changes
> >> to src/sms.c today.
> > 
> > Grrr -- found the bug and deal with it. However, when I tried to pull
> > your tree to rebase on top, I got weird conflicts on stkutil.c -- don't
> > have time to diagnose now, but just wanted to ask: did you do a rebase? 
> 
> Yep.

Rats. Don't. Rebase. A. Public. Tree. Please! :)

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Denis Kenzior]

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

On 07/09/2010 12:11 PM, Inaky Perez-Gonzalez wrote:
> On Thu, 2010-07-08 at 17:22 -0700, Denis Kenzior wrote: 
>> Inaky,
>>
>> ....
>>>>
>>>> No worries, take your time.  Just as a heads up, I pushed some changes
>>>> to src/sms.c today.
>>>
>>> Grrr -- found the bug and deal with it. However, when I tried to pull
>>> your tree to rebase on top, I got weird conflicts on stkutil.c -- don't
>>> have time to diagnose now, but just wanted to ask: did you do a rebase? 
>>
>> Yep.
> 
> Rats. Don't. Rebase. A. Public. Tree. Please! :)
> 
> 

Maybe we're not talking about the same thing.  My workflow is

git commit / git am
git fetch
git rebase
git push

Regards,
-Denis

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Fri, 2010-07-09 at 10:19 -0700, Denis Kenzior wrote: 
> On 07/09/2010 12:11 PM, Inaky Perez-Gonzalez wrote:
> > On Thu, 2010-07-08 at 17:22 -0700, Denis Kenzior wrote: 
> >> Inaky,
> >>
> >> ....
> >>>>
> >>>> No worries, take your time.  Just as a heads up, I pushed some changes
> >>>> to src/sms.c today.
> >>>
> >>> Grrr -- found the bug and deal with it. However, when I tried to pull
> >>> your tree to rebase on top, I got weird conflicts on stkutil.c -- don't
> >>> have time to diagnose now, but just wanted to ask: did you do a rebase? 
> >>
> >> Yep.
> > 
> > Rats. Don't. Rebase. A. Public. Tree. Please! :)
> > 
> > 
> 
> Maybe we're not talking about the same thing.  My workflow is
> 
> git commit / git am
> git fetch
> git rebase
> git push

gee, didn't see this -- well, if the rebase is changing the commit IDs,
which it probably does, then you are breaking the commit history. Would
depend on the details.

Anyway, I am not so sure that is the problem. Spent the whole day going
around it and I still couldn't figure the sucker out. Will keep trying.

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Marcel Holtmann]

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

Hi Inaky,

> > Maybe we're not talking about the same thing.  My workflow is
> > 
> > git commit / git am
> > git fetch
> > git rebase
> > git push
> 
> gee, didn't see this -- well, if the rebase is changing the commit IDs,
> which it probably does, then you are breaking the commit history. Would
> depend on the details.
> 
> Anyway, I am not so sure that is the problem. Spent the whole day going
> around it and I still couldn't figure the sucker out. Will keep trying.

the ofono.git master tree never got re-based. All commit ids are fully
atomic. So that is not the problem.

Regards

Marcel

Re: [SMS D-Bus 11/23] struct tx_queue_entry: add fields and destructor

[Inaky Perez-Gonzalez]

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

On Wed, 2010-07-07 at 16:32 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> On 07/07/2010 06:24 PM, Inaky Perez-Gonzalez wrote:
> > On Wed, 2010-07-07 at 16:04 -0700, Denis Kenzior wrote: 
> >> Hi Inaky,
> >>
> >> <snip>
> >>
> >> ....
> > 
> >>>  	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;
> >>>  	}
> >>
> >> Can we simply unify the two functions and simply call it
> >> tx_queue_entry_free?  For symmetry renaming create_tx_queue_entry to
> >> tx_queue_entry_new would be nice.
> > 
> > Makes sense -- thought it cannot be done, but I realized
> > sms_msg_cancel() needs a _destroy_free(), not just a _destroy(), so that
> > settles that last one. Will do the _create() rename too. 
> 
> Good, can you resubmit this one separately soonish?  I'm touching this
> area of the code because of Andrew's changes for STK Send SMS command
> handling.

Sigh, your changes broke havoc on mine, as they completely change the
code flow. I started trying to integrate Friday after giving up on that
rebase (or not) issue and just applying commits as patches, but it is
almost like starting from scratch. 

Heads up, this means I'll have to retest the heck out of it and yet
another delay once I figure out how to reshape everything for the new
code flow.