[RFC PATCH v2 2/4] smsutil: storing/loading sms status report over reboot

[RFC PATCH v2 2/4] smsutil: storing/loading sms status report over reboot

[Petteri Tikander]

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

---
 src/smsutil.c |  192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/smsutil.h |    2 +-
 2 files changed, 188 insertions(+), 6 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index c60b8ec..b001a1d 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -45,6 +45,10 @@
 #define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
 #define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
 
+#define SMS_SR_BACKUP_PATH STORAGEDIR "/%s/sms_sr"
+#define SMS_SR_BACKUP_PATH_DIR SMS_SR_BACKUP_PATH "/%s"
+#define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH_DIR "/%i"
+
 #define SMS_ADDR_FMT "%24[0-9A-F]"
 
 static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
@@ -2413,7 +2417,7 @@ static void sms_assembly_backup_free(struct sms_assembly *assembly,
 {
 	char *path;
 	int seq;
-	char straddr[25];
+	DECLARE_SMS_ADDR_STR(straddr);
 
 	if (!assembly->imsi)
 		return;
@@ -2642,20 +2646,182 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
 	}
 }
 
+static void sr_assembly_load_backup(GHashTable *assembly_table,
+					const char *imsi,
+					const struct dirent *addr_dir)
+{
+	char *path;
+	struct dirent **ids;
+	struct sms_address addr;
+	DECLARE_SMS_ADDR_STR(straddr);
+	struct id_table_node *node;
+	GHashTable *id_table;
+	int len;
+	int r;
+	char *assembly_table_key;
+	unsigned int *id_table_key;
+	struct stat segment_stat;
+
+	if (addr_dir->d_type != DT_DIR)
+		return;
+
+	/* Max of SMS address size is 12 bytes, hex encoded */
+	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT, straddr) < 1)
+		return;
+
+	if (sms_assembly_extract_address(straddr, &addr) == FALSE)
+		return;
+
+	/* Go through different msg_ids. */
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
+							addr_dir->d_name);
+	len = scandir(path, &ids, NULL, versionsort);
+
+	g_free(path);
+
+	if (len < 0)
+		return;
+
+	id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
+							g_free, g_free);
+
+	assembly_table_key = g_try_malloc(sizeof(addr.address));
+
+	if (assembly_table_key == NULL)
+		return;
+
+	assembly_table_key = g_strdup(sms_address_to_string(&addr));
+	g_hash_table_insert(assembly_table, assembly_table_key, id_table);
+
+	while (len--) {
+		path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
+				imsi, addr_dir->d_name, ids[len]->d_name);
+
+		node = g_new0(struct id_table_node, 1);
+
+		r = read_file((unsigned char *) node,
+				sizeof(struct id_table_node),
+				SMS_SR_BACKUP_PATH "/%s/%s",
+				imsi, addr_dir->d_name, ids[len]->d_name);
+
+		if (r < 0) {
+			g_free(path);
+			g_free(ids[len]);
+			g_free(node);
+			continue;
+		}
+
+		r = stat(path, &segment_stat);
+
+		if (r != 0) {
+			g_free(path);
+			g_free(ids[len]);
+			g_free(node);
+			continue;
+		}
+
+		/* Node ready, create key and add them to the table */
+		id_table_key = g_new0(unsigned int, 1);
+		*id_table_key = atoi(ids[len]->d_name);
+
+		g_hash_table_insert(id_table, id_table_key, node);
+
+		g_free(path);
+		g_free(ids[len]);
+	}
+	g_free(ids);
+}
+
 struct status_report_assembly *status_report_assembly_new(const char *imsi)
 {
+	char *path;
+	int len;
+	struct dirent **addresses;
 	struct status_report_assembly *ret =
 				g_new0(struct status_report_assembly, 1);
 
 	ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
 				g_free, (GDestroyNotify)g_hash_table_destroy);
 
-	if (imsi)
+	if (imsi) {
 		ret->imsi = imsi;
 
+		/* Restore state from backup */
+		path = g_strdup_printf(SMS_SR_BACKUP_PATH, imsi);
+		len = scandir(path, &addresses, NULL, alphasort);
+
+		g_free(path);
+
+		if (len < 0)
+			return ret;
+
+		/* Go through different addresses. Each address can relate to
+		 * 1-n msg_ids.
+		 */
+
+		while (len--) {
+			sr_assembly_load_backup(ret->assembly_table, imsi,
+								addresses[len]);
+			g_free(addresses[len]);
+		}
+
+		g_free(addresses);
+	}
+
 	return ret;
 }
 
