* [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function
[not found] <1281942533-9126-1-git-send-email-petteri.tikander@ixonos.com>
@ 2010-08-16 7:08 ` Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Petteri Tikander
2010-08-17 18:10 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Denis Kenzior
0 siblings, 2 replies; 10+ messages in thread
From: Petteri Tikander @ 2010-08-16 7:08 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function
2010-08-16 7:08 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Petteri Tikander
@ 2010-08-16 7:08 ` Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 4/4] smsutil: added function for data handling of status report Petteri Tikander
2010-08-17 18:16 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Denis Kenzior
2010-08-17 18:10 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Denis Kenzior
1 sibling, 2 replies; 10+ messages in thread
From: Petteri Tikander @ 2010-08-16 7:08 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC_PATCH 4/4] smsutil: added function for data handling of status report
2010-08-16 7:08 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Petteri Tikander
@ 2010-08-16 7:08 ` Petteri Tikander
2010-08-17 19:07 ` Denis Kenzior
2010-08-17 18:16 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Denis Kenzior
1 sibling, 1 reply; 10+ messages in thread
From: Petteri Tikander @ 2010-08-16 7:08 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function
2010-08-16 7:08 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Petteri Tikander
@ 2010-08-17 18:10 ` Denis Kenzior
2010-08-18 17:40 ` Petteri Tikander
1 sibling, 1 reply; 10+ messages in thread
From: Denis Kenzior @ 2010-08-17 18:10 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function
2010-08-16 7:08 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 4/4] smsutil: added function for data handling of status report Petteri Tikander
@ 2010-08-17 18:16 ` Denis Kenzior
1 sibling, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2010-08-17 18:16 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
2010-08-16 7:08 ` [RFC_PATCH 4/4] smsutil: added function for data handling of status report Petteri Tikander
@ 2010-08-17 19:07 ` Denis Kenzior
2010-08-18 17:53 ` Petteri Tikander
0 siblings, 1 reply; 10+ messages in thread
From: Denis Kenzior @ 2010-08-17 19:07 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function
2010-08-17 18:10 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Denis Kenzior
@ 2010-08-18 17:40 ` Petteri Tikander
2010-08-18 18:09 ` Denis Kenzior
0 siblings, 1 reply; 10+ messages in thread
From: Petteri Tikander @ 2010-08-18 17:40 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
2010-08-17 19:07 ` Denis Kenzior
@ 2010-08-18 17:53 ` Petteri Tikander
2010-08-18 18:11 ` Denis Kenzior
0 siblings, 1 reply; 10+ messages in thread
From: Petteri Tikander @ 2010-08-18 17:53 UTC (permalink / raw)
To: ofono
[-- 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function
2010-08-18 17:40 ` Petteri Tikander
@ 2010-08-18 18:09 ` Denis Kenzior
0 siblings, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2010-08-18 18:09 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
2010-08-18 17:53 ` Petteri Tikander
@ 2010-08-18 18:11 ` Denis Kenzior
0 siblings, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2010-08-18 18:11 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-08-18 18:11 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1281942533-9126-1-git-send-email-petteri.tikander@ixonos.com>
2010-08-16 7:08 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Petteri Tikander
2010-08-16 7:08 ` [RFC_PATCH 4/4] smsutil: added function for data handling of status report Petteri Tikander
2010-08-17 19:07 ` Denis Kenzior
2010-08-18 17:53 ` Petteri Tikander
2010-08-18 18:11 ` Denis Kenzior
2010-08-17 18:16 ` [RFC_PATCH 3/4] smsutil:add proper checking of sms-address in status report-function Denis Kenzior
2010-08-17 18:10 ` [RFC_PATCH 2/4] smsutil: Editorial changes to sms status report function Denis Kenzior
2010-08-18 17:40 ` Petteri Tikander
2010-08-18 18:09 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.