[RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function

[RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function

[Petteri Tikander]

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

---
 src/smsutil.c |  202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 198 insertions(+), 4 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 22c70cf..0972988 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-%i-%i"
+#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,
@@ -2642,20 +2646,204 @@ 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 sms_addr, *addr;
+	struct id_table_node *node;
+	GHashTable *id_table;
+	int len;
+	int r;
+	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
+			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
+	char *assembly_table_key;
+	unsigned int *id_table_key;
+	struct stat segment_stat;
+
+	if (addr_dir->d_type != DT_DIR)
+		return;
+
+	addr = &sms_addr;
+
+	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-%i-%i",
+			addr->address, (int *) &addr->number_type,
+			(int *) &addr->numbering_plan) < 3) {
+		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;
+
+	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
+	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);
+		r = read_file(buf, sizeof(buf), 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]);
+			continue;
+		}
+
+		r = stat(path, &segment_stat);
+
+		if (r != 0) {
+			g_free(path);
+			g_free(ids[len]);
+			continue;
+		}
+		/* Gather the data for id_table node */
+		node = g_new0(struct id_table_node, 1);
+		memcpy(&node->to, addr, sizeof(*addr));
+		node->expiration = segment_stat.st_mtime;
+		memcpy(node->mrs, buf, sizeof(node->mrs));
+		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
+						sizeof(node->total_mrs));
+		memcpy(&node->sent_mrs,
+			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
+			sizeof(node->sent_mrs));
+
+		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
+			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
+			sizeof(node->deliverable));
+		/* 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,
+					unsigned int msg_id)
+{
+	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
+			sizeof(node->sent_mrs) + sizeof(node->deliverable);
+	unsigned char buf[len];
+
+	if (!imsi)
+		return FALSE;
+
+	memcpy(buf, node->mrs, sizeof(node->mrs));
+
+	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
+					sizeof(node->total_mrs));
+
+	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
+				&node->sent_mrs, sizeof(node->sent_mrs));
+
+	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
+	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
+
+	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
+	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
+			node->to.address, node->to.number_type,
+			node->to.numbering_plan, msg_id) != len)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					unsigned int msg_id)
+{
+	char *path;
+
+	if (!imsi)
+		return FALSE;
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
+				node->to.number_type, node->to.numbering_plan,
+				msg_id);
+
+	unlink(path);
+	g_free(path);
+
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, node->to.address,
+				node->to.number_type, node->to.numbering_plan);
+
+	/* If the address does not have relating msg_ids anymore, remove it */
+	rmdir(path);
+	g_free(path);
+
+	return TRUE;
+}
+
+static gboolean sr_assembly_update_fragment_backup(const char *imsi,
+					const struct id_table_node *node,
+					unsigned int msg_id)
+{
+	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
+			sr_assembly_add_fragment_backup(imsi, node, msg_id);
+}
+
 void status_report_assembly_free(struct status_report_assembly *assembly)
 {
 	g_hash_table_destroy(assembly->assembly_table);
@@ -2714,7 +2902,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;
 
@@ -2724,7 +2911,6 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	/* Unable to find a message reference belonging to this address */
 	if (node == NULL)
 		return FALSE;
-
 	/* Mr belongs to this node. */
 	node->mrs[offset] ^= bit;
 
@@ -2743,8 +2929,12 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 		}
 	}
 
-	if (pending == TRUE && node->deliverable == TRUE)
+	if (pending == TRUE && node->deliverable == TRUE) {
+		sr_assembly_add_fragment_backup(assembly->imsi, node,
+						*((unsigned int *) key));
+
 		return FALSE;
+	}
 
 	if (out_delivered)
 		*out_delivered = node->deliverable;
@@ -2752,6 +2942,9 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	if (out_id)
 		*out_id = *((unsigned int *) key);
 
+	sr_assembly_remove_fragment_backup(assembly->imsi, node,
+						*((unsigned int *) key));
+
 	g_hash_table_iter_remove(&iter);
 
 	if (g_hash_table_size(id_table) == 0)
@@ -2805,6 +2998,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, msg_id);
 }
 
 void status_report_assembly_expire(struct status_report_assembly *assembly,
-- 
1.6.3.3

[RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function

[Petteri Tikander]

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

---
 src/smsutil.c |   54 ++++++++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 0972988..25e405c 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -46,7 +46,7 @@
 #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-%i-%i"
+#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]"
@@ -2417,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;
@@ -2652,7 +2652,8 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 {
 	char *path;
 	struct dirent **ids;
-	struct sms_address sms_addr, *addr;
+	struct sms_address addr;
+	DECLARE_SMS_ADDR_STR(straddr);
 	struct id_table_node *node;
 	GHashTable *id_table;
 	int len;
@@ -2666,13 +2667,12 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 	if (addr_dir->d_type != DT_DIR)
 		return;
 
-	addr = &sms_addr;
+	/* Max of SMS address size is 12 bytes, hex encoded */
+	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT, straddr) < 1)
+		return;
 
-	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-%i-%i",
-			addr->address, (int *) &addr->number_type,
-			(int *) &addr->numbering_plan) < 3) {
+	if (sms_assembly_extract_address(straddr, &addr) == FALSE)
 		return;
-	}
 
 	/* Go through different msg_ids. */
 	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
@@ -2687,12 +2687,12 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 	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));
+	assembly_table_key = g_try_malloc(sizeof(addr.address));
 
 	if (assembly_table_key == NULL)
 		return;
 
-	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
+	g_strlcpy(assembly_table_key, addr.address, sizeof(addr.address));
 	g_hash_table_insert(assembly_table, assembly_table_key, id_table);
 
 	while (len--) {
@@ -2716,7 +2716,7 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 		}
 		/* Gather the data for id_table node */
 		node = g_new0(struct id_table_node, 1);
-		memcpy(&node->to, addr, sizeof(*addr));
+		memcpy(&node->to, &addr, sizeof(addr));
 		node->expiration = segment_stat.st_mtime;
 		memcpy(node->mrs, buf, sizeof(node->mrs));
 		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
@@ -2766,7 +2766,6 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
 		/* 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]);
@@ -2786,10 +2785,14 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
 	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
 			sizeof(node->sent_mrs) + sizeof(node->deliverable);
 	unsigned char buf[len];
+	DECLARE_SMS_ADDR_STR(straddr);
 
 	if (!imsi)
 		return FALSE;
 
+	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
+		return FALSE;
+
 	memcpy(buf, node->mrs, sizeof(node->mrs));
 
 	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
@@ -2801,10 +2804,9 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
 	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
 	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
 
-	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
+	/* storagedir/%s/sms_sr/%s/%i */
 	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
-			node->to.address, node->to.number_type,
-			node->to.numbering_plan, msg_id) != len)
+			straddr, msg_id) != len)
 		return FALSE;
 
 	return TRUE;
@@ -2815,19 +2817,20 @@ static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
 					unsigned int msg_id)
 {
 	char *path;
+	DECLARE_SMS_ADDR_STR(straddr);
 
 	if (!imsi)
 		return FALSE;
 
-	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
-				node->to.number_type, node->to.numbering_plan,
-				msg_id);
+	if (sms_address_to_hex_string(&node->to, 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, node->to.address,
-				node->to.number_type, node->to.numbering_plan);
+	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);
@@ -2836,14 +2839,6 @@ static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
 	return TRUE;
 }
 
-static gboolean sr_assembly_update_fragment_backup(const char *imsi,
-					const struct id_table_node *node,
-					unsigned int msg_id)
-{
-	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
-			sr_assembly_add_fragment_backup(imsi, node, msg_id);
-}
-
 void status_report_assembly_free(struct status_report_assembly *assembly)
 {
 	g_hash_table_destroy(assembly->assembly_table);
@@ -2930,6 +2925,9 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	}
 
 	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,
 						*((unsigned int *) key));
 