+static gboolean sr_assembly_add_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					const struct sms_address *addr,
+					unsigned int msg_id)
+{
+	int len = sizeof(struct id_table_node);
+	DECLARE_SMS_ADDR_STR(straddr);
+
+	if (!imsi)
+		return FALSE;
+
+	if (sms_address_to_hex_string(addr, straddr) == FALSE)
+		return FALSE;
+
+	/* storagedir/%s/sms_sr/%s/%i */
+	if (write_file((unsigned char *) node, len, SMS_BACKUP_MODE,
+			SMS_SR_BACKUP_PATH_FILE, imsi,
+			straddr, msg_id) != len)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					const struct sms_address *addr,
+					unsigned int msg_id)
+{
+	char *path;
+	DECLARE_SMS_ADDR_STR(straddr);
+
+	if (!imsi)
+		return FALSE;
+
+	if (sms_address_to_hex_string(addr, straddr) == FALSE)
+		return FALSE;
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr, msg_id);
+
+	unlink(path);
+	g_free(path);
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
+
+	/* If the address does not have relating msg_ids anymore, remove it */
+	rmdir(path);
+	g_free(path);
+
+	return TRUE;
+}
+
 void status_report_assembly_free(struct status_report_assembly *assembly)
 {
 	g_hash_table_destroy(assembly->assembly_table);
@@ -2698,6 +2864,7 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	GHashTableIter iter;
 	gboolean pending;
 	int i;
+	unsigned int msg_id;
 
 	/* We ignore temporary or tempfinal status reports */
 	if (sr_st_to_delivered(status_report->status_report.st,
@@ -2714,7 +2881,6 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	g_hash_table_iter_init(&iter, id_table);
 	while (g_hash_table_iter_next(&iter, &key, &value)) {
 		node = value;
-
 		if (node->mrs[offset] & bit)
 			break;
 
@@ -2743,14 +2909,29 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 		}
 	}
 
-	if (pending == TRUE && node->deliverable == TRUE)
+	msg_id = *(unsigned int *) key;
+
+	if (pending == TRUE && node->deliverable == TRUE) {
+		/* More status reports expected, and already received
+		 * reports completed. Update backup file.
+		 */
+		sr_assembly_add_fragment_backup(
+					assembly->imsi, node,
+					&status_report->status_report.raddr,
+					msg_id);
+
 		return FALSE;
+	}
 
 	if (out_delivered)
 		*out_delivered = node->deliverable;
 
 	if (out_id)
-		*out_id = *((unsigned int *) key);
+		*out_id = msg_id;
+
+	sr_assembly_remove_fragment_backup(assembly->imsi, node,
+					&status_report->status_report.raddr,
+					msg_id);
 
 	g_hash_table_iter_remove(&iter);
 
@@ -2804,6 +2985,7 @@ void status_report_assembly_add_fragment(
 	node->mrs[offset] |= bit;
 	node->expiration = expiration;
 	node->sent_mrs++;
+	sr_assembly_add_fragment_backup(assembly->imsi, node, to, msg_id);
 }
 
 void status_report_assembly_expire(struct status_report_assembly *assembly,
diff --git a/src/smsutil.h b/src/smsutil.h
index eb70b6d..3c6b3ae 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -370,7 +370,7 @@ struct id_table_node {
 	unsigned char total_mrs;
 	unsigned char sent_mrs;
 	gboolean deliverable;
-};
+} __attribute__((packed));
 
 struct status_report_assembly {
 	const char *imsi;
-- 
1.6.3.3

Re: [RFC PATCH v2 2/4] smsutil: storing/loading sms status report over reboot

[Denis Kenzior]

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

Hi Petteri,

On 08/19/2010 12:41 PM, Petteri Tikander wrote:
> ---
>  src/smsutil.c |  192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  src/smsutil.h |    2 +-
>  2 files changed, 188 insertions(+), 6 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index c60b8ec..b001a1d 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -45,6 +45,10 @@
>  #define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
>  #define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
>  
> +#define SMS_SR_BACKUP_PATH STORAGEDIR "/%s/sms_sr"
> +#define SMS_SR_BACKUP_PATH_DIR SMS_SR_BACKUP_PATH "/%s"
> +#define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH_DIR "/%i"
> +
>  #define SMS_ADDR_FMT "%24[0-9A-F]"
>  
>  static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
> @@ -2413,7 +2417,7 @@ static void sms_assembly_backup_free(struct sms_assembly *assembly,
>  {
>  	char *path;
>  	int seq;
> -	char straddr[25];
> +	DECLARE_SMS_ADDR_STR(straddr);
>  
>  	if (!assembly->imsi)
>  		return;
> @@ -2642,20 +2646,182 @@ void sms_assembly_expire(struct sms_assembly *assembly, time_t before)
>  	}
>  }
>  
> +static void sr_assembly_load_backup(GHashTable *assembly_table,
> +					const char *imsi,
> +					const struct dirent *addr_dir)
> +{
> +	char *path;
> +	struct dirent **ids;
> +	struct sms_address addr;
> +	DECLARE_SMS_ADDR_STR(straddr);
> +	struct id_table_node *node;
> +	GHashTable *id_table;
> +	int len;
> +	int r;
> +	char *assembly_table_key;
> +	unsigned int *id_table_key;
> +	struct stat segment_stat;
> +
> +	if (addr_dir->d_type != DT_DIR)
> +		return;
> +
> +	/* Max of SMS address size is 12 bytes, hex encoded */
> +	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT, straddr) < 1)
> +		return;
> +

Potential suggestion might be to use a flatter directory structure and
do something like SMS_ADDR_FMT "-%d"...

> +	if (sms_assembly_extract_address(straddr, &addr) == FALSE)
> +		return;
> +
> +	/* Go through different msg_ids. */
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
> +							addr_dir->d_name);
> +	len = scandir(path, &ids, NULL, versionsort);
> +
> +	g_free(path);
> +
> +	if (len < 0)
> +		return;
> +
> +	id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> +							g_free, g_free);
> +
> +	assembly_table_key = g_try_malloc(sizeof(addr.address));
> +
> +	if (assembly_table_key == NULL)
> +		return;
> +
> +	assembly_table_key = g_strdup(sms_address_to_string(&addr));

