[RFC patches 00/13] misc cleanups and SMS ID API

[RFC patches 00/13] misc cleanups and SMS ID API

[Inaky Perez-Gonzalez]

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

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

Hi All

This set of patches introduce:

 - a few cleanups, code documentations nippets, .gitignore additions,
   header file dependency fixes in smutil.h 

 - adds BUILD_BUG_ON() [linux kernel style], printf-like attribute
   checking for {write,read}_file()

 - make write_file() transaction-safe

 - Introduce the SMS ID API (rfc, still incomplete).

feedback?

Thanks!


The following changes since commit 6b0f2328c5e190053f6a28ed119d01581e2119b3:
  Denis Kenzior (1):
        Update TODO

are available in the git repository at:

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

Patches follow for reviewing convenience.

Inaky Perez-Gonzalez (13):
      Update .gitignore to ignore cscope databases and backup files
      sms_send_message: add a short roadmap
      documentation: add note about referencing standards
      sms_assembly_add_fragment_backup: clarify how insertion spot is found
      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
      storage: add __attribute__((format)) to {write,read}_file() for printf-like variable arg verification
      Add function doc headers to ofono_sms_{create,register}
      doc: explain debugging options to -d, add a pointer in -h to manpage
      automake: fix installation of udev rules in VPATH builds
      SMS: introduce message ID API
      SMS: implement SHA256-based message IDs [incomplete]

 .gitignore             |    3 +
 HACKING                |   10 +++
 Makefile.am            |    7 +-
 doc/ofonod.8           |    5 +-
 doc/standards.txt      |    8 ++
 src/main.c             |    4 +-
 src/sms.c              |   69 +++++++++++++---
 src/smsutil.c          |  197 ++++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h          |   92 +++++++++++++++++++++
 src/storage.c          |   42 +++++++---
 src/storage.h          |    6 +-
 src/util.h             |   29 +++++++
 unit/test-sms-msg-id.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++
 13 files changed, 657 insertions(+), 27 deletions(-)
 create mode 100644 doc/standards.txt
 create mode 100644 unit/test-sms-msg-id.c

[RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files

[Inaky Perez-Gonzalez]

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

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

---
 .gitignore |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 1ce7067..d7e9254 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,3 +1,4 @@
+*~
 *.o
 *.lo
 *.la
@@ -16,6 +17,8 @@ config.sub
 configure
 depcomp
 compile
+cscope.files
+cscope.out
 install-sh
 libtool
 ltmain.sh
-- 
1.6.6.1

[RFC patches 02/13] sms_send_message: add a short roadmap

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/sms.c b/src/sms.c
index 3a1cff0..594481e 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -398,6 +398,19 @@ 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()]
+ *
+ * @conn: D-BUS connection
+ * @msg: message data (telephone number and text)
+ * @data: SMS object to use for transmision
+ *
+ * An alphabet is chosen for the text and it (might be) segmented in
+ * fragments by sms_text_prepare() into @msg_list. A queue list @entry
+ * is created by create_tx_queue_entry() and g_queue_push_tail()
+ * appends that entry to the SMS transmit queue. Then the tx_next()
+ * function is scheduled to run to process the queue.
+ */
 static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 					void *data)
 {
-- 
1.6.6.1

[RFC patches 03/13] 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

[RFC patches 04/13] sms_assembly_add_fragment_backup: clarify how insertion spot is found

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/smsutil.c b/src/smsutil.c
index 17e0e0e..c57fe8b 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2480,6 +2480,11 @@ static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
 		if (node->bitmap[offset] & bit)
 			return NULL;
 
+		/* Iterate over the bitmap to find in which position
+		 * should the fragment be inserted -- basically we
+		 * walk each bit in the bitmap until the bit we care
+		 * about (offset:bit) and count which are stored --
+		 * that gives us in which position we have to insert. */
 		position = 0;
 		for (i = 0; i < offset; i++)
 			for (j = 0; j < 32; j++)
-- 
1.6.6.1

[RFC patches 05/13] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1785 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 |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/src/util.h b/src/util.h
index 2835b76..cf34b67 100644
--- a/src/util.h
+++ b/src/util.h
@@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset,
 				long *items_written, unsigned char terminator);
 
 char *sim_string_to_utf8(const unsigned char *buffer, int 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

[RFC patches 06/13] smutil.h: add missing header file dependencies

[Inaky Perez-Gonzalez]

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

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

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

diff --git a/src/smsutil.h b/src/smsutil.h
index 469a49e..356ec5d 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
 
@@ -501,3 +509,5 @@ char *cbs_topic_ranges_to_string(GSList *ranges);
 GSList *cbs_extract_topic_ranges(const char *ranges);
 GSList *cbs_optimize_ranges(GSList *ranges);
 gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges);