-- 
1.6.3.3

[RFC_PATCH 4/4] smsutil: added function for data handling of status report

[Petteri Tikander]

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

---
 src/smsutil.c |  158 +++++++++++++++++++++++++++++++++------------------------
 1 files changed, 92 insertions(+), 66 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 25e405c..a12bede 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -57,6 +57,15 @@ static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
 					guint16 ref, guint8 max, guint8 seq,
 					gboolean backup);
 
+static void sr_assembly_add_fragment_backup(
+					struct status_report_assembly *assembly,
+					unsigned int msg_id, time_t ts,
+					const struct sms_address *to,
+					unsigned int *mrs,
+					unsigned char total_mrs,
+					unsigned char sent_mrs,
+					gboolean backup);
+
 /*
  * This function uses the meanings of digits 10..15 according to the rules
  * defined in 23.040 Section 9.1.2.3 and 24.008 Table 10.5.118
@@ -2646,22 +2655,19 @@ 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)
+static void sr_assembly_load_backup(
+				struct status_report_assembly *assembly_table,
+				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;
 	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
 			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
-	char *assembly_table_key;
-	unsigned int *id_table_key;
 	struct stat segment_stat;
 
 	if (addr_dir->d_type != DT_DIR)
@@ -2675,7 +2681,7 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 		return;
 
 	/* Go through different msg_ids. */
-	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", assembly_table->imsi,
 							addr_dir->d_name);
 	len = scandir(path, &ids, NULL, versionsort);
 
@@ -2684,22 +2690,13 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 	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;
-
-	g_strlcpy(assembly_table_key, addr.address, sizeof(addr.address));
-	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);
+				assembly_table->imsi, addr_dir->d_name,
+				ids[len]->d_name);
 		r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH "/%s/%s",
-				imsi, addr_dir->d_name, ids[len]->d_name);
+				assembly_table->imsi, addr_dir->d_name,
+				ids[len]->d_name);
 
 		if (r < 0) {
 			g_free(path);
@@ -2714,29 +2711,21 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
 			g_free(ids[len]);
 			continue;
 		}
-		/* Gather the data for id_table node */
-		node = g_new0(struct id_table_node, 1);
-		memcpy(&node->to, &addr, sizeof(addr));
-		node->expiration = segment_stat.st_mtime;
-		memcpy(node->mrs, buf, sizeof(node->mrs));
-		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
-						sizeof(node->total_mrs));
-		memcpy(&node->sent_mrs,
-			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
-			sizeof(node->sent_mrs));
-
-		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
-			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
-			sizeof(node->deliverable));
-		/* 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);
+		sr_assembly_add_fragment_backup(assembly_table,
+						atoi(ids[len]->d_name),
+						segment_stat.st_mtime,
+						&addr,
+						(unsigned int *) &buf[0],
+						buf[sizeof(node->mrs)],
+						buf[sizeof(node->mrs) +
+						sizeof(node->total_mrs)],
+						FALSE);
 
 		g_free(path);
 		g_free(ids[len]);
 	}
+
 	g_free(ids);
 }
 
@@ -2767,8 +2756,7 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
 		 * 1-n msg_ids.
 		 */
 		while (len--) {
-			sr_assembly_load_backup(ret->assembly_table, imsi,
-								addresses[len]);
+			sr_assembly_load_backup(ret, addresses[len]);
 			g_free(addresses[len]);
 		}
 
@@ -2778,16 +2766,17 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
 	return ret;
 }
 
-static gboolean sr_assembly_add_fragment_backup(const char *imsi,
-					const struct id_table_node *node,
-					unsigned int msg_id)
+static gboolean sr_assembly_backup_store(
+				struct status_report_assembly *assembly,
+				const struct id_table_node *node,
+				unsigned int msg_id)
 {
 	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
 			sizeof(node->sent_mrs) + sizeof(node->deliverable);
 	unsigned char buf[len];
 	DECLARE_SMS_ADDR_STR(straddr);
 
-	if (!imsi)
+	if (!assembly->imsi)
 		return FALSE;
 
 	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
@@ -2805,32 +2794,35 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
 	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
 
 	/* storagedir/%s/sms_sr/%s/%i */
-	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
+	if (write_file(buf, len, SMS_BACKUP_MODE,
+			SMS_SR_BACKUP_PATH_FILE, assembly->imsi,
 			straddr, msg_id) != len)
 		return FALSE;
 
 	return TRUE;
 }
 
-static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
+static gboolean sr_assembly_backup_free(struct status_report_assembly *assembly,
 					const struct id_table_node *node,
 					unsigned int msg_id)
 {
 	char *path;
 	DECLARE_SMS_ADDR_STR(straddr);
 
-	if (!imsi)
+	if (!assembly->imsi)
 		return FALSE;
 
 	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
 		return FALSE;
 
-	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr, msg_id);
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, assembly->imsi,
+				straddr, msg_id);
 
 	unlink(path);
 	g_free(path);
 
-	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
+	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, assembly->imsi,
+				straddr);
 
 	/* If the address does not have relating msg_ids anymore, remove it */
 	rmdir(path);
@@ -2928,8 +2920,8 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 		/* More status reports expected, and already received
 		   reports completed. Update backup file.
 		 */
-		sr_assembly_add_fragment_backup(assembly->imsi, node,
-						*((unsigned int *) key));
+		sr_assembly_backup_store(assembly, node,
+					 *((unsigned int *) key));
 
 		return FALSE;
 	}
@@ -2940,8 +2932,7 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	if (out_id)
 		*out_id = *((unsigned int *) key);
 
-	sr_assembly_remove_fragment_backup(assembly->imsi, node,
-						*((unsigned int *) key));
+	sr_assembly_backup_free(assembly, node,	*((unsigned int *) key));
 
 	g_hash_table_iter_remove(&iter);
 
@@ -2952,18 +2943,18 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
 	return TRUE;
 }
 
