Write state of SMS assembly to disk and restore on startup.

Write state of SMS assembly to disk and restore on startup.

[Andrzej Zaborowski]

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

This way we can continue receiving segmented messages over a reset or
crash.
---
 src/common.c     |   29 +++++++
 src/common.h     |   10 +++
 src/sim.c        |   34 --------
 src/sms.c        |   14 +++-
 src/smsutil.c    |  237 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/smsutil.h    |    3 +-
 unit/Makefile.am |    6 +-
 unit/test-sms.c  |    4 +-
 8 files changed, 293 insertions(+), 44 deletions(-)

diff --git a/src/common.c b/src/common.c
index 14bf7f9..e1d560e 100644
--- a/src/common.c
+++ b/src/common.c
@@ -26,6 +26,7 @@
 #define _GNU_SOURCE
 #include <string.h>
 #include <ctype.h>
+#include <errno.h>
 
 #include <glib.h>
 
@@ -593,3 +594,31 @@ gboolean is_valid_pin(const char *pin)
 
 	return TRUE;
 }
+
+int create_dirs(const char *filename, const mode_t mode)
+{
+	struct stat st;
+	char *dir;
+	const char *prev, *next;
+	int err;
+
+	err = stat(filename, &st);
+	if (!err && S_ISREG(st.st_mode))
+		return 0;
+
+	dir = g_malloc(strlen(filename) + 1);
+	strcpy(dir, "/");
+
+	for (prev = filename; (next = strchr(prev + 1, '/')); prev = next)
+		if (next > prev + 1) {
+			strncat(dir, prev + 1, next - prev);
+
+			if (mkdir(dir, mode) && errno != EEXIST) {
+				g_free(dir);
+				return -1;
+			}
+		}
+
+	g_free(dir);
+	return 0;
+}
diff --git a/src/common.h b/src/common.h
index 3805e21..18fb0c5 100644
--- a/src/common.h
+++ b/src/common.h
@@ -135,3 +135,13 @@ const char *ss_control_type_to_string(enum ss_control_type type);
 const char *bearer_class_to_string(enum bearer_class cls);
 
 gboolean is_valid_pin(const char *pin);
+
+#ifdef TEMP_FAILURE_RETRY
+#define TFR TEMP_FAILURE_RETRY
+#else
+#define TFR
+#endif
+
+#include <fcntl.h>
+
+int create_dirs(const char *filename, const mode_t mode);
diff --git a/src/sim.c b/src/sim.c
index d4387c9..cd608f7 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -44,12 +44,6 @@
 #include "sim.h"
 #include "simutil.h"
 
-#ifdef TEMP_FAILURE_RETRY
-#define TFR TEMP_FAILURE_RETRY
-#else
-#define TFR
-#endif
-
 #define SIM_MANAGER_INTERFACE "org.ofono.SimManager"
 
 #define SIM_CACHE_MODE 0600
@@ -515,34 +509,6 @@ static void sim_retrieve_imsi(struct ofono_modem *modem)
 	sim->ops->read_imsi(modem, sim_imsi_cb, modem);
 }
 