+
+#endif /* #ifndef __smsutil_h__ */
-- 
1.6.6.1

[RFC patches 07/13] 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 618c111..157329f 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

[RFC patches 08/13] storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification

[Inaky Perez-Gonzalez]

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

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

---
 src/storage.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/storage.h b/src/storage.h
index d74de35..74cbba5 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -30,10 +30,12 @@
 int create_dirs(const char *filename, const mode_t mode);
 
 ssize_t read_file(unsigned char *buffer, size_t len,
-			const char *path_fmt, ...);
+			const char *path_fmt, ...)
+	__attribute__((format(printf, 3, 4)));
 
 ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode,
-			const char *path_fmt, ...);
+			const char *path_fmt, ...)
+	__attribute__((format(printf, 4, 5)));
 
 GKeyFile *storage_open(const char *imsi, const char *store);
 void storage_sync(const char *imsi, const char *store, GKeyFile *keyfile);
-- 
1.6.6.1

[RFC patches 09/13] Add function doc headers to ofono_sms_{create, register}

[Inaky Perez-Gonzalez]

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

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

Signed-off-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
---
 src/sms.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 594481e..26b1653 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -910,6 +910,17 @@ static void sms_remove(struct ofono_atom *atom)
 	g_free(sms);
 }
 
+
+/*
+ * Create a SMS driver
+ *
+ * This creates a SMS driver that is hung off a @modem
+ * object. However, for the driver to be used by the system, it has to
+ * be registered with the oFono core using ofono_sms_register().
+ *
+ * This is done once the modem driver determines that SMS is properly
+ * supported by the hardware.
+ */
 struct ofono_sms *ofono_sms_create(struct ofono_modem *modem,
 					unsigned int vendor,
 					const char *driver,
@@ -980,6 +991,14 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi)
 
 }
 
+
+/*
+ * Indicate oFono that a SMS driver is ready for operation
+ *
+ * This is called after ofono_sms_create() was done and the modem
+ * driver determined that a modem supports SMS correctly. Once this
+ * call succeeds, the D-BUS interface for SMS goes live.
+ */
 void ofono_sms_register(struct ofono_sms *sms)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
-- 
1.6.6.1

[RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2200 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 8e686ac..c5791be 100644
--- a/src/main.c
+++ b/src/main.c
@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
 
 static GOptionEntry options[] = {
 	{ "debug", 'd', 0, G_OPTION_ARG_STRING, &option_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

[RFC patches 11/13] automake: fix installation of udev rules in VPATH builds

[Inaky Perez-Gonzalez]

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

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

When the build directory is different than the source directory, we
need to specify the source prefix to the original file we are
copying.
---
 Makefile.am |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index ed13346..31c157c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -418,7 +418,7 @@ src/ofono.ver: src/ofono.exp
 	$(AM_V_at)echo "local: *; };" >> $@
 
 plugins/%.rules:
-	$(AM_V_GEN)cp $(subst 97-,,$@) $@
+	$(AM_V_GEN)cp $(srcdir)/$(subst 97-,,$@) $@
 
 $(src_ofonod_OBJECTS) $(unit_objects): $(local_headers)
 
-- 
1.6.6.1

[RFC patches 12/13] SMS: introduce message ID API

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 17308 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          |  192 +++++++++++++++++++++++++++++++++++++++++++
 src/smsutil.h          |   82 +++++++++++++++++++
 unit/test-sms-msg-id.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 490 insertions(+), 1 deletions(-)
 create mode 100644 unit/test-sms-msg-id.c

diff --git a/Makefile.am b/Makefile.am
index 31c157c..f821305 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -345,7 +345,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@
@@ -384,6 +384,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 c57fe8b..ecb9acd 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -3702,3 +3702,195 @@ gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges)
 	return g_slist_find_custom(ranges, GUINT_TO_POINTER(topic),
 					cbs_topic_compare) != NULL;
 }
+
+
+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 356ec5d..199d3e6 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -510,4 +510,86 @@ GSList *cbs_extract_topic_ranges(const char *ranges);
 GSList *cbs_optimize_ranges(GSList *ranges);
 gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges);
 
+
+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

[RFC patches 13/13] SMS: implement SHA256-based message IDs [incomplete]

[Inaky Perez-Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 3988 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: not yet complete, need to figure out how to do so that
identical messages sent to the same number also have a different ID.
---
 src/sms.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 26b1653..24539cd 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;
 	time_t last_mms;
@@ -81,6 +80,7 @@ struct tx_queue_entry {
 	unsigned char num_pdus;
 	unsigned char cur_pdu;
 	unsigned int msg_id;
+	struct sms_msg_id sms_msg_id;
 	unsigned int retry;
 	DBusMessage *msg;
 };
@@ -421,6 +421,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	int ref_offset;
 	struct tx_queue_entry *entry;
 	struct ofono_modem *modem;
+	DECLARE_SMS_MSG_ID_STRBUF(msg_id_str);
 
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
@@ -451,7 +452,17 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	}
 
 	entry->msg = dbus_message_ref(msg);
-	entry->msg_id = sms->next_msg_id++;
+	sms_msg_id_init(&entry->sms_msg_id);
+	sms_msg_id_hash(&entry->sms_msg_id, text, strlen(text));
+	sms_msg_id_hash(&entry->sms_msg_id, to, strlen(to));
+#warning FIXME: add source time or something? otherwise repeats are the same; using ref for now, doesn't really work
+	sms_msg_id_hash(&entry->sms_msg_id, &sms->ref, sizeof(sms->ref));
+	sms_msg_id_hash(&entry->sms_msg_id, NULL, 0);
+	sms_msg_id_memcpy(&entry->msg_id, sizeof(entry->msg_id),
+			  &entry->sms_msg_id);
+	sms_msg_id_printf(&entry->sms_msg_id, msg_id_str, sizeof(msg_id_str));
+	ofono_debug("SMS TX: ref %08x, msg id: %s\n",
+		    sms->ref, msg_id_str);
 
 	g_queue_push_tail(sms->txq, entry);
 
@@ -508,6 +519,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;
@@ -549,11 +563,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));
+#warning 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)
@@ -896,8 +917,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);
 
 		storage_close(sms->imsi, SETTINGS_STORE, sms->settings, TRUE);
@@ -981,8 +1000,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);
 
-- 
1.6.6.1

Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files

[Marcel Holtmann]

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

Hi Inaky,

> diff --git a/.gitignore b/.gitignore
> index 1ce7067..d7e9254 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,3 +1,4 @@
> +*~

believe it or not, I always left that out on purpose. I like having git
status show me all the stupid backup files my editor creates.

>  *.o
>  *.lo
>  *.la
> @@ -16,6 +17,8 @@ config.sub
>  configure
>  depcomp
>  compile
> +cscope.files
> +cscope.out
>  install-sh
>  libtool
>  ltmain.sh

Please put the cscope stuff behind autom4te.cache since it is not part
of autoconf/automake.

Regards

Marcel

Re: [RFC patches 02/13] sms_send_message: add a short roadmap