-void status_report_assembly_add_fragment(
-					struct status_report_assembly *assembly,
-					unsigned int msg_id,
-					const struct sms_address *to,
-					unsigned char mr, time_t expiration,
-					unsigned char total_mrs)
+static void sr_assembly_add_fragment_backup(
+				struct status_report_assembly *assembly,
+				unsigned int msg_id, time_t ts,
+				const struct sms_address *to,
+				unsigned int *mrs, unsigned char total_mrs,
+				unsigned char sent_mrs, gboolean backup)
 {
-	unsigned int offset = mr / 32;
-	unsigned int bit = 1 << (mr % 32);
 	GHashTable *id_table;
 	struct id_table_node *node;
 	unsigned int *id_table_key;
+	int i;
+	int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
 
 	id_table = g_hash_table_lookup(assembly->assembly_table,
 					sms_address_to_string(to));
@@ -2971,7 +2962,7 @@ void status_report_assembly_add_fragment(
 	/* Create hashtable keyed by the to address if required */
 	if (id_table == NULL) {
 		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
-								g_free, g_free);
+						g_free, g_free);
 		g_hash_table_insert(assembly->assembly_table,
 					g_strdup(sms_address_to_string(to)),
 					id_table);
@@ -2993,10 +2984,45 @@ void status_report_assembly_add_fragment(
 	}
 
 	/* id_table and node both exists */
-	node->mrs[offset] |= bit;
-	node->expiration = expiration;
-	node->sent_mrs++;
-	sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
+
+	for (i = 0; i < len; i++)
+		node->mrs[i] |= mrs[i];
+
+	node->expiration = ts;
+
+	/* storing to the backup-file is needed, when new SMS-message is sent */
+	if (backup) {
+		node->sent_mrs++;
+		sr_assembly_backup_store(assembly, node, msg_id);
+	}
+	/* storing to the backup-file is not needed, when backup is loaded */
+	else
+		node->sent_mrs = sent_mrs;
+}
+
+void status_report_assembly_add_fragment(
+				struct status_report_assembly *assembly,
+				unsigned int msg_id,
+				const struct sms_address *to,
+				unsigned char mr, time_t expiration,
+				unsigned char total_mrs)
+{
+	struct id_table_node *node;
+	unsigned int offset = mr / 32;
+	unsigned int bit = 1 << (mr % 32);
+	int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
+	unsigned int mrs[len];
+
+	memset(mrs, 0, len);
+
+	/* set correct bit corresponding to the
+	 *  received mr-number in mr-buffer
+	 */
+	mrs[offset] |= bit;
+
+	sr_assembly_add_fragment_backup(assembly, msg_id, expiration,
+					to, mrs, total_mrs,
+					FALSE, TRUE);
 }
 
 void status_report_assembly_expire(struct status_report_assembly *assembly,
-- 
1.6.3.3

Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report

[Denis Kenzior]

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

Hi Petteri,

On 08/16/2010 02:08 AM, Petteri Tikander wrote:
> ---
>  src/smsutil.c |  158 +++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 92 insertions(+), 66 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 25e405c..a12bede 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -57,6 +57,15 @@ static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
>  					guint16 ref, guint8 max, guint8 seq,
>  					gboolean backup);
>  
> +static void sr_assembly_add_fragment_backup(
> +					struct status_report_assembly *assembly,
> +					unsigned int msg_id, time_t ts,
> +					const struct sms_address *to,
> +					unsigned int *mrs,
> +					unsigned char total_mrs,
> +					unsigned char sent_mrs,
> +					gboolean backup);
> +

Please avoid forward-declarations unless absolutely necessary.  It is
better to just move the function upwards.