-static int create_dirs(const char *filename, const mode_t mode)
-{
-	struct stat st;
-	char *dir;
-	const char *prev, *next;
-	int err;
-
-	err = stat(filename, &st);
-	if (!err && S_ISREG(st.st_mode))
-		return 0;
-
-	dir = g_malloc(strlen(filename) + 1);
-	strcpy(dir, "/");
-
-	for (prev = filename; (next = strchr(prev + 1, '/')); prev = next)
-		if (next > prev + 1) {
-			strncat(dir, prev + 1, next - prev);
-
-			if (mkdir(dir, mode) && errno != EEXIST) {
-				g_free(dir);
-				return -1;
-			}
-		}
-
-	g_free(dir);
-	return 0;
-}
-
 static void sim_op_error(struct ofono_modem *modem)
 {
 	struct sim_manager_data *sim = modem->sim_manager;
diff --git a/src/sms.c b/src/sms.c
index c7d83fa..0781f86 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -72,7 +72,6 @@ static struct sms_manager_data *sms_manager_create()
 	sms->sca.type = 129;
 	sms->ref = 1;
 
-	sms->assembly = sms_assembly_new();
 	sms->txq = g_queue_new();
 
 	return sms;
@@ -792,6 +791,14 @@ void ofono_sms_status_notify(struct ofono_modem *modem, unsigned char *pdu,
 	ofono_error("SMS Status-Report not yet handled");
 }
 
+static void sms_got_imsi(struct ofono_modem *modem)
+{
+	const char *imsi = ofono_sim_get_imsi(modem);
+	struct sms_manager_data *sms = modem->sms_manager;
+
+	sms->assembly = sms_assembly_new(imsi);
+}
+
 int ofono_sms_manager_register(struct ofono_modem *modem,
 					struct ofono_sms_ops *ops)
 {
@@ -830,6 +837,11 @@ int ofono_sms_manager_register(struct ofono_modem *modem,
 
 	ofono_modem_add_interface(modem, SMS_MANAGER_INTERFACE);
 
+	if (ofono_sim_get_ready(modem))
+		sms_got_imsi(modem);
+	else
+		ofono_sim_ready_notify_register(modem, sms_got_imsi);
+
 	return 0;
 }
 
diff --git a/src/smsutil.c b/src/smsutil.c
index fcff9aa..2480d71 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -23,11 +23,20 @@
 #include <config.h>
 #endif
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <glib.h>
-
+#include <dirent.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "driver.h"
+#include "common.h"
 #include "util.h"
 #include "smsutil.h"
 
@@ -2106,9 +2115,211 @@ char *sms_decode_text(GSList *sms_list)
 	return utf8;
 }
 
-struct sms_assembly *sms_assembly_new()
+static int sms_serialize(unsigned char *buf, const struct sms *sms)
+{
+	int len, tpdu_len;
+
+	sms_encode(sms, &len, &tpdu_len, buf + 1);
+	buf[0] = tpdu_len;
+
+	return len;
+}
+
+static gboolean sms_deserialize(const unsigned char *buf,
+		struct sms *sms, int len)
+{
+	if (len < 1)
+		return FALSE;
+
+	return sms_decode(buf + 1, len - 1, FALSE, buf[0], sms);
+}
+
+static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
+					const struct sms *sms, time_t ts,
+					const struct sms_address *addr,
+					guint16 ref, guint8 max, guint8 seq,
+					gboolean backup);
+
+#define SMS_BACKUP_MODE 0600
+#define SMS_BACKUP_PATH STORAGEDIR "/%s/sms"
+#define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
+#define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
+
+#define SMS_ADDR_FMT "%21[0-9+*#]"
+
+static void sms_assembly_load(struct sms_assembly *assembly,
+				const struct dirent *dir)
+{
+	struct sms_address addr;
+	char straddr[sizeof(addr.address) + 1];
+	guint16 ref;
+	guint8 max;
+	guint8 seq;
+	char *path;
+	int len;
+	struct stat segment_stat;
+	struct dirent **segments;
+	char *endp;
+	int fd;
+	int r;
+	unsigned char buf[177];
+	struct sms segment;
+
+	if (dir->d_type != DT_DIR)
+		return;
+
+	if (sscanf(dir->d_name, SMS_ADDR_FMT "-%hi-%hhi",
+				straddr, &ref, &max) < 3)
+		return;
+	sms_address_from_string(&addr, straddr);
+
+	path = g_strdup_printf(SMS_BACKUP_PATH "/%s",
+			assembly->imsi, dir->d_name);
+	len = scandir(path, &segments, NULL, versionsort);
+	g_free(path);
+
+	if (len < 0)
+		return;
+
+	for (; len--; free(segments[len])) {
+		if (segments[len]->d_type != DT_REG)
+			continue;
+
+		seq = strtol(segments[len]->d_name, &endp, 10);
+		if (*endp != '\0')
+			continue;
+
+		path = g_strdup_printf(SMS_BACKUP_PATH "/%s/%s",
+				assembly->imsi,
+				dir->d_name, segments[len]->d_name);
+		fd = TFR(open(path, O_RDONLY));
+		g_free(path);
+
+		if (fd == -1)
+			continue;
+
+		if (fstat(fd, &segment_stat) != 0) {
+			TFR(close(fd));
+			continue;
+		}
+
+		r = TFR(read(fd, buf, sizeof(buf)));
+
+		if (r > 0 && sms_deserialize(buf, &segment, r)) {
+			if (sms_assembly_add_fragment_backup(assembly,
+						&segment,
+						segment_stat.st_mtime,
+						&addr, ref, max, seq, FALSE)) {
+				/* This can't happen */
+			}
+		}
+
+		TFR(close(fd));
+	}
+
+	free(segments);
+}
+
+static gboolean sms_assembly_store(struct sms_assembly *assembly,
+				struct sms_assembly_node *node,
+				const struct sms *sms, guint8 seq)
+{
+	unsigned char buf[177];
+	char *path;
+	int fd;
+	int len;
+
+	if (!assembly->imsi)
+		return;
+
+	len = sms_serialize(buf, sms);
+
+	path = g_strdup_printf(SMS_BACKUP_PATH_FILE, assembly->imsi,
+				sms_address_to_string(&node->addr),
+				node->ref, node->max_fragments, seq);
+
+	if (create_dirs(path, SMS_BACKUP_MODE | S_IXUSR)) {
+		g_free(path);
+		return FALSE;
+	}
+
+	fd = TFR(open(path, O_WRONLY | O_CREAT, SMS_BACKUP_MODE));
+	if (fd == -1) {
+		g_free(path);
+		return FALSE;
+	}
+
+	if (TFR(write(fd, buf, len)) < len) {
+		TFR(close(fd));
+		unlink(path);
+		g_free(path);
+		return FALSE;
+	}
+
+	g_free(path);
+	TFR(close(fd));
+
+	return TRUE;
+}
+
+static void sms_assembly_backup_free(struct sms_assembly *assembly,
+					struct sms_assembly_node *node)
 {
-	return g_new0(struct sms_assembly, 1);
+	char *path;
+	int seq;
+
+	if (!assembly->imsi)
+		return;
+
+	for (seq = 0; seq < node->max_fragments; seq++) {
+		int offset = seq / 32;
+		int bit = 1 << (seq % 32);
+
+		if (node->bitmap[offset] & bit) {
+			path = g_strdup_printf(SMS_BACKUP_PATH_FILE,
+					assembly->imsi,
+					sms_address_to_string(&node->addr),
+					node->ref, node->max_fragments, seq);
+			unlink(path);
+			g_free(path);
+		}
+	}
+
+	path = g_strdup_printf(SMS_BACKUP_PATH_DIR, assembly->imsi,
+				sms_address_to_string(&node->addr),
+				node->ref, node->max_fragments);
+	unlink(path);
+	g_free(path);
+}
+
+struct sms_assembly *sms_assembly_new(const char *imsi)
+{
+	struct sms_assembly *ret = g_new0(struct sms_assembly, 1);
+	char *path;
+	struct dirent **entries;
+	int len;
+
+	if (imsi) {
+		ret->imsi = imsi;
+
+		/* Restore state from backup */
+
+		path = g_strdup_printf(SMS_BACKUP_PATH, imsi);
+		len = scandir(path, &entries, NULL, alphasort);
+		g_free(path);
+
+		if (len < 0)
+			return ret;
+
+		while (len--) {
+			sms_assembly_load(ret, entries[len]);
+			free(entries[len]);
+		}
+
+		free(entries);
+	}
+
+	return ret;
 }
 
 void sms_assembly_free(struct sms_assembly *assembly)