[Marcel Holtmann]

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

Hi Inaky,

> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> ---
>  src/sms.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/src/sms.c b/src/sms.c
> index 3a1cff0..594481e 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -398,6 +398,19 @@ 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()]
> + *
> + * @conn: D-BUS connection
> + * @msg: message data (telephone number and text)
> + * @data: SMS object to use for transmision
> + *
> + * An alphabet is chosen for the text and it (might be) segmented in
> + * fragments by sms_text_prepare() into @msg_list. A queue list @entry
> + * is created by create_tx_queue_entry() and g_queue_push_tail()
> + * appends that entry to the SMS transmit queue. Then the tx_next()
> + * function is scheduled to run to process the queue.
> + */
>  static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  					void *data)
>  {

small comments here. So it is D-Bus. That is the official naming in
documentation we settled on some time ago. Half of the documentation
might be still using it wrongly, but we might wanna set a good example
here.

I don't know how picky gtk-doc is about not having the : for title and
the empty line after the title. However this is nicely readable inside
the source code, so I am fine with it.

Regards

Marcel

Re: [RFC patches 03/13] documentation: add note about referencing standards

[Marcel Holtmann]

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

Hi Inaky,

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

would this not better fit into HACKING document. I don't really have a
strong opinion here. Just asking.

Regards

Marcel

Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files

[=?unknown-8bit?q?Jo=C3=A3o?= Paulo Rechi Vita]

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

On Fri, May 28, 2010 at 11:08, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Inaky,
>
>> diff --git a/.gitignore b/.gitignore
>> index 1ce7067..d7e9254 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -1,3 +1,4 @@
>> +*~
>
> believe it or not, I always left that out on purpose. I like having git
> status show me all the stupid backup files my editor creates.
>
>>  *.o
>>  *.lo
>>  *.la
>> @@ -16,6 +17,8 @@ config.sub
>>  configure
>>  depcomp
>>  compile
>> +cscope.files
>> +cscope.out
>>  install-sh
>>  libtool
>>  ltmain.sh
>
> Please put the cscope stuff behind autom4te.cache since it is not part
> of autoconf/automake.
>

Does this makes sense since this files are not related to the project
itself but to specific tools? I usually put those on my personal
gitignore file.

-- 
João Paulo Rechi Vita
http://jprvita.wordpress.com/

Re: [RFC patches 05/13] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 2107 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 |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/src/util.h b/src/util.h
> index 2835b76..cf34b67 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset,
>  				long *items_written, unsigned char terminator);
>  
>  char *sim_string_to_utf8(const unsigned char *buffer, int 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.
> + */

sounds like a nice addition, but src/util.h is the wrong file. I prefer
we make this compatible so that it can be applied to ConnMan and BlueZ
as well.

I think something like include/bug.h is better.

Regards

Marcel

Re: [RFC patches 06/13] smutil.h: add missing header file dependencies

[Marcel Holtmann]

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

Hi Inaky,

> diff --git a/src/smsutil.h b/src/smsutil.h
> index 469a49e..356ec5d 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__

please leave the circular dependency protection out here. I really want
it to fail if we go so crazy and would require it.

Regards

Marcel

Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage

[Marcel Holtmann]

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

Hi Inaky,

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

you need to hook this up to automake :)