>  /*
>   * This function uses the meanings of digits 10..15 according to the rules
>   * defined in 23.040 Section 9.1.2.3 and 24.008 Table 10.5.118
> @@ -2646,22 +2655,19 @@ 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)
> +static void sr_assembly_load_backup(
> +				struct status_report_assembly *assembly_table,
> +				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;
>  	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
>  			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
> -	char *assembly_table_key;
> -	unsigned int *id_table_key;
>  	struct stat segment_stat;
>  
>  	if (addr_dir->d_type != DT_DIR)
> @@ -2675,7 +2681,7 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  		return;
>  
>  	/* Go through different msg_ids. */
> -	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", assembly_table->imsi,
>  							addr_dir->d_name);
>  	len = scandir(path, &ids, NULL, versionsort);
>  
> @@ -2684,22 +2690,13 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  	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;
> -
> -	g_strlcpy(assembly_table_key, addr.address, sizeof(addr.address));
> -	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);
> +				assembly_table->imsi, addr_dir->d_name,
> +				ids[len]->d_name);
>  		r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH "/%s/%s",
> -				imsi, addr_dir->d_name, ids[len]->d_name);
> +				assembly_table->imsi, addr_dir->d_name,
> +				ids[len]->d_name);
>  
>  		if (r < 0) {
>  			g_free(path);
> @@ -2714,29 +2711,21 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  			g_free(ids[len]);
>  			continue;
>  		}
> -		/* Gather the data for id_table node */
> -		node = g_new0(struct id_table_node, 1);
> -		memcpy(&node->to, &addr, sizeof(addr));
> -		node->expiration = segment_stat.st_mtime;
> -		memcpy(node->mrs, buf, sizeof(node->mrs));
> -		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> -						sizeof(node->total_mrs));
> -		memcpy(&node->sent_mrs,
> -			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> -			sizeof(node->sent_mrs));
> -
> -		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
> -			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
> -			sizeof(node->deliverable));
> -		/* 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);
> +		sr_assembly_add_fragment_backup(assembly_table,
> +						atoi(ids[len]->d_name),
> +						segment_stat.st_mtime,
> +						&addr,
> +						(unsigned int *) &buf[0],
> +						buf[sizeof(node->mrs)],
> +						buf[sizeof(node->mrs) +
> +						sizeof(node->total_mrs)],
> +						FALSE);
>  

If we can write / read the node structure directly, then I'm actually
thinking we won't be needing this patch at all.

>  		g_free(path);
>  		g_free(ids[len]);
>  	}
> +
>  	g_free(ids);
>  }
>  
> @@ -2767,8 +2756,7 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
>  		 * 1-n msg_ids.
>  		 */
>  		while (len--) {
> -			sr_assembly_load_backup(ret->assembly_table, imsi,
> -								addresses[len]);
> +			sr_assembly_load_backup(ret, addresses[len]);
>  			g_free(addresses[len]);
>  		}
>  
> @@ -2778,16 +2766,17 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
>  	return ret;
>  }
>  
> -static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> -					const struct id_table_node *node,
> -					unsigned int msg_id)
> +static gboolean sr_assembly_backup_store(
> +				struct status_report_assembly *assembly,
> +				const struct id_table_node *node,
> +				unsigned int msg_id)
>  {
>  	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
>  			sizeof(node->sent_mrs) + sizeof(node->deliverable);
>  	unsigned char buf[len];
>  	DECLARE_SMS_ADDR_STR(straddr);
>  
> -	if (!imsi)
> +	if (!assembly->imsi)
>  		return FALSE;
>  
>  	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
> @@ -2805,32 +2794,35 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
>  	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
>  
>  	/* storagedir/%s/sms_sr/%s/%i */
> -	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
> +	if (write_file(buf, len, SMS_BACKUP_MODE,
> +			SMS_SR_BACKUP_PATH_FILE, assembly->imsi,
>  			straddr, msg_id) != len)
>  		return FALSE;
>  
>  	return TRUE;
>  }
>  
> -static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> +static gboolean sr_assembly_backup_free(struct status_report_assembly *assembly,
>  					const struct id_table_node *node,
>  					unsigned int msg_id)
>  {
>  	char *path;
>  	DECLARE_SMS_ADDR_STR(straddr);
>  
> -	if (!imsi)
> +	if (!assembly->imsi)
>  		return FALSE;
>  
>  	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
>  		return FALSE;
>  
> -	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr, msg_id);
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, assembly->imsi,
> +				straddr, msg_id);
>  
>  	unlink(path);
>  	g_free(path);
>  
> -	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, assembly->imsi,
> +				straddr);
>  
>  	/* If the address does not have relating msg_ids anymore, remove it */
>  	rmdir(path);
> @@ -2928,8 +2920,8 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		/* More status reports expected, and already received
>  		   reports completed. Update backup file.
>  		 */
> -		sr_assembly_add_fragment_backup(assembly->imsi, node,
> -						*((unsigned int *) key));
> +		sr_assembly_backup_store(assembly, node,
> +					 *((unsigned int *) key));
>  
>  		return FALSE;
>  	}
> @@ -2940,8 +2932,7 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	if (out_id)
>  		*out_id = *((unsigned int *) key);
>  
> -	sr_assembly_remove_fragment_backup(assembly->imsi, node,
> -						*((unsigned int *) key));
> +	sr_assembly_backup_free(assembly, node,	*((unsigned int *) key));
>  
>  	g_hash_table_iter_remove(&iter);
>  
> @@ -2952,18 +2943,18 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	return TRUE;
>  }
>  
> -void status_report_assembly_add_fragment(
> -					struct status_report_assembly *assembly,
> -					unsigned int msg_id,
> -					const struct sms_address *to,
> -					unsigned char mr, time_t expiration,
> -					unsigned char total_mrs)
> +static void sr_assembly_add_fragment_backup(
> +				struct status_report_assembly *assembly,
> +				unsigned int msg_id, time_t ts,
> +				const struct sms_address *to,
> +				unsigned int *mrs, unsigned char total_mrs,
> +				unsigned char sent_mrs, gboolean backup)
>  {
> -	unsigned int offset = mr / 32;
> -	unsigned int bit = 1 << (mr % 32);
>  	GHashTable *id_table;
>  	struct id_table_node *node;
>  	unsigned int *id_table_key;
> +	int i;
> +	int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
>  
>  	id_table = g_hash_table_lookup(assembly->assembly_table,
>  					sms_address_to_string(to));
> @@ -2971,7 +2962,7 @@ void status_report_assembly_add_fragment(
>  	/* Create hashtable keyed by the to address if required */
>  	if (id_table == NULL) {
>  		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> -								g_free, g_free);
> +						g_free, g_free);
>  		g_hash_table_insert(assembly->assembly_table,
>  					g_strdup(sms_address_to_string(to)),
>  					id_table);
> @@ -2993,10 +2984,45 @@ void status_report_assembly_add_fragment(
>  	}
>  
>  	/* id_table and node both exists */
> -	node->mrs[offset] |= bit;
> -	node->expiration = expiration;
> -	node->sent_mrs++;
> -	sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
> +
> +	for (i = 0; i < len; i++)
> +		node->mrs[i] |= mrs[i];
> +
> +	node->expiration = ts;
> +
> +	/* storing to the backup-file is needed, when new SMS-message is sent */
> +	if (backup) {
> +		node->sent_mrs++;
> +		sr_assembly_backup_store(assembly, node, msg_id);
> +	}
> +	/* storing to the backup-file is not needed, when backup is loaded */
> +	else
> +		node->sent_mrs = sent_mrs;
> +}
> +
> +void status_report_assembly_add_fragment(
> +				struct status_report_assembly *assembly,
> +				unsigned int msg_id,
> +				const struct sms_address *to,
> +				unsigned char mr, time_t expiration,
> +				unsigned char total_mrs)
> +{
> +	struct id_table_node *node;
> +	unsigned int offset = mr / 32;
> +	unsigned int bit = 1 << (mr % 32);
> +	int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
> +	unsigned int mrs[len];
> +
> +	memset(mrs, 0, len);
> +
> +	/* set correct bit corresponding to the
> +	 *  received mr-number in mr-buffer
> +	 */
> +	mrs[offset] |= bit;
> +

The memset above and this looks fishy to me...

> +	sr_assembly_add_fragment_backup(assembly, msg_id, expiration,
> +					to, mrs, total_mrs,
> +					FALSE, TRUE);
>  }
>  
>  void status_report_assembly_expire(struct status_report_assembly *assembly,

Regards,
-Denis

Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report

[Petteri Tikander]

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

Hi Denis,

> Hi Petteri,
> 
> On 08/16/2010 02:08 AM, Petteri Tikander wrote:
> > ---
> >  src/smsutil.c |  158
> > +++++++++++++++++++++++++++++++++------------------------ 1 files
> > changed, 92 insertions(+), 66 deletions(-)
> >
> > diff --git a/src/smsutil.c b/src/smsutil.c
> > index 25e405c..a12bede 100644
> > --- a/src/smsutil.c
> > +++ b/src/smsutil.c
> > @@ -57,6 +57,15 @@ static GSList *sms_assembly_add_fragment_backup(struct
> > sms_assembly *assembly, guint16 ref, guint8 max, guint8 seq, gboolean
> > backup);
> >
> > +static void sr_assembly_add_fragment_backup(
> > +                                     struct status_report_assembly
> > *assembly, +                                     unsigned int msg_id,
> > time_t ts, +                                     const struct sms_address
> > *to, +                                     unsigned int *mrs,
> > +                                     unsigned char total_mrs,
> > +                                     unsigned char sent_mrs,
> > +                                     gboolean backup);
> > +
> 
> Please avoid forward-declarations unless absolutely necessary.  It is
> better to just move the function upwards.
> 
> >  /*
> >   * This function uses the meanings of digits 10..15 according to the
> > rules * defined in 23.040 Section 9.1.2.3 and 24.008 Table 10.5.118
> > @@ -2646,22 +2655,19 @@ 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)
> > +static void sr_assembly_load_backup(
> > +                             struct status_report_assembly
> > *assembly_table, +                             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;
> >       unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
> >                       sizeof(node->sent_mrs) +
> > sizeof(node->deliverable)]; -     char *assembly_table_key;
> > -     unsigned int *id_table_key;
> >       struct stat segment_stat;
> >
> >       if (addr_dir->d_type != DT_DIR)
> > @@ -2675,7 +2681,7 @@ static void sr_assembly_load_backup(GHashTable
> > *assembly_table, return;
> >
> >       /* Go through different msg_ids. */
> > -     path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s",
> > assembly_table->imsi, addr_dir->d_name); len = scandir(path, &ids, NULL,
> > versionsort);
> >
> > @@ -2684,22 +2690,13 @@ static void sr_assembly_load_backup(GHashTable
> > *assembly_table, 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;
> > -
> > -     g_strlcpy(assembly_table_key, addr.address, sizeof(addr.address));
> > -     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);
> > +                             assembly_table->imsi, addr_dir->d_name,
> > +                             ids[len]->d_name);
> >               r = read_file(buf, sizeof(buf), SMS_SR_BACKUP_PATH
> > "/%s/%s", -                             imsi, addr_dir->d_name,
> > ids[len]->d_name); +                             assembly_table->imsi,
> > addr_dir->d_name, +                             ids[len]->d_name);
> >
> >               if (r < 0) {
> >                       g_free(path);
> > @@ -2714,29 +2711,21 @@ static void sr_assembly_load_backup(GHashTable
> > *assembly_table, g_free(ids[len]);
> >                       continue;
> >               }
> > -             /* Gather the data for id_table node */
> > -             node = g_new0(struct id_table_node, 1);
> > -             memcpy(&node->to, &addr, sizeof(addr));
> > -             node->expiration = segment_stat.st_mtime;
> > -             memcpy(node->mrs, buf, sizeof(node->mrs));
> > -             memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> > -                                             sizeof(node->total_mrs));
> > -             memcpy(&node->sent_mrs,
> > -                     buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> > -                     sizeof(node->sent_mrs));
> > -
> > -             memcpy(&node->deliverable, buf + sizeof(node->mrs) +
> > -                     sizeof(node->total_mrs) + sizeof(node->sent_mrs),
> > -                     sizeof(node->deliverable));
> > -             /* 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);
> > +             sr_assembly_add_fragment_backup(assembly_table,
> > +                                             atoi(ids[len]->d_name),
> > +                                             segment_stat.st_mtime,
> > +                                             &addr,
> > +                                             (unsigned int *) &buf[0],
> > +                                             buf[sizeof(node->mrs)],
> > +                                             buf[sizeof(node->mrs) +
> > +                                             sizeof(node->total_mrs)],
> > +                                             FALSE);
> 
> If we can write / read the node structure directly, then I'm actually
> thinking we won't be needing this patch at all.

OK. We'll get nicer code-solution with packed node-structure, and get rid of 
memcopies etc.  So, could it be that I'll just send you updated [PATCH 2/4], 
and that's only needed patch with status report backup-files?

> 
> >               g_free(path);
> >               g_free(ids[len]);
> >       }
> > +
> >       g_free(ids);
> >  }
> >
> > @@ -2767,8 +2756,7 @@ struct status_report_assembly
> > *status_report_assembly_new(const char *imsi) * 1-n msg_ids.
> >                */
> >               while (len--) {
> > -                     sr_assembly_load_backup(ret->assembly_table, imsi,
> > -                                                            
> > addresses[len]); +                     sr_assembly_load_backup(ret,
> > addresses[len]); g_free(addresses[len]);
> >               }
> >
> > @@ -2778,16 +2766,17 @@ struct status_report_assembly
> > *status_report_assembly_new(const char *imsi) return ret;
> >  }
> >
> > -static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> > -                                     const struct id_table_node *node,
> > -                                     unsigned int msg_id)
> > +static gboolean sr_assembly_backup_store(
> > +                             struct status_report_assembly *assembly,
> > +                             const struct id_table_node *node,
> > +                             unsigned int msg_id)
> >  {
> >       int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
> >                       sizeof(node->sent_mrs) + sizeof(node->deliverable);
> >       unsigned char buf[len];
> >       DECLARE_SMS_ADDR_STR(straddr);
> >
> > -     if (!imsi)
> > +     if (!assembly->imsi)
> >               return FALSE;
> >
> >       if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
> > @@ -2805,32 +2794,35 @@ static gboolean
> > sr_assembly_add_fragment_backup(const char *imsi, sizeof(node->sent_mrs),
> > &node->deliverable, sizeof(node->deliverable));
> >
> >       /* storagedir/%s/sms_sr/%s/%i */
> > -     if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE,
> > imsi, +     if (write_file(buf, len, SMS_BACKUP_MODE,
> > +                     SMS_SR_BACKUP_PATH_FILE, assembly->imsi,
> >                       straddr, msg_id) != len)
> >               return FALSE;
> >
> >       return TRUE;
> >  }
> >
> > -static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> > +static gboolean sr_assembly_backup_free(struct status_report_assembly
> > *assembly, const struct id_table_node *node, unsigned int msg_id)
> >  {
> >       char *path;
> >       DECLARE_SMS_ADDR_STR(straddr);
> >
> > -     if (!imsi)
> > +     if (!assembly->imsi)
> >               return FALSE;
> >
> >       if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
> >               return FALSE;
> >
> > -     path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr,
> > msg_id); +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE,
> > assembly->imsi, +                             straddr, msg_id);
> >
> >       unlink(path);
> >       g_free(path);
> >
> > -     path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, assembly->imsi,
> > +                             straddr);
> >
> >       /* If the address does not have relating msg_ids anymore, remove it
> > */ rmdir(path);
> > @@ -2928,8 +2920,8 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, /* More status reports expected, and
> > already received reports completed. Update backup file.
> >                */
> > -             sr_assembly_add_fragment_backup(assembly->imsi, node,
> > -                                             *((unsigned int *) key));
> > +             sr_assembly_backup_store(assembly, node,
> > +                                      *((unsigned int *) key));
> >
> >               return FALSE;
> >       }
> > @@ -2940,8 +2932,7 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, if (out_id)
> >               *out_id = *((unsigned int *) key);
> >
> > -     sr_assembly_remove_fragment_backup(assembly->imsi, node,
> > -                                             *((unsigned int *) key));
> > +     sr_assembly_backup_free(assembly, node, *((unsigned int *) key));
> >
> >       g_hash_table_iter_remove(&iter);
> >
> > @@ -2952,18 +2943,18 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, return TRUE;
> >  }
> >
> > -void status_report_assembly_add_fragment(
> > -                                     struct status_report_assembly
> > *assembly, -                                     unsigned int msg_id,
> > -                                     const struct sms_address *to,
> > -                                     unsigned char mr, time_t
> > expiration, -                                     unsigned char
> > total_mrs)
> > +static void sr_assembly_add_fragment_backup(
> > +                             struct status_report_assembly *assembly,
> > +                             unsigned int msg_id, time_t ts,
> > +                             const struct sms_address *to,
> > +                             unsigned int *mrs, unsigned char total_mrs,
> > +                             unsigned char sent_mrs, gboolean backup)
> >  {
> > -     unsigned int offset = mr / 32;
> > -     unsigned int bit = 1 << (mr % 32);
> >       GHashTable *id_table;
> >       struct id_table_node *node;
> >       unsigned int *id_table_key;
> > +     int i;
> > +     int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
> >
> >       id_table = g_hash_table_lookup(assembly->assembly_table,
> >                                       sms_address_to_string(to));
> > @@ -2971,7 +2962,7 @@ void status_report_assembly_add_fragment(
> >       /* Create hashtable keyed by the to address if required */
> >       if (id_table == NULL) {
> >               id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> > -                                                             g_free,
> > g_free); +                                             g_free, g_free);
> >               g_hash_table_insert(assembly->assembly_table,
> >                                      
> > g_strdup(sms_address_to_string(to)), id_table);
> > @@ -2993,10 +2984,45 @@ void status_report_assembly_add_fragment(
> >       }
> >
> >       /* id_table and node both exists */
> > -     node->mrs[offset] |= bit;
> > -     node->expiration = expiration;
> > -     node->sent_mrs++;
> > -     sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
> > +
> > +     for (i = 0; i < len; i++)
> > +             node->mrs[i] |= mrs[i];
> > +
> > +     node->expiration = ts;
> > +
> > +     /* storing to the backup-file is needed, when new SMS-message is
> > sent */ +     if (backup) {
> > +             node->sent_mrs++;
> > +             sr_assembly_backup_store(assembly, node, msg_id);
> > +     }
> > +     /* storing to the backup-file is not needed, when backup is loaded
> > */ +     else
> > +             node->sent_mrs = sent_mrs;
> > +}
> > +
> > +void status_report_assembly_add_fragment(
> > +                             struct status_report_assembly *assembly,
> > +                             unsigned int msg_id,
> > +                             const struct sms_address *to,
> > +                             unsigned char mr, time_t expiration,
> > +                             unsigned char total_mrs)
> > +{
> > +     struct id_table_node *node;
> > +     unsigned int offset = mr / 32;
> > +     unsigned int bit = 1 << (mr % 32);
> > +     int len = sizeof(node->mrs) / sizeof(node->mrs[0]);
> > +     unsigned int mrs[len];
> > +
> > +     memset(mrs, 0, len);
> > +
> > +     /* set correct bit corresponding to the
> > +      *  received mr-number in mr-buffer
> > +      */
> > +     mrs[offset] |= bit;
> > +
> 
> The memset above and this looks fishy to me...
> 
> > +     sr_assembly_add_fragment_backup(assembly, msg_id, expiration,
> > +                                     to, mrs, total_mrs,
> > +                                     FALSE, TRUE);
> >  }
> >
> >  void status_report_assembly_expire(struct status_report_assembly
> > *assembly,
> 
> Regards,
> -Denis
> 

Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report

[Denis Kenzior]

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

Hi Petteri,

>> If we can write / read the node structure directly, then I'm actually
>> thinking we won't be needing this patch at all.
> 
> OK. We'll get nicer code-solution with packed node-structure, and get rid of 
> memcopies etc.  So, could it be that I'll just send you updated [PATCH 2/4], 
> and that's only needed patch with status report backup-files?
> 

Sounds like it should be sufficient.  Lets go in this direction and mop
up any further issues afterwards.

Regards,
-Denis

Re: [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function

[Denis Kenzior]

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

Hi Petteri,

On 08/16/2010 02:08 AM, Petteri Tikander wrote:
> ---
>  src/smsutil.c |   54 ++++++++++++++++++++++++++----------------------------
>  1 files changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 0972988..25e405c 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -46,7 +46,7 @@
>  #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-%i-%i"
> +#define SMS_SR_BACKUP_PATH_DIR SMS_SR_BACKUP_PATH "/%s"

Aha you're already following my comments from last patch :)  Can you
please modify the previous patch to include this change?

>  #define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH_DIR "/%i"
>  
>  #define SMS_ADDR_FMT "%24[0-9A-F]"
> @@ -2417,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);

Same comment as above, squish into previous patch.

>  
>  	if (!assembly->imsi)
>  		return;
> @@ -2652,7 +2652,8 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  {
>  	char *path;
>  	struct dirent **ids;
> -	struct sms_address sms_addr, *addr;
> +	struct sms_address addr;
> +	DECLARE_SMS_ADDR_STR(straddr);

Here as well, squish into previous patch.

>  	struct id_table_node *node;
>  	GHashTable *id_table;
>  	int len;
> @@ -2666,13 +2667,12 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  	if (addr_dir->d_type != DT_DIR)
>  		return;
>  
> -	addr = &sms_addr;
> +	/* Max of SMS address size is 12 bytes, hex encoded */
> +	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT, straddr) < 1)
> +		return;
>  
> -	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-%i-%i",
> -			addr->address, (int *) &addr->number_type,
> -			(int *) &addr->numbering_plan) < 3) {
> +	if (sms_assembly_extract_address(straddr, &addr) == FALSE)
>  		return;
> -	}