g_strdup already mallocs the needed space.  The above g_try_malloc is
only leaking memory.

> +	g_hash_table_insert(assembly_table, assembly_table_key, id_table);
> +
> +	while (len--) {
> +		path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
> +				imsi, addr_dir->d_name, ids[len]->d_name);
> +

You have to make sure to sanitize ids[len]->d_name.  If it is not an
integer, then it needs to be skipped.  Also watch out for atoi() use
below, strtol is a much better alternative.

> +		node = g_new0(struct id_table_node, 1);
> +
> +		r = read_file((unsigned char *) node,
> +				sizeof(struct id_table_node),
> +				SMS_SR_BACKUP_PATH "/%s/%s",
> +				imsi, addr_dir->d_name, ids[len]->d_name);
> +
> +		if (r < 0) {
> +			g_free(path);
> +			g_free(ids[len]);
> +			g_free(node);
> +			continue;
> +		}
> +
> +		r = stat(path, &segment_stat);

Hah, for sms_assembly we were extra tricky and used the modification
time stamp on the file system as the received time stamp in the node
structure.  This works because the file is written at the same time.
Since we're being lazy and writing out the structure directly this part
is not needed.

> +
> +		if (r != 0) {
> +			g_free(path);
> +			g_free(ids[len]);
> +			g_free(node);
> +			continue;
> +		}
> +
> +		/* Node ready, create key and add them to the table */
> +		id_table_key = g_new0(unsigned int, 1);
> +		*id_table_key = atoi(ids[len]->d_name);
> +
> +		g_hash_table_insert(id_table, id_table_key, node);
> +
> +		g_free(path);
> +		g_free(ids[len]);
> +	}
> +	g_free(ids);
> +}
> +
>  struct status_report_assembly *status_report_assembly_new(const char *imsi)
>  {
> +	char *path;
> +	int len;
> +	struct dirent **addresses;
>  	struct status_report_assembly *ret =
>  				g_new0(struct status_report_assembly, 1);
>  
>  	ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
>  				g_free, (GDestroyNotify)g_hash_table_destroy);
>  
> -	if (imsi)
> +	if (imsi) {
>  		ret->imsi = imsi;
>  
> +		/* Restore state from backup */
> +		path = g_strdup_printf(SMS_SR_BACKUP_PATH, imsi);
> +		len = scandir(path, &addresses, NULL, alphasort);
> +
> +		g_free(path);
> +
> +		if (len < 0)
> +			return ret;
> +
> +		/* Go through different addresses. Each address can relate to
> +		 * 1-n msg_ids.
> +		 */

Please stick to our multi-line comment guidelines, doc/coding-style.txt
item M2.  And yes I know the sms_assembly does it this way :)