> diff --git a/src/main.c b/src/main.c
> index 8e686ac..c5791be 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
>  
>  static GOptionEntry options[] = {
>  	{ "debug", 'd', 0, G_OPTION_ARG_STRING, &option_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" },

Please leave this out. If we have a man page and man ofonod works, then
this is not needed.

Regards

Marcel

Re: [RFC patches 11/13] automake: fix installation of udev rules in VPATH builds

[Marcel Holtmann]

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

Hi Inaky,

> When the build directory is different than the source directory, we
> need to specify the source prefix to the original file we are
> copying.
> ---
>  Makefile.am |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index ed13346..31c157c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -418,7 +418,7 @@ src/ofono.ver: src/ofono.exp
>  	$(AM_V_at)echo "local: *; };" >> $@
>  
>  plugins/%.rules:
> -	$(AM_V_GEN)cp $(subst 97-,,$@) $@
> +	$(AM_V_GEN)cp $(srcdir)/$(subst 97-,,$@) $@
>  
>  $(src_ofonod_OBJECTS) $(unit_objects): $(local_headers)

I tested it within my release build system and it looks fine to me. At
least nothing breaks. I leave it up to Denis to apply the patch, but
please also submit similar patches for BlueZ and ConnMan since I am
pretty sure I missed it there as well.

Regards

Marcel

Re: [RFC patches 04/13] sms_assembly_add_fragment_backup: clarify how insertion spot is found

[Denis Kenzior]

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

Hi Inaky,

> +		/* Iterate over the bitmap to find in which position
> +		 * should the fragment be inserted -- basically we
> +		 * walk each bit in the bitmap until the bit we care
> +		 * about (offset:bit) and count which are stored --
> +		 * that gives us in which position we have to insert. */

Patch has been applied.  However please keep your commit header to 50 
characters or less.  Also, the preferred style for multi-line comments is:
/* <empty line>
 * ... comment
 */

Regards,
-Denis

Re: [RFC patches 12/13] SMS: introduce message ID API

[Marcel Holtmann]

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

Hi Inaky,

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

can we just stick with gtk-doc like you had in your previous patches. I
think intermixing it with doxygen is a bad idea.

Regards

Marcel

Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files

[Marcel Holtmann]

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

Hi Joao,

> >> diff --git a/.gitignore b/.gitignore
> >> index 1ce7067..d7e9254 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> >> @@ -1,3 +1,4 @@
> >> +*~
> >
> > believe it or not, I always left that out on purpose. I like having git
> > status show me all the stupid backup files my editor creates.
> >
> >>  *.o
> >>  *.lo
> >>  *.la
> >> @@ -16,6 +17,8 @@ config.sub
> >>  configure
> >>  depcomp
> >>  compile
> >> +cscope.files
> >> +cscope.out
> >>  install-sh
> >>  libtool
> >>  ltmain.sh
> >
> > Please put the cscope stuff behind autom4te.cache since it is not part
> > of autoconf/automake.
> >
> 
> Does this makes sense since this files are not related to the project
> itself but to specific tools? I usually put those on my personal
> gitignore file.

actually I have to agree. They really don't belong in the project and
the personal gitignore configuration is a suitable place for them.

Regards

Marcel

Re: [RFC patches 06/13] smutil.h: add missing header file dependencies

[Denis Kenzior]

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

Hi Inaky,

> +#include <glib/gtypes.h>
> +#include <glib/gslist.h>
> +#include <glib/gmessages.h>
> +#include <glib/gchecksum.h>

Isn't it sufficient to just add glib/glib.h here?

Regards,
-Denis

Re: [RFC patches 02/13] sms_send_message: add a short roadmap

[Denis Kenzior]

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

Hi Inaky,

> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> ---
>  src/sms.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 

Applied with s/D-BUS/D-Bus

Regards,
-Denis 

Re: [RFC patches 07/13] write_file: make transaction-safe

[Denis Kenzior]

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

Hi Inaky,

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

Again, prefer multiline comments to be in a certain format

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

And I get trouble applying your patch:
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.


Regards,
-Denis

Re: [RFC patches 08/13] storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification

[Denis Kenzior]

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

Hi Inaky,

> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> ---
>  src/storage.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 

Patch has been applied with a modified commit message.

Thanks,
-Denis

Re: [RFC patches 09/13] Add function doc headers to ofono_sms_{create, register}

[Denis Kenzior]

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

Hi Inaky,

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

We don't use signed off lines here, please don't include them in the future.

Patch has been applied with a modified commit message.

Thanks,
-Denis

Re: [RFC patches 11/13] automake: fix installation of udev rules in VPATH builds

[Denis Kenzior]

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

Hi Inaky,

> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> When the build directory is different than the source directory, we
> need to specify the source prefix to the original file we are
> copying.

Patch has been applied with a modified commit message.

Thanks,
-Denis

Re: [RFC patches 02/13] sms_send_message: add a short roadmap

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:11 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > ---
> >  src/sms.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/sms.c b/src/sms.c
> > index 3a1cff0..594481e 100644
> > --- a/src/sms.c
> > +++ b/src/sms.c
> > @@ -398,6 +398,19 @@ 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()]
> > + *
> > + * @conn: D-BUS connection
> > + * @msg: message data (telephone number and text)
> > + * @data: SMS object to use for transmision
> > + *
> > + * An alphabet is chosen for the text and it (might be) segmented in
> > + * fragments by sms_text_prepare() into @msg_list. A queue list @entry
> > + * is created by create_tx_queue_entry() and g_queue_push_tail()
> > + * appends that entry to the SMS transmit queue. Then the tx_next()
> > + * function is scheduled to run to process the queue.
> > + */
> >  static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  					void *data)
> >  {
> 
> small comments here. So it is D-Bus. That is the official naming in
> documentation we settled on some time ago. Half of the documentation
> might be still using it wrongly, but we might wanna set a good example
> here.

Ack to that -- I'll do a S&R for all of those.

> I don't know how picky gtk-doc is about not having the : for title and
> the empty line after the title. However this is nicely readable inside
> the source code, so I am fine with it.

Actually I am thinking gtk-doc is probably not the best idea to settle
on...it's quite a pain to generate and quite limited. More on this
later.

Re: [RFC patches 03/13] documentation: add note about referencing standards

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:13 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > 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.
> 
> would this not better fit into HACKING document. I don't really have a
> strong opinion here. Just asking.

This was Denis' request to have it as separate. I think initially I had
put it on HACKING or README? It fits me in any, as long as it is easy to
find.

Re: [RFC patches 05/13] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:17 -0700, Marcel Holtmann 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 |   29 +++++++++++++++++++++++++++++
> >  1 files changed, 29 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/util.h b/src/util.h
> > index 2835b76..cf34b67 100644
> > --- a/src/util.h
> > +++ b/src/util.h
> > @@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset,
> >  				long *items_written, unsigned char terminator);
> >  
> >  char *sim_string_to_utf8(const unsigned char *buffer, int 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.
> > + */
> 
> sounds like a nice addition, but src/util.h is the wrong file. I prefer
> we make this compatible so that it can be applied to ConnMan and BlueZ
> as well.
> 
> I think something like include/bug.h is better.

Sounds ok to me. Denis?

Re: [RFC patches 06/13] smutil.h: add missing header file dependencies

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:19 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > diff --git a/src/smsutil.h b/src/smsutil.h
> > index 469a49e..356ec5d 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__
> 
> please leave the circular dependency protection out here. I really want
> it to fail if we go so crazy and would require it.

I personally NAK here, but if you guys don't like it, I'll just drop
this guy from the series. Not that critical.

Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:21 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > 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.
> 
> you need to hook this up to automake :)