And here

>  
>  	/* Go through different msg_ids. */
>  	path = g_strdup_printf(SMS_SR_BACKUP_PATH "/%s", imsi,
> @@ -2687,12 +2687,12 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  	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));
> +	assembly_table_key = g_try_malloc(sizeof(addr.address));
>  
>  	if (assembly_table_key == NULL)
>  		return;
>  
> -	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
> +	g_strlcpy(assembly_table_key, addr.address, sizeof(addr.address));
>  	g_hash_table_insert(assembly_table, assembly_table_key, id_table);

g_strdup(sms_address_to_string(&addr)) seems better.  Squish this one
into the previous patch as well.

>  
>  	while (len--) {
> @@ -2716,7 +2716,7 @@ static void sr_assembly_load_backup(GHashTable *assembly_table,
>  		}
>  		/* Gather the data for id_table node */
>  		node = g_new0(struct id_table_node, 1);
> -		memcpy(&node->to, addr, sizeof(*addr));
> +		memcpy(&node->to, &addr, sizeof(addr));
>  		node->expiration = segment_stat.st_mtime;
>  		memcpy(node->mrs, buf, sizeof(node->mrs));
>  		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> @@ -2766,7 +2766,6 @@ struct status_report_assembly *status_report_assembly_new(const char *imsi)
>  		/* Go through different addresses. Each address can relate to
>  		 * 1-n msg_ids.
>  		 */
> -

Again, please leave this newline here.

>  		while (len--) {
>  			sr_assembly_load_backup(ret->assembly_table, imsi,
>  								addresses[len]);
> @@ -2786,10 +2785,14 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
>  	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
>  			sizeof(node->sent_mrs) + sizeof(node->deliverable);
>  	unsigned char buf[len];
> +	DECLARE_SMS_ADDR_STR(straddr);
>  
>  	if (!imsi)
>  		return FALSE;
>  
> +	if (sms_address_to_hex_string(&node->to, straddr) == FALSE)
> +		return FALSE;
> +

And squish here as well..

>  	memcpy(buf, node->mrs, sizeof(node->mrs));
>  
>  	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
> @@ -2801,10 +2804,9 @@ static gboolean sr_assembly_add_fragment_backup(const char *imsi,
>  	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
>  	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
>  
> -	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
> +	/* storagedir/%s/sms_sr/%s/%i */
>  	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
> -			node->to.address, node->to.number_type,
> -			node->to.numbering_plan, msg_id) != len)
> +			straddr, msg_id) != len)

