5:08 p.m.
---
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
5:08 p.m.
New subject: [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function
---
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
5:08 p.m.
New subject: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
---
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
5:07 a.m.
New subject: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
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
3:53 a.m.
New subject: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
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
4:11 a.m.
New subject: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
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
4:16 a.m.
New subject: [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function
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
4:10 a.m.
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
3:40 a.m.
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
4:09 a.m.
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
4414
days inactive
4416
days old
9 comments
2 participants
participants (2)
-
Denis Kenzior -
Petteri Tikander