Can you clarify, please?

> > diff --git a/src/main.c b/src/main.c
> > index 8e686ac..c5791be 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
> >  
> >  static GOptionEntry options[] = {
> >  	{ "debug", 'd', 0, G_OPTION_ARG_STRING, &option_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" },
> 
> Please leave this out. If we have a man page and man ofonod works, then
> this is not needed.

Denis and me, AFAIR, agreed on this wording as it just gives the quick
pointer to where the extra information is found. Again, not critical,
but confusing feedback.

Re: [RFC patches 04/13] sms_assembly_add_fragment_backup: clarify how insertion spot is found

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:27 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +		/* Iterate over the bitmap to find in which position
> > +		 * should the fragment be inserted -- basically we
> > +		 * walk each bit in the bitmap until the bit we care
> > +		 * about (offset:bit) and count which are stored --
> > +		 * that gives us in which position we have to insert. */
> 
> Patch has been applied.  However please keep your commit header to 50 
> characters or less.  Also, the preferred style for multi-line comments is:
> /* <empty line>
>  * ... comment
>  */

Ops, sorry -- I'll go fix those and provide updates for it.

Re: [RFC patches 12/13] SMS: introduce message ID API

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:27 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > +/**
> > + * 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).
> > + */
> 
> can we just stick with gtk-doc like you had in your previous patches. I
> think intermixing it with doxygen is a bad idea.

Yep, my bad, sorry.

In any case, picking this up from earlier:

I have my doubts on gtk-doc vs doxygen. I remember there was a thread
somewhere (email? irc? voice?) on which system to settle on which never
got finalized. I am not too satisfied with gtk-doc's limitations, I'd
rather do doxygen. Does anyone have any preference on once vs the other?

In any case, when we settle on one, I'll have to go and cleanup all
these to conform to it. I'll also write a quick summary with examples on
how to use it in doc/.

Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:29 -0700, Marcel Holtmann wrote: 
> Hi Joao,
> 
> > >> diff --git a/.gitignore b/.gitignore
> > >> index 1ce7067..d7e9254 100644
> > >> --- a/.gitignore
> > >> +++ b/.gitignore
> > >> @@ -1,3 +1,4 @@
> > >> +*~
> > >
> > > believe it or not, I always left that out on purpose. I like having git
> > > status show me all the stupid backup files my editor creates.
> > >
> > >>  *.o
> > >>  *.lo
> > >>  *.la
> > >> @@ -16,6 +17,8 @@ config.sub
> > >>  configure
> > >>  depcomp
> > >>  compile
> > >> +cscope.files
> > >> +cscope.out
> > >>  install-sh
> > >>  libtool
> > >>  ltmain.sh
> > >
> > > Please put the cscope stuff behind autom4te.cache since it is not part
> > > of autoconf/automake.
> > >
> > 
> > Does this makes sense since this files are not related to the project
> > itself but to specific tools? I usually put those on my personal
> > gitignore file.
> 
> actually I have to agree. They really don't belong in the project and
> the personal gitignore configuration is a suitable place for them.

ACK, I'll drop this one then

Re: [RFC patches 06/13] smutil.h: add missing header file dependencies

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:31 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > +#include <glib/gtypes.h>
> > +#include <glib/gslist.h>
> > +#include <glib/gmessages.h>
> > +#include <glib/gchecksum.h>
> 
> Isn't it sufficient to just add glib/glib.h here?

You can definitely do that, but it also increases the compilation
time...not that you or I would notice anyway.

Re: [RFC patches 02/13] sms_send_message: add a short roadmap

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:34 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > ---
> >  src/sms.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> 
> Applied with s/D-BUS/D-Bus

Thank you for applying, but please *don't* do it like that.

Complain and ask me to fix it, I'll be happy to comply and resend, but
by modifying the commit yourself you drive git crazy and create
conflicts between my commits and yours.

Now I have to go for a time-consuming process of finding which ones are
and working around it. You might have saved some of your time in doing
that, but it is way less that the one I am going to use on resolving the
mess on each commit *plus* I don't get to really learn / set on stone
what your preferences are so these review round trips become less
frequent in the long term. We both loose here.

Re: [RFC patches 07/13] write_file: make transaction-safe

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:43 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > 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.
> > +	/* 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. */
> 
> Again, prefer multiline comments to be in a certain format

I'll fix this and resubmit.

> > +	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;
> >  }
> > 
> 
> And I get trouble applying your patch:
> 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.

Ops, my bad. Ditto, I'll fix and resubmit.

Re: [RFC patches 08/13] storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:45 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > ---
> >  src/storage.h |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> 
> Patch has been applied with a modified commit message.

Thanks.

Recording the same complain as before wrt to modifying commits
contributed by others. Please mention which changes you want, let me do
them and resubmit.

Re: [RFC patches 09/13] Add function doc headers to ofono_sms_{create, register}

[Inaky Perez-Gonzalez]

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

On Fri, 2010-05-28 at 07:47 -0700, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> > 
> > Signed-off-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> We don't use signed off lines here, please don't include them in the future.

Grr, sorry, my autopilot from the kernel keeps adding -s to 'git
commit'. I added a tripcheck for it.

> Patch has been applied with a modified commit message.

Same complaint as before wrt to committing modified patches.

Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage

[Marcel Holtmann]

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

Hi Inaky,

> > > 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.
> > 
> > you need to hook this up to automake :)
> 
> Can you clarify, please?