@@ -2132,6 +2343,16 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					const struct sms_address *addr,
 					guint16 ref, guint8 max, guint8 seq)
 {
+	sms_assembly_add_fragment_backup(assembly, sms,
+						ts, addr, ref, max, seq, TRUE);
+}
+
+static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
+					const struct sms *sms, time_t ts,
+					const struct sms_address *addr,
+					guint16 ref, guint8 max, guint8 seq,
+					gboolean backup)
+{
 	int offset = seq / 32;
 	int bit = 1 << (seq % 32);
 	GSList *l;
@@ -2205,11 +2426,17 @@ out:
 	node->bitmap[offset] |= bit;
 	node->num_fragments += 1;
 
-	if (node->num_fragments < node->max_fragments)
+	if (node->num_fragments < node->max_fragments) {
+		if (backup)
+			sms_assembly_store(assembly, node, sms, seq);
+
 		return NULL;
+	}
 
 	completed = node->fragment_list;
 
+	sms_assembly_backup_free(assembly, node);
+
 	if (prev)
 		prev->next = l->next;
 	else
@@ -2243,6 +2470,8 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
 			continue;
 		}
 
+		sms_assembly_backup_free(assembly, node);
+
 		g_slist_foreach(node->fragment_list, (GFunc)g_free, 0);
 		g_slist_free(node->fragment_list);
 		g_free(node);
diff --git a/src/smsutil.h b/src/smsutil.h
index 95d0c78..f6541b6 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -360,6 +360,7 @@ struct sms_assembly_node {
 };
 
 struct sms_assembly {
+	const char *imsi;
 	GSList *assembly_list;
 };
 