And here

>  		return FALSE;
>  
>  	return TRUE;
> @@ -2815,19 +2817,20 @@ static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
>  					unsigned int msg_id)
>  {
>  	char *path;
> +	DECLARE_SMS_ADDR_STR(straddr);
>  
>  	if (!imsi)
>  		return FALSE;
>  
> -	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
> -				node->to.number_type, node->to.numbering_plan,
> -				msg_id);
> +	if (sms_address_to_hex_string(&node->to, 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, node->to.address,
> -				node->to.number_type, node->to.numbering_plan);
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, straddr);

And squish this chunk into previous as well.

>  
>  	/* If the address does not have relating msg_ids anymore, remove it */
>  	rmdir(path);
> @@ -2836,14 +2839,6 @@ static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
>  	return TRUE;
>  }
>  
> -static gboolean sr_assembly_update_fragment_backup(const char *imsi,
> -					const struct id_table_node *node,
> -					unsigned int msg_id)
> -{
> -	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
> -			sr_assembly_add_fragment_backup(imsi, node, msg_id);
> -}
> -

Simply removing this function from the previous patch is better.

>  void status_report_assembly_free(struct status_report_assembly *assembly)
>  {
>  	g_hash_table_destroy(assembly->assembly_table);
> @@ -2930,6 +2925,9 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	}
>  
>  	if (pending == TRUE && node->deliverable == TRUE) {
> +		/* More status reports expected, and already received
> +		   reports completed. Update backup file.
> +		 */

Please update this comment based on the coding style guidelines.  For
multi line comments we prefer

/*
 * Comment line 1...
 * Comment line 2...
 */

>  		sr_assembly_add_fragment_backup(assembly->imsi, node,
>  						*((unsigned int *) key));
>  

Regards,
-Denis

Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function

[Denis Kenzior]

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

Hi Petteri,

On 08/16/2010 02:08 AM, Petteri Tikander wrote:
> ---
>  src/smsutil.c |  202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 198 insertions(+), 4 deletions(-)
> 

Can we change the commit message a bit, something about storing /
loading sms status report assemblies over reboots ;)

> diff --git a/src/smsutil.c b/src/smsutil.c
> index 22c70cf..0972988 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-%i-%i"
> +#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,
> @@ -2642,20 +2646,204 @@ 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 sms_addr, *addr;
> +	struct id_table_node *node;
> +	GHashTable *id_table;
> +	int len;
> +	int r;
> +	unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
> +			sizeof(node->sent_mrs) + sizeof(node->deliverable)];
> +	char *assembly_table_key;
> +	unsigned int *id_table_key;
> +	struct stat segment_stat;
> +
> +	if (addr_dir->d_type != DT_DIR)
> +		return;
> +
> +	addr = &sms_addr;
> +
> +	if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-%i-%i",
> +			addr->address, (int *) &addr->number_type,
> +			(int *) &addr->numbering_plan) < 3) {
> +		return;
> +	}

Lets follow what the sms_assembly does here.  Namely SMS_ADDR_FMT is
actually geared towards a hex-encoded address.  This already contains
the address, number_type and numbering_plan elements.  So simply using
sms_assembly_extract_address() afterwards is actually fine.  This has
the advantage of getting rid of the unnecessary casts.

> +
> +	/* 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;
> +
> +	g_strlcpy(assembly_table_key, addr->address, sizeof(addr->address));
> +	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);
> +		r = read_file(buf, sizeof(buf), 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]);
> +			continue;
> +		}
> +
> +		r = stat(path, &segment_stat);
> +
> +		if (r != 0) {
> +			g_free(path);
> +			g_free(ids[len]);
> +			continue;
> +		}
> +		/* Gather the data for id_table node */
> +		node = g_new0(struct id_table_node, 1);
> +		memcpy(&node->to, addr, sizeof(*addr));

So I got rid of the node->to for now.  There's no reason to have it that
I can see...  Any time you are building / loading / accessing a node,
you actually know the current to address anyway.

> +		node->expiration = segment_stat.st_mtime;
> +		memcpy(node->mrs, buf, sizeof(node->mrs));
> +		memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> +						sizeof(node->total_mrs));
> +		memcpy(&node->sent_mrs,
> +			buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> +			sizeof(node->sent_mrs));
> +
> +		memcpy(&node->deliverable, buf + sizeof(node->mrs) +
> +			sizeof(node->total_mrs) + sizeof(node->sent_mrs),
> +			sizeof(node->deliverable));