> +
> +		while (len--) {
> +			sr_assembly_load_backup(ret->assembly_table, imsi,
> +								addresses[len]);
> +			g_free(addresses[len]);
> +		}
> +
> +		g_free(addresses);
> +	}
> +

This part looks good otherwise

>  	return ret;
>  }
>  
> +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					const struct sms_address *addr,
> +					unsigned int msg_id)
> +{
> +	int len = sizeof(struct id_table_node);
> +	DECLARE_SMS_ADDR_STR(straddr);
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	if (sms_address_to_hex_string(addr, straddr) == FALSE)
> +		return FALSE;
> +
> +	/* storagedir/%s/sms_sr/%s/%i */
> +	if (write_file((unsigned char *) node, len, SMS_BACKUP_MODE,
> +			SMS_SR_BACKUP_PATH_FILE, imsi,
> +			straddr, msg_id) != len)
> +		return FALSE;
> +
> +	return TRUE;
> +}
> +
> +static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					const struct sms_address *addr,
> +					unsigned int msg_id)
> +{
> +	char *path;
> +	DECLARE_SMS_ADDR_STR(straddr);
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	if (sms_address_to_hex_string(addr, straddr) == FALSE)
> +		return FALSE;
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr, msg_id);
> +
> +	unlink(path);
> +	g_free(path);
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
> +
> +	/* If the address does not have relating msg_ids anymore, remove it */
> +	rmdir(path);
> +	g_free(path);
> +
> +	return TRUE;
> +}
> +

Looks good, but see my earlier comment above about a flatter file structure.