@@ -454,7 +455,7 @@ gboolean sms_extract_concatenation(const struct sms *sms, guint16 *ref_num,
 unsigned char *sms_decode_datagram(GSList *sms_list, long *out_len);
 char *sms_decode_text(GSList *sms_list);
 
-struct sms_assembly *sms_assembly_new();
+struct sms_assembly *sms_assembly_new(const char *imsi);
 void sms_assembly_free(struct sms_assembly *assembly);
 GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					const struct sms *sms, time_t ts,
diff --git a/unit/Makefile.am b/unit/Makefile.am
index 8a27267..74d0f40 100644
--- a/unit/Makefile.am
+++ b/unit/Makefile.am
@@ -6,11 +6,13 @@ test_common_SOURCES = test-common.c $(top_srcdir)/src/common.c
 test_util_SOURCES = test-util.c $(top_srcdir)/src/util.c
 
 test_sms_SOURCES = test-sms.c $(top_srcdir)/src/util.c \
-					$(top_srcdir)/src/smsutil.c
+					$(top_srcdir)/src/smsutil.c \
+					$(top_srcdir)/src/common.c
 
 test_simutil_SOURCES = test-simutil.c $(top_srcdir)/src/util.c \
 					$(top_srcdir)/src/simutil.c \
-					$(top_srcdir)/src/smsutil.c
+					$(top_srcdir)/src/smsutil.c \
+					$(top_srcdir)/src/common.c
 
 LDADD = @GLIB_LIBS@ @GTHREAD_LIBS@
 
diff --git a/unit/test-sms.c b/unit/test-sms.c
index 52d6859..a94cd4b 100644
--- a/unit/test-sms.c
+++ b/unit/test-sms.c
@@ -619,7 +619,7 @@ static void test_assembly()
 	unsigned char pdu[164];
 	long pdu_len;
 	struct sms sms;
-	struct sms_assembly *assembly = sms_assembly_new();
+	struct sms_assembly *assembly = sms_assembly_new(NULL);
 	guint16 ref;
 	guint8 max;
 	guint8 seq;
@@ -776,7 +776,7 @@ static void test_prepare_concat()
 	struct sms *sms;
 	struct sms decoded;
 	int pdu_len, tpdu_len;
-	struct sms_assembly *assembly = sms_assembly_new();
+	struct sms_assembly *assembly = sms_assembly_new(NULL);
 	guint16 ref;
 	guint8 max;
 	guint8 seq;
-- 
1.6.1

Re: Write state of SMS assembly to disk and restore on startup.

[Denis Kenzior]

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

Hi Andrew,

On Fri, Aug 21, 2009 at 9:40 PM, Andrzej Zaborowski <
andrew.zaborowski@intel.com> wrote:

> This way we can continue receiving segmented messages over a reset or
> crash.
> ---
>  src/common.c     |   29 +++++++
>  src/common.h     |   10 +++
>  src/sim.c        |   34 --------
>  src/sms.c        |   14 +++-
>  src/smsutil.c    |  237
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/smsutil.h    |    3 +-
>  unit/Makefile.am |    6 +-
>  unit/test-sms.c  |    4 +-
>  8 files changed, 293 insertions(+), 44 deletions(-)
>
>
Somehow this patch fell through the cracks completely.  Can you rebase and
resend?


> +
> +int create_dirs(const char *filename, const mode_t mode)
> +{
>

Can we move this part to storage.c instead of common.c to be more consistent
with connman & bluez?


> +
> +#ifdef TEMP_FAILURE_RETRY
> +#define TFR TEMP_FAILURE_RETRY
> +#else
> +#define TFR
> +#endif
> +
> +#include <fcntl.h>
> +
> +int create_dirs(const char *filename, const mode_t mode);
>

storage.c as well.


> +static GSList *sms_assembly_add_fragment_backup(struct sms_assembly
> *assembly,
> +                                       const struct sms *sms, time_t ts,
> +                                       const struct sms_address *addr,
> +                                       guint16 ref, guint8 max, guint8
> seq,
> +                                       gboolean backup);
> +
> +#define SMS_BACKUP_MODE 0600
> +#define SMS_BACKUP_PATH STORAGEDIR "/%s/sms"
> +#define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
> +#define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
> +
> +#define SMS_ADDR_FMT "%21[0-9+*#]"
>

These really belong at the top of the file.  Includes, Defines, globals,
forward declarations, structure definitions in that order.


> +
> +static void sms_assembly_load(struct sms_assembly *assembly,
> +                               const struct dirent *dir)
> +{
> +       for (; len--; free(segments[len])) {
>

Please don't do such evilness.  I don't even understand how this can work.


> +               fd = TFR(open(path, O_RDONLY));
> +               g_free(path);
> +
> +               if (fd == -1)
> +                       continue;
> +
> +               if (fstat(fd, &segment_stat) != 0) {
> +                       TFR(close(fd));
> +                       continue;
> +               }
> +
> +               r = TFR(read(fd, buf, sizeof(buf)));
>

This part should be a utility function in storage.c.  We repeat almost the
same code in sim.c


> +
> +               if (r > 0 && sms_deserialize(buf, &segment, r)) {
> +                       if (sms_assembly_add_fragment_backup(assembly,
> +                                               &segment,
> +                                               segment_stat.st_mtime,
> +                                               &addr, ref, max, seq,
> FALSE)) {
> +                               /* This can't happen */
> +                       }
>

We read it from the backup store and almost immediately overwrite the backup
store?  Seems inefficient.


> +       if (create_dirs(path, SMS_BACKUP_MODE | S_IXUSR)) {
> +               g_free(path);
> +               return FALSE;
> +       }
> +
> +       fd = TFR(open(path, O_WRONLY | O_CREAT, SMS_BACKUP_MODE));
> +       if (fd == -1) {
> +               g_free(path);
> +               return FALSE;
> +       }
> +
> +       if (TFR(write(fd, buf, len)) < len) {
> +               TFR(close(fd));
> +               unlink(path);
> +               g_free(path);
> +               return FALSE;
> +       }
> +
> +       g_free(path);
> +       TFR(close(fd));
>

Again, sounds like this should be a utility function in storage.c

Regards,
-Denis

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 5398 bytes --]

Re: Write state of SMS assembly to disk and restore on startup.

[Andrzej Zaborowski]

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

2009/9/1 Denis Kenzior <denkenz@gmail.com>:
> On Fri, Aug 21, 2009 at 9:40 PM, Andrzej Zaborowski
> <andrew.zaborowski@intel.com> wrote:
>> This way we can continue receiving segmented messages over a reset or
>> crash.
>> ---
>>  src/common.c     |   29 +++++++
>>  src/common.h     |   10 +++
>>  src/sim.c        |   34 --------
>>  src/sms.c        |   14 +++-
>>  src/smsutil.c    |  237
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/smsutil.h    |    3 +-
>>  unit/Makefile.am |    6 +-
>>  unit/test-sms.c  |    4 +-
>>  8 files changed, 293 insertions(+), 44 deletions(-)
>>
>
>> +int create_dirs(const char *filename, const mode_t mode)
>> +{
>
> Can we move this part to storage.c instead of common.c to be more consistent
> with connman & bluez?

Ok, I moved the common part of the file reading / writing to storage.c
but some of the file access in sim.c is still doing it manually
because there's lseeking in the middle.

>> +
>> +               if (r > 0 && sms_deserialize(buf, &segment, r)) {
>> +                       if (sms_assembly_add_fragment_backup(assembly,
>> +                                               &segment,
>> +                                               segment_stat.st_mtime,
>> +                                               &addr, ref, max, seq,
>> FALSE)) {
>> +                               /* This can't happen */
>> +                       }
>
> We read it from the backup store and almost immediately overwrite the backup
> store?  Seems inefficient.

The FALSE in the last parameter tells the function not to write the
backup, I had renamed the function to sms_assembly_add_fragment_backup
and made a wrapper under the old name.  I agree the naming is poor.

Sending updated patches in attachments.

Regards

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Move-create_dirs-to-storage.c-add-file-read-write-u.patch --]
[-- Type: text/x-patch, Size: 7659 bytes --]

From f986a17536dca707ba6294d17efa3c2907e8f147 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <andrew.zaborowski@intel.com>
Date: Sat, 5 Sep 2009 03:12:19 +0200
Subject: [PATCH 1/2] Move create_dirs to storage.c, add file read/write utilities

---
 Makefile.am   |    2 +-
 src/sim.c     |   69 ++-----------------------------
 src/storage.c |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/storage.h |   36 ++++++++++++++++
 4 files changed, 166 insertions(+), 66 deletions(-)
 create mode 100644 src/storage.c
 create mode 100644 src/storage.h

diff --git a/Makefile.am b/Makefile.am
index 9bc321c..068ca96 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -140,7 +140,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
 			src/call-meter.c src/smsutil.h src/smsutil.c \
 			src/ssn.c src/call-barring.c src/sim.c \
 			src/phonebook.c src/history.c src/message-waiting.c \
-			src/simutil.h src/simutil.c
+			src/simutil.h src/simutil.c src/storage.h src/storage.c
 
 src_ofonod_LDADD = $(builtin_libadd) \
 			@GLIB_LIBS@ @GTHREAD_LIBS@ @DBUS_LIBS@ -ldl
diff --git a/src/sim.c b/src/sim.c
index f53be9d..a072c8c 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -41,12 +41,7 @@
 #include "util.h"
 #include "smsutil.h"
 #include "simutil.h"
-
-#ifdef TEMP_FAILURE_RETRY
-#define TFR TEMP_FAILURE_RETRY
-#else
-#define TFR
-#endif
+#include "storage.h"
 
 #define SIM_MANAGER_INTERFACE "org.ofono.SimManager"
 
@@ -617,34 +612,6 @@ static void sim_retrieve_imsi(struct ofono_sim *sim)
 	sim->driver->read_imsi(sim, sim_imsi_cb, sim);
 }
 
-static int create_dirs(const char *filename, const mode_t mode)
-{
-	struct stat st;
-	char *dir;
-	const char *prev, *next;
-	int err;
-
-	err = stat(filename, &st);
-	if (!err && S_ISREG(st.st_mode))
-		return 0;
-
-	dir = g_malloc(strlen(filename) + 1);
-	strcpy(dir, "/");
-
-	for (prev = filename; (next = strchr(prev + 1, '/')); prev = next)
-		if (next > prev + 1) {
-			strncat(dir, prev + 1, next - prev);
-
-			if (mkdir(dir, mode) && errno != EEXIST) {
-				g_free(dir);
-				return -1;
-			}
-		}
-
-	g_free(dir);
-	return 0;
-}
-
 static void sim_op_error(struct ofono_sim *sim)
 {
 	struct sim_file_op *op = g_queue_pop_head(sim->simop_q);
@@ -769,33 +736,6 @@ static gboolean sim_op_retrieve_next(gpointer user)
 	return FALSE;
 }
 
-static gboolean cache_info(const char *path, const unsigned char *info, int len)
-{
-	int fd;
-	int r;
-
-	if (create_dirs(path, SIM_CACHE_MODE | S_IXUSR) != 0)
-		return FALSE;
-
-	fd = TFR(open(path, O_WRONLY | O_CREAT, SIM_CACHE_MODE));
-
-	if (fd == -1) {
-		ofono_debug("Error %i creating cache file %s",
-				errno, path);
-		return FALSE;
-	}
-
-	r = TFR(write(fd, info, len));
-	TFR(close(fd));
-
-	if (r < len) {
-		unlink(path);
-		return FALSE;
-	}
-
-	return TRUE;
-}
-
 static void sim_op_info_cb(const struct ofono_error *error, int length,
 				enum ofono_sim_file_structure structure,
 				int record_length,
@@ -838,7 +778,6 @@ static void sim_op_info_cb(const struct ofono_error *error, int length,
 	sim->simop_source = g_timeout_add(0, sim_op_retrieve_next, sim);
 
 	if (op->cache && imsi) {
-		char *path = g_strdup_printf(SIM_CACHE_PATH, imsi, op->id);
 		unsigned char fileinfo[6];
 
 		fileinfo[0] = error->type;
@@ -848,9 +787,9 @@ static void sim_op_info_cb(const struct ofono_error *error, int length,
 		fileinfo[4] = record_length >> 8;
 		fileinfo[5] = record_length & 0xff;
 
-		op->cache = cache_info(path, fileinfo, 6);
-
-		g_free(path);
+		if (write_file(fileinfo, 6, SIM_CACHE_MODE,
+					SIM_CACHE_PATH, imsi, op->id) != 6)
+			op->cache = FALSE;
 	}
 }
 
diff --git a/src/storage.c b/src/storage.c
new file mode 100644
index 0000000..d66f20f
--- /dev/null
+++ b/src/storage.c
@@ -0,0 +1,125 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdarg.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/types.h>
+#include "storage.h"
+
+int create_dirs(const char *filename, const mode_t mode)
+{
+	struct stat st;
+	char *dir;
+	const char *prev, *next;
+	int err;
+
+	err = stat(filename, &st);
+	if (!err && S_ISREG(st.st_mode))
+		return 0;
+
+	dir = g_malloc(strlen(filename) + 1);
+	strcpy(dir, "/");
+
+	for (prev = filename; (next = strchr(prev + 1, '/')); prev = next)
+		if (next > prev + 1) {
+			strncat(dir, prev + 1, next - prev);
+
+			if (mkdir(dir, mode) && errno != EEXIST) {
+				g_free(dir);
+				return -1;
+			}
+		}
+
+	g_free(dir);
+	return 0;
+}
+
+ssize_t read_file(unsigned char *buffer, size_t len,
+			const char *path_fmt, ...) {
+	va_list ap;
+	char *path;
+	ssize_t r;
+	int fd;
+
+	va_start(ap, path_fmt);
+	path = g_strdup_vprintf(path_fmt, ap);
+	va_end(ap);
+
+	fd = TFR(open(path, O_RDONLY));
+
+	g_free(path);
+
+	if (fd == -1)
+		return -1;
+
+	r = TFR(read(fd, buffer, len));
+
+	TFR(close(fd));
+
+	return r;
+}
+
+ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode,
+			const char *path_fmt, ...) {
+	va_list ap;
+	char *path;
+	ssize_t r;
+	int fd;
+
+	va_start(ap, path_fmt);
+	path = g_strdup_vprintf(path_fmt, ap);
+	va_end(ap);
+
+	if (create_dirs(path, mode | S_IXUSR) != 0) {
+		g_free(path);
+		return -1;
+	}
+
+	fd = TFR(open(path, O_WRONLY | O_CREAT | O_TRUNC, mode));
+	if (fd == -1) {
+		g_free(path);
+		return -1;
+	}
+
+	r = TFR(write(fd, buffer, len));
+
+	TFR(close(fd));
+
+	if (r != (ssize_t) len) {
+		unlink(path);
+		r = -1;
+	}
+
+	g_free(path);
+	return r;
+}
diff --git a/src/storage.h b/src/storage.h
new file mode 100644
index 0000000..305c4a5
--- /dev/null
+++ b/src/storage.h
@@ -0,0 +1,36 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef TEMP_FAILURE_RETRY
+#define TFR TEMP_FAILURE_RETRY
+#else
+#define TFR
+#endif
+
+#include <fcntl.h>
+
+int create_dirs(const char *filename, const mode_t mode);
+
+ssize_t read_file(unsigned char *buffer, size_t len,
+			const char *path_fmt, ...);
+
+ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode,
+			const char *path_fmt, ...);
-- 
1.6.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0003-Write-state-of-SMS-assembly-to-disk-and-restore-on-s.patch --]
[-- Type: text/x-patch, Size: 12010 bytes --]

From 39ebf0ce647ea37f937028d491b5a530a3074068 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <andrew.zaborowski@intel.com>
Date: Sat, 5 Sep 2009 03:16:02 +0200
Subject: [PATCH 2/2] Write state of SMS assembly to disk and restore on startup.

This way we can continue receiving segmented messages over a reset or
crash.
---
 Makefile.am     |    5 +-
 src/common.c    |    1 +
 src/sms.c       |   58 +++++++++++++++-
 src/smsutil.c   |  214 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/smsutil.h   |    3 +-
 unit/test-sms.c |    4 +-
 6 files changed, 276 insertions(+), 9 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 068ca96..214073f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -204,12 +204,13 @@ unit_test_util_SOURCES = unit/test-util.c src/util.c
 unit_test_util_LDADD = @GLIB_LIBS@
 unit_objects += $(unit_test_utils_OBJECTS)
 
-unit_test_sms_SOURCES = unit/test-sms.c src/util.c src/smsutil.c
+unit_test_sms_SOURCES = unit/test-sms.c src/util.c src/smsutil.c src/storage.c
 unit_test_sms_LDADD = @GLIB_LIBS@
 unit_objects += $(unit_test_sms_OBJECTS)
 
 unit_test_simutil_SOURCES = unit/test-simutil.c src/util.c \
-						src/simutil.c src/smsutil.c
+						src/simutil.c src/smsutil.c \
+						src/storage.c
 unit_test_simutil_LDADD = @GLIB_LIBS@
 unit_objects += $(unit_test_simutil_OBJECTS)
 
diff --git a/src/common.c b/src/common.c
index ff94fc0..63884a3 100644
--- a/src/common.c
+++ b/src/common.c
@@ -26,6 +26,7 @@
 #define _GNU_SOURCE
 #include <string.h>
 #include <ctype.h>
+#include <errno.h>
 
 #include <glib.h>
 
diff --git a/src/sms.c b/src/sms.c
index fe76580..149ecd0 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -57,6 +57,9 @@ struct ofono_sms {
 	gint tx_source;
 	struct ofono_message_waiting *mw;
 	unsigned int mw_watch;
+	struct ofono_sim *sim;
+	unsigned int sim_watch;
+	unsigned int imsi_watch;
 	const struct ofono_sms_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -785,6 +788,17 @@ static void sms_unregister(struct ofono_atom *atom)
 		__ofono_modem_remove_atom_watch(modem, sms->mw_watch);
 		sms->mw_watch = 0;
 	}
+
+	if (sms->sim_watch) {
+		if (sms->imsi_watch) {
+			ofono_sim_remove_ready_watch(sms->sim,
+							sms->imsi_watch);
+			sms->imsi_watch = 0;
+		}
+
+		__ofono_modem_remove_atom_watch(modem, sms->sim_watch);
+		sms->sim_watch = 0;
+	}
 }
 
 static void sms_remove(struct ofono_atom *atom)
@@ -836,7 +850,6 @@ struct ofono_sms *ofono_sms_create(struct ofono_modem *modem,
 
 	sms->sca.type = 129;
 	sms->ref = 1;
-	sms->assembly = sms_assembly_new();
 	sms->txq = g_queue_new();
 	sms->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_SMS,
 						sms_remove, sms);
@@ -870,12 +883,45 @@ static void mw_watch(struct ofono_atom *atom,
 	sms->mw = __ofono_atom_get_data(atom);
 }
 
+static void sms_got_imsi(void *data)
+{
+	struct ofono_sms *sms = data;
+	const char *imsi = ofono_sim_get_imsi(sms->sim);
+
+	sms->assembly = sms_assembly_new(imsi);
+}
+
+static void sim_watch(struct ofono_atom *atom,
+			enum ofono_atom_watch_condition cond, void *data)
+{
+	struct ofono_sms *sms = data;
+
+	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) {
+		sms->imsi_watch = 0;
+
+		if (sms->assembly) {
+			sms_assembly_free(sms->assembly);
+			sms->assembly = NULL;
+		}
+
+		return;
+	}
+
+	sms->sim = __ofono_atom_get_data(atom);
+	sms->imsi_watch = ofono_sim_add_ready_watch(sms->sim, sms_got_imsi,
+							sms, NULL);
+
+	if (ofono_sim_get_ready(sms->sim))
+		sms_got_imsi(sms);
+}
+
 void ofono_sms_register(struct ofono_sms *sms)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	const char *path = __ofono_atom_get_path(sms->atom);
 	struct ofono_atom *mw_atom;
+	struct ofono_atom *sim_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					SMS_MANAGER_INTERFACE,
@@ -899,6 +945,16 @@ void ofono_sms_register(struct ofono_sms *sms)
 	if (mw_atom && __ofono_atom_get_registered(mw_atom))
 		mw_watch(mw_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
 
+	sms->sim_watch = __ofono_modem_add_atom_watch(modem,
+					OFONO_ATOM_TYPE_SIM,
+					sim_watch, sms, NULL);
+
+	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
+
+	if (sim_atom && __ofono_atom_get_registered(sim_atom))
+		sim_watch(sim_atom,
+				OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
+
 	__ofono_atom_register(sms->atom, sms_unregister);
 }
 
diff --git a/src/smsutil.c b/src/smsutil.c
index fcff9aa..79b5048 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -23,16 +23,38 @@
 #include <config.h>
 #endif
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <dirent.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
 #include <glib.h>
 
+#include "types.h"
+#include "common.h"
 #include "util.h"
+#include "storage.h"
 #include "smsutil.h"
 
 #define uninitialized_var(x) x = x
 
+#define SMS_BACKUP_MODE 0600
+#define SMS_BACKUP_PATH STORAGEDIR "/%s/sms"
+#define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
+#define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
+
+#define SMS_ADDR_FMT "%21[0-9+*#]"
+
+static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
+					const struct sms *sms, time_t ts,
+					const struct sms_address *addr,
+					guint16 ref, guint8 max, guint8 seq,
+					gboolean backup);
+
 void extract_bcd_number(const unsigned char *buf, int len, char *out)
 {
 	static const char digit_lut[] = "0123456789*#abc\0";
@@ -2106,9 +2128,177 @@ char *sms_decode_text(GSList *sms_list)
 	return utf8;
 }
 
-struct sms_assembly *sms_assembly_new()
+static int sms_serialize(unsigned char *buf, const struct sms *sms)
+{
+	int len, tpdu_len;
+
+	sms_encode(sms, &len, &tpdu_len, buf + 1);
+	buf[0] = tpdu_len;
+
+	return len;
+}
+
+static gboolean sms_deserialize(const unsigned char *buf,
+		struct sms *sms, int len)
+{
+	if (len < 1)
+		return FALSE;
+
+	return sms_decode(buf + 1, len - 1, FALSE, buf[0], sms);
+}
+
+static void sms_assembly_load(struct sms_assembly *assembly,
+				const struct dirent *dir)
+{
+	struct sms_address addr;
+	char straddr[sizeof(addr.address) + 1];
+	guint16 ref;
+	guint8 max;
+	guint8 seq;
+	char *path;
+	int len;
+	struct stat segment_stat;
+	struct dirent **segments;
+	char *endp;
+	int r;
+	int i;
+	unsigned char buf[177];
+	struct sms segment;
+
+	if (dir->d_type != DT_DIR)
+		return;
+
+	if (sscanf(dir->d_name, SMS_ADDR_FMT "-%hi-%hhi",
+				straddr, &ref, &max) < 3)
+		return;
+	sms_address_from_string(&addr, straddr);
+
+	path = g_strdup_printf(SMS_BACKUP_PATH "/%s",
+			assembly->imsi, dir->d_name);
+	len = scandir(path, &segments, NULL, versionsort);
+	g_free(path);
+
+	if (len < 0)
+		return;
+
+	for (i = 0; i < len; i++) {
+		if (segments[i]->d_type != DT_REG)
+			continue;
+
+		seq = strtol(segments[i]->d_name, &endp, 10);
+		if (*endp != '\0')
+			continue;
+
+		r = read_file(buf, sizeof(buf), SMS_BACKUP_PATH "/%s/%s",
+				assembly->imsi,
+				dir->d_name, segments[i]->d_name);
+		if (r < 0)
+			continue;
+
+		if (!sms_deserialize(buf, &segment, r))
+			continue;
+
+		path = g_strdup_printf(SMS_BACKUP_PATH "/%s/%s",
+				assembly->imsi,
+				dir->d_name, segments[i]->d_name);
+		r = stat(path, &segment_stat);
+		g_free(path);
+
+		if (r != 0)
+			continue;
+
+		if (sms_assembly_add_fragment_backup(assembly, &segment,
+						segment_stat.st_mtime,
+						&addr, ref, max, seq, FALSE)) {
+			/* This should not happen */
+		}
+	}
+
+	for (i = 0; i < len; i++)
+		free(segments[i]);
+
+	free(segments);
+}
+
+static gboolean sms_assembly_store(struct sms_assembly *assembly,
+				struct sms_assembly_node *node,
+				const struct sms *sms, guint8 seq)
+{
+	unsigned char buf[177];
+	int len;
+
+	if (!assembly->imsi)
+		return FALSE;
+
+	len = sms_serialize(buf, sms);
+
+	if (write_file(buf, len, SMS_BACKUP_MODE,
+				SMS_BACKUP_PATH_FILE, assembly->imsi,
+				sms_address_to_string(&node->addr),
+				node->ref, node->max_fragments, seq) != len)
+		return FALSE;
+
+	return TRUE;
+}
+
+static void sms_assembly_backup_free(struct sms_assembly *assembly,
+					struct sms_assembly_node *node)
+{
+	char *path;
+	int seq;
+
+	if (!assembly->imsi)
+		return;
+
+	for (seq = 0; seq < node->max_fragments; seq++) {
+		int offset = seq / 32;
+		int bit = 1 << (seq % 32);
+
+		if (node->bitmap[offset] & bit) {
+			path = g_strdup_printf(SMS_BACKUP_PATH_FILE,
+					assembly->imsi,
+					sms_address_to_string(&node->addr),
+					node->ref, node->max_fragments, seq);
+			unlink(path);
+			g_free(path);
+		}
+	}
+
+	path = g_strdup_printf(SMS_BACKUP_PATH_DIR, assembly->imsi,
+				sms_address_to_string(&node->addr),
+				node->ref, node->max_fragments);
+	rmdir(path);
+	g_free(path);
+}
+
+struct sms_assembly *sms_assembly_new(const char *imsi)
 {
-	return g_new0(struct sms_assembly, 1);
+	struct sms_assembly *ret = g_new0(struct sms_assembly, 1);
+	char *path;
+	struct dirent **entries;
+	int len;
+
+	if (imsi) {
+		ret->imsi = imsi;
+
+		/* Restore state from backup */
+
+		path = g_strdup_printf(SMS_BACKUP_PATH, imsi);
+		len = scandir(path, &entries, NULL, alphasort);
+		g_free(path);
+
+		if (len < 0)
+			return ret;
+
+		while (len--) {
+			sms_assembly_load(ret, entries[len]);
+			free(entries[len]);
+		}
+
+		free(entries);
+	}
+
+	return ret;
 }
 
 void sms_assembly_free(struct sms_assembly *assembly)
@@ -2132,6 +2322,16 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					const struct sms_address *addr,
 					guint16 ref, guint8 max, guint8 seq)
 {
+	return sms_assembly_add_fragment_backup(assembly, sms,
+						ts, addr, ref, max, seq, TRUE);
+}
+
+static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
+					const struct sms *sms, time_t ts,
+					const struct sms_address *addr,
+					guint16 ref, guint8 max, guint8 seq,
+					gboolean backup)
+{
 	int offset = seq / 32;
 	int bit = 1 << (seq % 32);
 	GSList *l;
@@ -2205,11 +2405,17 @@ out:
 	node->bitmap[offset] |= bit;
 	node->num_fragments += 1;
 
-	if (node->num_fragments < node->max_fragments)
+	if (node->num_fragments < node->max_fragments) {
+		if (backup)
+			sms_assembly_store(assembly, node, sms, seq);
+
 		return NULL;
+	}
 
 	completed = node->fragment_list;
 
+	sms_assembly_backup_free(assembly, node);
+
 	if (prev)
 		prev->next = l->next;
 	else
@@ -2243,6 +2449,8 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
 			continue;
 		}
 