To make our lives easier, I'd make the id_table_node structure packed
and read the data directly into it.  We don't have to worry about
byte-ordering at this point.

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

This looks just fine to me.

>  	return ret;
>  }
>  
> +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)
> +{
> +	int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
> +			sizeof(node->sent_mrs) + sizeof(node->deliverable);
> +	unsigned char buf[len];
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	memcpy(buf, node->mrs, sizeof(node->mrs));
> +
> +	memcpy(buf + sizeof(node->mrs), &node->total_mrs,
> +					sizeof(node->total_mrs));
> +
> +	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> +				&node->sent_mrs, sizeof(node->sent_mrs));
> +
> +	memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
> +	sizeof(node->sent_mrs),	&node->deliverable, sizeof(node->deliverable));
> +

Again, simply writing the id_table_node structure might be easier here,
especially since you're not taking care of loading the expiration member.

> +	/* storagedir/%s/sms_sr/%s-%i-%i/%i */
> +	if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE, imsi,
> +			node->to.address, node->to.number_type,
> +			node->to.numbering_plan, msg_id) != len)
> +		return FALSE;
> +
> +	return TRUE;
> +}
> +
> +static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)
> +{
> +	char *path;
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, node->to.address,
> +				node->to.number_type, node->to.numbering_plan,
> +				msg_id);
> +
> +	unlink(path);
> +	g_free(path);
> +
> +	path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi, node->to.address,
> +				node->to.number_type, node->to.numbering_plan);
> +
> +	/* If the address does not have relating msg_ids anymore, remove it */
> +	rmdir(path);
> +	g_free(path);
> +
> +	return TRUE;
> +}
> +
> +static gboolean sr_assembly_update_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)
> +{
> +	return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
> +			sr_assembly_add_fragment_backup(imsi, node, msg_id);
> +}
> +
>  void status_report_assembly_free(struct status_report_assembly *assembly)
>  {
>  	g_hash_table_destroy(assembly->assembly_table);
> @@ -2714,7 +2902,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;
> -

Leave this empty line here, remember to check doc/coding-style.txt for a
reason why.

>  		if (node->mrs[offset] & bit)
>  			break;
>  
> @@ -2724,7 +2911,6 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	/* Unable to find a message reference belonging to this address */
>  	if (node == NULL)
>  		return FALSE;
> -

Same comment as above.

>  	/* Mr belongs to this node. */
>  	node->mrs[offset] ^= bit;
>  
> @@ -2743,8 +2929,12 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		}
>  	}
>  
> -	if (pending == TRUE && node->deliverable == TRUE)
> +	if (pending == TRUE && node->deliverable == TRUE) {
> +		sr_assembly_add_fragment_backup(assembly->imsi, node,
> +						*((unsigned int *) key));
> +
>  		return FALSE;
> +	}
>  
>  	if (out_delivered)
>  		*out_delivered = node->deliverable;
> @@ -2752,6 +2942,9 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	if (out_id)
>  		*out_id = *((unsigned int *) key);
>  
> +	sr_assembly_remove_fragment_backup(assembly->imsi, node,
> +						*((unsigned int *) key));
> +

Honestly I'd like to see the cast done once and then used everywhere
afterwards.  Feel free to add an extra variable for this.

>  	g_hash_table_iter_remove(&iter);
>  
>  	if (g_hash_table_size(id_table) == 0)
> @@ -2805,6 +2998,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, msg_id);
>  }
>  
>  void status_report_assembly_expire(struct status_report_assembly *assembly,

Regards,
-Denis

Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function

[Petteri Tikander]

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

Hi, Denis
and thanks for the comments.

> Hi Petteri,
> 
> On 08/16/2010 02:08 AM, Petteri Tikander wrote:
> > ---
> >  src/smsutil.c |  202
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed,
> > 198 insertions(+), 4 deletions(-)
> 
> Can we change the commit message a bit, something about storing /
> loading sms status report assemblies over reboots ;)
> 

OK. Will change. 

> > diff --git a/src/smsutil.c b/src/smsutil.c
> > index 22c70cf..0972988 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-%i-%i"
> > +#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, @@ -2642,20 +2646,204 @@ 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 sms_addr, *addr;
> > +     struct id_table_node *node;
> > +     GHashTable *id_table;
> > +     int len;
> > +     int r;
> > +     unsigned char buf[sizeof(node->mrs) + sizeof(node->total_mrs) +
> > +                     sizeof(node->sent_mrs) +
> > sizeof(node->deliverable)]; +     char *assembly_table_key;
> > +     unsigned int *id_table_key;
> > +     struct stat segment_stat;
> > +
> > +     if (addr_dir->d_type != DT_DIR)
> > +             return;
> > +
> > +     addr = &sms_addr;
> > +
> > +     if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-%i-%i",
> > +                     addr->address, (int *) &addr->number_type,
> > +                     (int *) &addr->numbering_plan) < 3) {
> > +             return;
> > +     }
> 
> Lets follow what the sms_assembly does here.  Namely SMS_ADDR_FMT is
> actually geared towards a hex-encoded address.  This already contains
> the address, number_type and numbering_plan elements.  So simply using
> sms_assembly_extract_address() afterwards is actually fine.  This has
> the advantage of getting rid of the unnecessary casts.
> 

Done in the PATCH 3/4, but I'll import PATCH 3/4-changes into this patch, as 
you mentioned in the PATCH 3/4-comments.

> > +
> > +     /* 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;
> > +
> > +     g_strlcpy(assembly_table_key, addr->address,
> > sizeof(addr->address)); +     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);
> > +             r = read_file(buf, sizeof(buf), 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]);
> > +                     continue;
> > +             }
> > +
> > +             r = stat(path, &segment_stat);
> > +
> > +             if (r != 0) {
> > +                     g_free(path);
> > +                     g_free(ids[len]);
> > +                     continue;
> > +             }
> > +             /* Gather the data for id_table node */
> > +             node = g_new0(struct id_table_node, 1);
> > +             memcpy(&node->to, addr, sizeof(*addr));
> 
> So I got rid of the node->to for now.  There's no reason to have it that
> I can see...  Any time you are building / loading / accessing a node,
> you actually know the current to address anyway.
> 

Well, I cannot see the need to include address in node-variable either. It was 
some kind of backup to keep address in save, but not actually needed anywhere. 
So I'll remove it from the node.

> > +             node->expiration = segment_stat.st_mtime;
> > +             memcpy(node->mrs, buf, sizeof(node->mrs));
> > +             memcpy(&node->total_mrs, buf + sizeof(node->mrs),
> > +                                             sizeof(node->total_mrs));
> > +             memcpy(&node->sent_mrs,
> > +                     buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> > +                     sizeof(node->sent_mrs));
> > +
> > +             memcpy(&node->deliverable, buf + sizeof(node->mrs) +
> > +                     sizeof(node->total_mrs) + sizeof(node->sent_mrs),
> > +                     sizeof(node->deliverable));
> 
> To make our lives easier, I'd make the id_table_node structure packed
> and read the data directly into it.  We don't have to worry about
> byte-ordering at this point.
> 

Yep. So, we'll get rid of memcopies, and temp-buffer allocation. So I'll change 
to this:

struct id_table_node {
	struct sms_address to; // => this will be removed
	unsigned int mrs[8];
	time_t expiration;
	unsigned char total_mrs;
	unsigned char sent_mrs;
	gboolean deliverable;
} __attribute__((packed));