we do wanna install the man pages, right? Then this needs man_MANS and
EXTRA_DIST magic to get included and installed.

> > > diff --git a/src/main.c b/src/main.c
> > > index 8e686ac..c5791be 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
> > >  
> > >  static GOptionEntry options[] = {
> > >  	{ "debug", 'd', 0, G_OPTION_ARG_STRING, &option_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" },
> > 
> > Please leave this out. If we have a man page and man ofonod works, then
> > this is not needed.
> 
> Denis and me, AFAIR, agreed on this wording as it just gives the quick
> pointer to where the extra information is found. Again, not critical,
> but confusing feedback.

Maybe I am too old school Linux here ;)

If we have an installed man page and I don't understand the usage
information, then it is clear to just do man ofonod. I don't need the
usage tell me that.

Regards

Marcel

Re: [RFC patches 12/13] SMS: introduce message ID API

[Marcel Holtmann]

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

Hi Inaky,

> > > +/**
> > > + * 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).
> > > + */
> > 
> > can we just stick with gtk-doc like you had in your previous patches. I
> > think intermixing it with doxygen is a bad idea.
> 
> Yep, my bad, sorry.
> 
> In any case, picking this up from earlier:
> 
> I have my doubts on gtk-doc vs doxygen. I remember there was a thread
> somewhere (email? irc? voice?) on which system to settle on which never
> got finalized. I am not too satisfied with gtk-doc's limitations, I'd
> rather do doxygen. Does anyone have any preference on once vs the other?
> 
> In any case, when we settle on one, I'll have to go and cleanup all
> these to conform to it. I'll also write a quick summary with examples on
> how to use it in doc/.

my preference with gtk-doc is that it generates nicer documentation and
also makes devhelp files available.

Regards

Marcel

Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage

[Inaky Perez-Gonzalez]

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

On Tue, 2010-06-08 at 16:13 -0700, Marcel Holtmann wrote: 
> Hi Inaky,
> 
> > > > 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.
> > > 
> > > you need to hook this up to automake :)
> > 
> > Can you clarify, please?
> 
> we do wanna install the man pages, right? Then this needs man_MANS and
> EXTRA_DIST magic to get included and installed.

Hmm, I didn't realize the makefile magic wasn't plugged in--I just
patched the man page.

Ack to that, will add that as soon as I have the tree rebased from the
modified commits.

> > > > diff --git a/src/main.c b/src/main.c
> > > > index 8e686ac..c5791be 100644
> > > > --- a/src/main.c
> > > > +++ b/src/main.c
> > > > @@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
> > > >  
> > > >  static GOptionEntry options[] = {
> > > >  	{ "debug", 'd', 0, G_OPTION_ARG_STRING, &option_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" },
> > > 
> > > Please leave this out. If we have a man page and man ofonod works, then
> > > this is not needed.
> > 
> > Denis and me, AFAIR, agreed on this wording as it just gives the quick
> > pointer to where the extra information is found. Again, not critical,
> > but confusing feedback.
> 
> Maybe I am too old school Linux here ;)
> 
> If we have an installed man page and I don't understand the usage
> information, then it is clear to just do man ofonod. I don't need the
> usage tell me that.

heh -- yeah, I tend to do that too. Anyhow, let's leave it up to Denis.