+		sms_assembly_backup_free(assembly, node);
+
 		g_slist_foreach(node->fragment_list, (GFunc)g_free, 0);
 		g_slist_free(node->fragment_list);
 		g_free(node);
diff --git a/src/smsutil.h b/src/smsutil.h
index 95d0c78..f6541b6 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -360,6 +360,7 @@ struct sms_assembly_node {
 };
 
 struct sms_assembly {
+	const char *imsi;
 	GSList *assembly_list;
 };
 
@@ -454,7 +455,7 @@ gboolean sms_extract_concatenation(const struct sms *sms, guint16 *ref_num,
 unsigned char *sms_decode_datagram(GSList *sms_list, long *out_len);
 char *sms_decode_text(GSList *sms_list);
 
-struct sms_assembly *sms_assembly_new();
+struct sms_assembly *sms_assembly_new(const char *imsi);
 void sms_assembly_free(struct sms_assembly *assembly);
 GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 					const struct sms *sms, time_t ts,
diff --git a/unit/test-sms.c b/unit/test-sms.c
index 52d6859..a94cd4b 100644
--- a/unit/test-sms.c
+++ b/unit/test-sms.c
@@ -619,7 +619,7 @@ static void test_assembly()
 	unsigned char pdu[164];
 	long pdu_len;
 	struct sms sms;
-	struct sms_assembly *assembly = sms_assembly_new();
+	struct sms_assembly *assembly = sms_assembly_new(NULL);
 	guint16 ref;
 	guint8 max;
 	guint8 seq;
@@ -776,7 +776,7 @@ static void test_prepare_concat()
 	struct sms *sms;
 	struct sms decoded;
 	int pdu_len, tpdu_len;
-	struct sms_assembly *assembly = sms_assembly_new();
+	struct sms_assembly *assembly = sms_assembly_new(NULL);
 	guint16 ref;
 	guint8 max;
 	guint8 seq;
-- 
1.6.1

Re: Write state of SMS assembly to disk and restore on startup.

[Denis Kenzior]

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

Hi Andrew,

Both patches have been applied.

I had to fix up a few things in order to make this actually work.  Please 
review the changes and make sure they're OK with you.

Thanks,
-Denis

Re: Write state of SMS assembly to disk and restore on startup.

[Andrzej Zaborowski]

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

Hi,

2009/9/8 Denis Kenzior <denkenz@gmail.com>:
> Both patches have been applied.
>
> I had to fix up a few things in order to make this actually work.  Please
> review the changes and make sure they're OK with you.

Yes, all of the changes make a lot of sense, thanks.

Regards