> > +             /* 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);
> > +     }
> > +
> 
> This looks just fine to me.
>

:) 

> >       return ret;
> >  }
> >
> > +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> > +                                     const struct id_table_node *node,
> > +                                     unsigned int msg_id)
> > +{
> > +     int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
> > +                     sizeof(node->sent_mrs) + sizeof(node->deliverable);
> > +     unsigned char buf[len];
> > +
> > +     if (!imsi)
> > +             return FALSE;
> > +
> > +     memcpy(buf, node->mrs, sizeof(node->mrs));
> > +
> > +     memcpy(buf + sizeof(node->mrs), &node->total_mrs,
> > +                                     sizeof(node->total_mrs));
> > +
> > +     memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
> > +                             &node->sent_mrs, sizeof(node->sent_mrs));
> > +
> > +     memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
> > +     sizeof(node->sent_mrs), &node->deliverable,
> > sizeof(node->deliverable)); +
> 
> Again, simply writing the id_table_node structure might be easier here,
> especially since you're not taking care of loading the expiration member.
> 

Good point:) I forgot this expiration-time member :(
With packed-node type shouldn't be missed anymore. 

I have to bother you little bit more with status report-expiration logic soon, 
because it's not implemented yet..

> > +     /* storagedir/%s/sms_sr/%s-%i-%i/%i */
> > +     if (write_file(buf, len, SMS_BACKUP_MODE, SMS_SR_BACKUP_PATH_FILE,
> > imsi, +                     node->to.address, node->to.number_type,
> > +                     node->to.numbering_plan, msg_id) != len)
> > +             return FALSE;
> > +
> > +     return TRUE;
> > +}
> > +
> > +static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
> > +                                     const struct id_table_node *node,
> > +                                     unsigned int msg_id)
> > +{
> > +     char *path;
> > +
> > +     if (!imsi)
> > +             return FALSE;
> > +
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi,
> > node->to.address, +                             node->to.number_type,
> > node->to.numbering_plan, +                             msg_id);
> > +
> > +     unlink(path);
> > +     g_free(path);
> > +
> > +     path = g_strdup_printf(SMS_SR_BACKUP_PATH_DIR, imsi,
> > node->to.address, +                             node->to.number_type,
> > node->to.numbering_plan); +
> > +     /* If the address does not have relating msg_ids anymore, remove it
> > */ +     rmdir(path);
> > +     g_free(path);
> > +
> > +     return TRUE;
> > +}
> > +
> > +static gboolean sr_assembly_update_fragment_backup(const char *imsi,
> > +                                     const struct id_table_node *node,
> > +                                     unsigned int msg_id)
> > +{
> > +     return sr_assembly_remove_fragment_backup(imsi, node, msg_id) &&
> > +                     sr_assembly_add_fragment_backup(imsi, node,
> > msg_id); +}
> > +
> >  void status_report_assembly_free(struct status_report_assembly
> > *assembly) {
> >       g_hash_table_destroy(assembly->assembly_table);
> > @@ -2714,7 +2902,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;
> > -
> 
> Leave this empty line here, remember to check doc/coding-style.txt for a
> reason why.
> 

OK.

> >               if (node->mrs[offset] & bit)
> >                       break;
> >
> > @@ -2724,7 +2911,6 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, /* Unable to find a message reference
> > belonging to this address */ if (node == NULL)
> >               return FALSE;
> > -
> 
> Same comment as above.
> 

OK.

> >       /* Mr belongs to this node. */
> >       node->mrs[offset] ^= bit;
> >
> > @@ -2743,8 +2929,12 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, }
> >       }
> >
> > -     if (pending == TRUE && node->deliverable == TRUE)
> > +     if (pending == TRUE && node->deliverable == TRUE) {
> > +             sr_assembly_add_fragment_backup(assembly->imsi, node,
> > +                                             *((unsigned int *) key));
> > +
> >               return FALSE;
> > +     }
> >
> >       if (out_delivered)
> >               *out_delivered = node->deliverable;
> > @@ -2752,6 +2942,9 @@ gboolean status_report_assembly_report(struct
> > status_report_assembly *assembly, if (out_id)
> >               *out_id = *((unsigned int *) key);
> >
> > +     sr_assembly_remove_fragment_backup(assembly->imsi, node,
> > +                                             *((unsigned int *) key));
> > +
> 
> Honestly I'd like to see the cast done once and then used everywhere
> afterwards.  Feel free to add an extra variable for this.
> 

OK. Will change.

> >       g_hash_table_iter_remove(&iter);
> >
> >       if (g_hash_table_size(id_table) == 0)
> > @@ -2805,6 +2998,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, msg_id);
> >  }
> >
> >  void status_report_assembly_expire(struct status_report_assembly
> > *assembly,
> 
> Regards,
> -Denis
> 

Best regards,
Petteri

Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function

[Denis Kenzior]

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

Hi Petteri,

<snip>

>>> +             node->expiration = segment_stat.st_mtime;
>>> +             memcpy(node->mrs, buf, sizeof(node->mrs));
>>> +             memcpy(&node->total_mrs, buf + sizeof(node->mrs),
>>> +                                             sizeof(node->total_mrs));
>>> +             memcpy(&node->sent_mrs,
>>> +                     buf + sizeof(node->mrs) + sizeof(node->total_mrs),
>>> +                     sizeof(node->sent_mrs));
>>> +
>>> +             memcpy(&node->deliverable, buf + sizeof(node->mrs) +
>>> +                     sizeof(node->total_mrs) + sizeof(node->sent_mrs),
>>> +                     sizeof(node->deliverable));
>>
>> To make our lives easier, I'd make the id_table_node structure packed
>> and read the data directly into it.  We don't have to worry about
>> byte-ordering at this point.
>>
> 
> Yep. So, we'll get rid of memcopies, and temp-buffer allocation. So I'll change 
> to this:
> 
> struct id_table_node {
> 	struct sms_address to; // => this will be removed
> 	unsigned int mrs[8];
> 	time_t expiration;
> 	unsigned char total_mrs;
> 	unsigned char sent_mrs;
> 	gboolean deliverable;
> } __attribute__((packed));
> 

Yes, this seems just fine to me.  I already removed the to member by the
way. :)

<snip>

>>> +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
>>> +                                     const struct id_table_node *node,
>>> +                                     unsigned int msg_id)
>>> +{
>>> +     int len = sizeof(node->mrs) + sizeof(node->total_mrs) +
>>> +                     sizeof(node->sent_mrs) + sizeof(node->deliverable);
>>> +     unsigned char buf[len];
>>> +
>>> +     if (!imsi)
>>> +             return FALSE;
>>> +
>>> +     memcpy(buf, node->mrs, sizeof(node->mrs));
>>> +
>>> +     memcpy(buf + sizeof(node->mrs), &node->total_mrs,
>>> +                                     sizeof(node->total_mrs));
>>> +
>>> +     memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs),
>>> +                             &node->sent_mrs, sizeof(node->sent_mrs));
>>> +
>>> +     memcpy(buf + sizeof(node->mrs) + sizeof(node->total_mrs) +
>>> +     sizeof(node->sent_mrs), &node->deliverable,
>>> sizeof(node->deliverable)); +
>>
>> Again, simply writing the id_table_node structure might be easier here,
>> especially since you're not taking care of loading the expiration member.
>>
> 
> Good point:) I forgot this expiration-time member :(
> With packed-node type shouldn't be missed anymore. 
> 
> I have to bother you little bit more with status report-expiration logic soon, 
> because it's not implemented yet..

Sure :)

We might also need to revisit the address / mr matching.  Some networks
are stupid and rewrite the SMS address in the status report.  E.g. when
I send an SMS to 555-123-4567, the status report comes back with the
address rewritten to +15551234567.

So we might need to allow some fuzzy matching based on the last n digits
or something more clever.

Regards,
-Denis