* [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 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 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 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
* 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 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 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 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
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.