>  void status_report_assembly_free(struct status_report_assembly *assembly)
>  {
>  	g_hash_table_destroy(assembly->assembly_table);
> @@ -2698,6 +2864,7 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	GHashTableIter iter;
>  	gboolean pending;
>  	int i;
> +	unsigned int msg_id;
>  
>  	/* We ignore temporary or tempfinal status reports */
>  	if (sr_st_to_delivered(status_report->status_report.st,
> @@ -2714,7 +2881,6 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	g_hash_table_iter_init(&iter, id_table);
>  	while (g_hash_table_iter_next(&iter, &key, &value)) {
>  		node = value;
> -

This newline is actually needed, please see doc/coding-style.txt item M1

>  		if (node->mrs[offset] & bit)
>  			break;
>  
> @@ -2743,14 +2909,29 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		}
>  	}
>  
> -	if (pending == TRUE && node->deliverable == TRUE)
> +	msg_id = *(unsigned int *) key;
> +
> +	if (pending == TRUE && node->deliverable == TRUE) {
> +		/* More status reports expected, and already received
> +		 * reports completed. Update backup file.
> +		 */

Coding style item M2

> +		sr_assembly_add_fragment_backup(
> +					assembly->imsi, node,
> +					&status_report->status_report.raddr,
> +					msg_id);
> +
>  		return FALSE;
> +	}
>  
>  	if (out_delivered)
>  		*out_delivered = node->deliverable;
>  
>  	if (out_id)
> -		*out_id = *((unsigned int *) key);
> +		*out_id = msg_id;
> +
> +	sr_assembly_remove_fragment_backup(assembly->imsi, node,
> +					&status_report->status_report.raddr,
> +					msg_id);
>  
>  	g_hash_table_iter_remove(&iter);
>  
> @@ -2804,6 +2985,7 @@ void status_report_assembly_add_fragment(
>  	node->mrs[offset] |= bit;
>  	node->expiration = expiration;
>  	node->sent_mrs++;
> +	sr_assembly_add_fragment_backup(assembly->imsi, node, to, msg_id);
>  }
>  
>  void status_report_assembly_expire(struct status_report_assembly *assembly,
> diff --git a/src/smsutil.h b/src/smsutil.h
> index eb70b6d..3c6b3ae 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -370,7 +370,7 @@ struct id_table_node {
>  	unsigned char total_mrs;
>  	unsigned char sent_mrs;
>  	gboolean deliverable;
> -};
> +} __attribute__((packed));
>  
>  struct status_report_assembly {
>  	const char *imsi;

Rest looks fine.

Thanks,
-Denis

Re: [RFC PATCH v2 2/4] smsutil: storing/loading sms status report over reboot

[Petteri Tikander]

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

Hi Denis,
and thanks for the comments.
I'm changing the code according to your comments. So I'll change backup-file 
system to more like flat-system. Earlier messages were stored in address-based 
subdirectories, and now, if I understood correctly, those sub-directories 
shouldn't exist anymore. But instead, should be stored in next form in the 
same directory:

sms_addr_1_msg1...
sms_addr_1_msgn
sms_addr_2_msg1...
sms_addr_2_msgn

OK. I already made some implementation, but for some strange reason my testing 
crashes after first sms-message and status-notify (OK, that works, but next 
sms-message sequencies fail):(
I have to investigate tomorrow, what's wrong in my code/environment. At least 
my modem seems not to work properly all the time, because similar problems 
occur, when I execute 0.26-version.

Br, Petteri

> Hi Petteri,
> 
> On 08/19/2010 12:41 PM, Petteri Tikander wrote:
> > ---
> >  src/smsutil.c |  192
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- src/smsutil.h |
> >    2 +-
> >  2 files changed, 188 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/smsutil.c b/src/smsutil.c
> > index c60b8ec..b001a1d 100644
> > --- a/src/smsutil.c
> > +++ b/src/smsutil.c
> > @@ -45,6 +45,10 @@
> >  #define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
> >  #define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
> >
> > +#define SMS_SR_BACKUP_PATH STORAGEDIR "/%s/sms_sr"
> > +#define SMS_SR_BACKUP_PATH_DIR SMS_SR_BACKUP_PATH "/%s"
> > +#define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH_DIR "/%i"
> > +
> >  #define SMS_ADDR_FMT "%24[0-9A-F]"
> >
> >  static GSList *sms_assembly_add_fragment_backup(struct sms_assembly
> > *assembly, @@ -2413,7 +2417,7 @@ static void
> > sms_assembly_backup_free(struct sms_assembly *assembly, {
> >       char *path;
> >       int seq;
> > -     char straddr[25];
> > +     DECLARE_SMS_ADDR_STR(straddr);
> >
> >       if (!assembly->imsi)
> >               return;
> > @@ -2642,20 +2646,182 @@ void sms_assembly_expire(struct sms_assembly
> > *assembly, time_t before) }
> >  }
> >
> > +static void sr_assembly_load_backup(GHashTable *assembly_table,
> > +                                     const char *imsi,
> > +                                     const struct dirent *addr_dir)
> > +{
> > +     char *path;
> > +     struct dirent **ids;
> > +     struct sms_address addr;
> > +     DECLARE_SMS_ADDR_STR(straddr);
> > +     struct id_table_node *node;
> > +     GHashTable *id_table;
> > +     int len;
> > +     int r;
> > +     char *assembly_table_key;
> > +     unsigned int *id_table_key;
> > +     struct stat segment_stat;
> > +
> > +     if (addr_dir->d_type != DT_DIR)
> > +             return;
> > +
> > +     /* Max of SMS address size is 12 bytes, hex encoded */
> > +     if (sscanf(addr_dir->d_name, SMS_ADDR_FMT, straddr) < 1)
> > +             return;
> > +
> 
> Potential suggestion might be to use a flatter directory structure and
> do something like SMS_ADDR_FMT "-%d"...
> 
> > +     if (sms_assembly_extract_address(straddr, &addr) == FALSE)
> > +             return;
> > +
> > +     /* Go through different msg_ids. */
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
> > +                                                     addr_dir->d_name);
> > +     len = scandir(path, &ids, NULL, versionsort);
> > +
> > +     g_free(path);
> > +
> > +     if (len < 0)
> > +             return;
> > +
> > +     id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> > +                                                     g_free, g_free);
> > +
> > +     assembly_table_key = g_try_malloc(sizeof(addr.address));
> > +
> > +     if (assembly_table_key == NULL)
> > +             return;
> > +
> > +     assembly_table_key = g_strdup(sms_address_to_string(&addr));
> 
> g_strdup already mallocs the needed space.  The above g_try_malloc is
> only leaking memory.
> 
> > +     g_hash_table_insert(assembly_table, assembly_table_key, id_table);
> > +
> > +     while (len--) {
> > +             path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s/%s",
> > +                             imsi, addr_dir->d_name, ids[len]->d_name);
> > +
> 
> You have to make sure to sanitize ids[len]->d_name.  If it is not an
> integer, then it needs to be skipped.  Also watch out for atoi() use
> below, strtol is a much better alternative.
> 
> > +             node = g_new0(struct id_table_node, 1);
> > +
> > +             r = read_file((unsigned char *) node,
> > +                             sizeof(struct id_table_node),
> > +                             SMS_SR_BACKUP_PATH "/%s/%s",
> > +                             imsi, addr_dir->d_name, ids[len]->d_name);
> > +
> > +             if (r < 0) {
> > +                     g_free(path);
> > +                     g_free(ids[len]);
> > +                     g_free(node);
> > +                     continue;
> > +             }
> > +
> > +             r = stat(path, &segment_stat);
> 
> Hah, for sms_assembly we were extra tricky and used the modification
> time stamp on the file system as the received time stamp in the node
> structure.  This works because the file is written at the same time.
> Since we're being lazy and writing out the structure directly this part
> is not needed.
> 
> > +
> > +             if (r != 0) {
> > +                     g_free(path);
> > +                     g_free(ids[len]);
> > +                     g_free(node);
> > +                     continue;
> > +             }
> > +
> > +             /* Node ready, create key and add them to the table */
> > +             id_table_key = g_new0(unsigned int, 1);
> > +             *id_table_key = atoi(ids[len]->d_name);
> > +
> > +             g_hash_table_insert(id_table, id_table_key, node);
> > +
> > +             g_free(path);
> > +             g_free(ids[len]);
> > +     }
> > +     g_free(ids);
> > +}
> > +
> >  struct status_report_assembly *status_report_assembly_new(const char
> > *imsi) {
> > +     char *path;
> > +     int len;
> > +     struct dirent **addresses;
> >       struct status_report_assembly *ret =
> >                               g_new0(struct status_report_assembly, 1);
> >
> >       ret->assembly_table = g_hash_table_new_full(g_str_hash,
> > g_str_equal, g_free, (GDestroyNotify)g_hash_table_destroy);
> >
> > -     if (imsi)
> > +     if (imsi) {
> >               ret->imsi = imsi;
> >
> > +             /* Restore state from backup */
> > +             path = g_strdup_printf(SMS_SR_BACKUP_PATH, imsi);
> > +             len = scandir(path, &addresses, NULL, alphasort);
> > +
> > +             g_free(path);
> > +
> > +             if (len < 0)
> > +                     return ret;
> > +
> > +             /* Go through different addresses. Each address can relate
> > to +              * 1-n msg_ids.
> > +              */
> 
> Please stick to our multi-line comment guidelines, doc/coding-style.txt
> item M2.  And yes I know the sms_assembly does it this way :)
> 
> > +
> > +             while (len--) {
> > +                     sr_assembly_load_backup(ret->assembly_table, imsi,
> > +                                                            
> > addresses[len]); +                     g_free(addresses[len]);
> > +             }
> > +
> > +             g_free(addresses);
> > +     }
> > +
> 
> This part looks good otherwise
> 
> >       return ret;
> >  }
> >
> > +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> > +                                     const struct id_table_node *node,
> > +                                     const struct sms_address *addr,
> > +                                     unsigned int msg_id)
> > +{
> > +     int len = sizeof(struct id_table_node);
> > +     DECLARE_SMS_ADDR_STR(straddr);
> > +
> > +     if (!imsi)
> > +             return FALSE;
> > +
> > +     if (sms_address_to_hex_string(addr, straddr) == FALSE)
> > +             return FALSE;
> > +
> > +     /* storagedir/%s/sms_sr/%s/%i */
> > +     if (write_file((unsigned char *) node, len, SMS_BACKUP_MODE,
> > +                     SMS_SR_BACKUP_PATH_FILE, imsi,
> > +                     straddr, msg_id) != len)
> > +             return FALSE;
> > +
> > +     return TRUE;
> > +}
> > +
> > +static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> > +                                     const struct id_table_node *node,
> > +                                     const struct sms_address *addr,
> > +                                     unsigned int msg_id)
> > +{
> > +     char *path;
> > +     DECLARE_SMS_ADDR_STR(straddr);
> > +
> > +     if (!imsi)
> > +             return FALSE;
> > +
> > +     if (sms_address_to_hex_string(addr, straddr) == FALSE)
> > +             return FALSE;
> > +
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr,
> > msg_id); +
> > +     unlink(path);
> > +     g_free(path);
> > +
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
> > +
> > +     /* If the address does not have relating msg_ids anymore, remove it
> > */ +     rmdir(path);
> > +     g_free(path);
> > +
> > +     return TRUE;
> > +}
> > +
> 
> Looks good, but see my earlier comment above about a flatter file
>  structure.
> 
> >  void status_report_assembly_free(struct status_report_assembly
> > *assembly) {
> >       g_hash_table_destroy(assembly->assembly_table);
> > @@ -2698,6 +2864,7 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, GHashTableIter iter;
> >       gboolean pending;
> >       int i;
> > +     unsigned int msg_id;
> >
> >       /* We ignore temporary or tempfinal status reports */
> >       if (sr_st_to_delivered(status_report->status_report.st,
> > @@ -2714,7 +2881,6 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, g_hash_table_iter_init(&iter,
> > id_table);
> >       while (g_hash_table_iter_next(&iter, &key, &value)) {
> >               node = value;
> > -
> 
> This newline is actually needed, please see doc/coding-style.txt item M1
> 
> >               if (node->mrs[offset] & bit)
> >                       break;
> >
> > @@ -2743,14 +2909,29 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, }
> >       }
> >
> > -     if (pending == TRUE && node->deliverable == TRUE)
> > +     msg_id = *(unsigned int *) key;
> > +
> > +     if (pending == TRUE && node->deliverable == TRUE) {
> > +             /* More status reports expected, and already received
> > +              * reports completed. Update backup file.
> > +              */
> 
> Coding style item M2
> 
> > +             sr_assembly_add_fragment_backup(
> > +                                     assembly->imsi, node,
> > +                                    
> > &status_report->status_report.raddr, +                                   
> >  msg_id);
> > +
> >               return FALSE;
> > +     }
> >
> >       if (out_delivered)
> >               *out_delivered = node->deliverable;
> >
> >       if (out_id)
> > -             *out_id = *((unsigned int *) key);
> > +             *out_id = msg_id;
> > +
> > +     sr_assembly_remove_fragment_backup(assembly->imsi, node,
> > +                                    
> > &status_report->status_report.raddr, +                                   
> >  msg_id);
> >
> >       g_hash_table_iter_remove(&iter);
> >
> > @@ -2804,6 +2985,7 @@ void status_report_assembly_add_fragment(
> >       node->mrs[offset] |= bit;
> >       node->expiration = expiration;
> >       node->sent_mrs++;
> > +     sr_assembly_add_fragment_backup(assembly->imsi, node, to, msg_id);
> >  }
> >
> >  void status_report_assembly_expire(struct status_report_assembly
> > *assembly, diff --git a/src/smsutil.h b/src/smsutil.h
> > index eb70b6d..3c6b3ae 100644
> > --- a/src/smsutil.h
> > +++ b/src/smsutil.h
> > @@ -370,7 +370,7 @@ struct id_table_node {
> >       unsigned char total_mrs;
> >       unsigned char sent_mrs;
> >       gboolean deliverable;
> > -};
> > +} __attribute__((packed));
> >
> >  struct status_report_assembly {
> >       const char *imsi;
> 
> Rest looks fine.
> 
> Thanks,
> -Denis
>