Hi Petteri,
On 08/27/2010 01:09 PM, Petteri Tikander wrote:
---
src/smsutil.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
src/smsutil.h | 10 +++-
2 files changed, 180 insertions(+), 6 deletions(-)
diff --git a/src/smsutil.c b/src/smsutil.c
index c60b8ec..88c567c 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -33,6 +33,7 @@
#include <unistd.h>
#include <glib.h>
+#include <errno.h>
Remove this if strtoul is no longer used.
#include "util.h"
#include "storage.h"
@@ -45,6 +46,9 @@
#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_FILE SMS_SR_BACKUP_PATH "/%s-%i"
+
#define SMS_ADDR_FMT "%24[0-9A-F]"
static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,
@@ -2413,7 +2417,7 @@ static void sms_assembly_backup_free(struct sms_assembly
*assembly,
{
char *path;
int seq;
- char straddr[25];
+ DECLARE_SMS_ADDR_STR(straddr);
Please send this chunk as a separate patch, it doesn't belong to status
report assembly.
if (!assembly->imsi)
return;
@@ -2529,7 +2533,8 @@ static GSList *sms_assembly_add_fragment_backup(struct sms_assembly
*assembly,
if (ref != node->ref)
continue;
- /* Message Reference and address the same, but max is not
+ /*
+ * Message Reference and address the same, but max is not
* ignore the SMS completely
*/
Again, please send this as a separate cleanup patch.
if (max != node->max_fragments)
@@ -2642,20 +2647,163 @@ 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)
+{
+ struct sms_address addr;
+ DECLARE_SMS_ADDR_STR(straddr);
+ struct id_table_node *node;
+ GHashTable *id_table;
+ int r;
+ char *assembly_table_key;
+ unsigned int *id_table_key;
+ DECLARE_SMS_MSGID_STR(str_msg_id);
+ char *endp;
+ unsigned int msg_id;
+
+ if (addr_dir->d_type != DT_REG)
+ return;
+
+ /*
+ * All SMS-messages under the same IMSI-code are
+ * included in the same directory.
+ * So, SMS-address and message ID are included in the same file name
+ * Max of SMS address size is 12 bytes, hex encoded
+ */
+ if (sscanf(addr_dir->d_name, SMS_ADDR_FMT "-" SMS_MSGID_FMT,
+ straddr, str_msg_id) < 2)
+ return;
Using "-%u" should be sufficient here instead of SMS_MSGID_FMT. Let us
hold off being too pedantic. The directory is only readable / writeable
by root on properly configured systems, so we don't have to be too clever.
Also, we will be migrating away from unsigned int message ids to SHA-160
strings in the future, so lets not invest too much time getting this
part right.
+
+ if (sms_assembly_extract_address(straddr, &addr) == FALSE)
+ return;
+
This looks fine...
+ errno = 0;
+ msg_id = strtoul(str_msg_id, &endp, 10);
+
+ if (*endp != '\0')
+ return;
+
+ if (errno == ERANGE && msg_id == UINT_MAX)
+ return;
+
So this part is no longer needed, remember to remove the errno.h include
above.
+ id_table = g_hash_table_lookup(assembly_table,
+ sms_address_to_string(&addr));
+
+ /* 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);
+
+ assembly_table_key = g_strdup(sms_address_to_string(&addr));
+ g_hash_table_insert(assembly_table, assembly_table_key,
+ id_table);
+ }
+
+ node = g_new0(struct id_table_node, 1);
+
+ r = read_file((unsigned char *) node,
+ sizeof(struct id_table_node),
+ SMS_SR_BACKUP_PATH "/%s",
+ imsi, addr_dir->d_name);
+
+ if (r < 0) {
+ g_free(node);
+ return;
+ }
So I suggest turning the order around here slightly. If (id_table ==
NULL) and we fail to read the file, the assembly_table now has an
address hash table with no elements.
If we try to read the node first and fail, we won't have this issue.
+
+ /* Node ready, create key and add them to the table */
+ id_table_key = g_new0(unsigned int, 1);
+ *id_table_key = msg_id;
+
+ g_hash_table_insert(id_table, id_table_key, node);
+}
+
struct status_report_assembly *status_report_assembly_new(const char *imsi)
{
+ char *path;
+ int len;
+ struct dirent **addresses;
struct status_report_assembly *ret =
g_new0(struct status_report_assembly, 1);
ret->assembly_table = g_hash_table_new_full(g_str_hash, g_str_equal,
g_free, (GDestroyNotify)g_hash_table_destroy);
- if (imsi)
+ if (imsi) {
ret->imsi = imsi;
+ /* Restore state from backup */
+ path = g_strdup_printf(SMS_SR_BACKUP_PATH, imsi);
+ len = scandir(path, &addresses, NULL, alphasort);
+
+ g_free(path);
+
+ if (len < 0)
+ return ret;
+
+ /*
+ * Go through different addresses. Each address can relate to
+ * 1-n msg_ids.
+ */
+
+ while (len--) {
+ sr_assembly_load_backup(ret->assembly_table, imsi,
+ addresses[len]);
+ g_free(addresses[len]);
+ }
+
+ g_free(addresses);
+ }
+
return ret;
}
+static gboolean sr_assembly_add_fragment_backup(const char *imsi,
+ const struct id_table_node *node,
+ const struct sms_address *addr,
+ unsigned int msg_id)
+{
+ int len = sizeof(struct id_table_node);
+ DECLARE_SMS_ADDR_STR(straddr);
+
+ if (!imsi)
+ return FALSE;
+
+ if (sms_address_to_hex_string(addr, straddr) == FALSE)
+ return FALSE;
+
+ /* storagedir/%s/sms_sr/%s-%i */
+ if (write_file((unsigned char *) node, len, SMS_BACKUP_MODE,
+ SMS_SR_BACKUP_PATH_FILE, imsi,
+ straddr, msg_id) != len)
+ return FALSE;
+
+ return TRUE;
+}
+
+static gboolean sr_assembly_remove_fragment_backup(const char *imsi,
+ const struct id_table_node *node,
+ const struct sms_address *addr,
+ unsigned int msg_id)
+{
+ char *path;
+ DECLARE_SMS_ADDR_STR(straddr);
+
+ if (!imsi)
+ return FALSE;
+
+ if (sms_address_to_hex_string(addr, straddr) == FALSE)
+ return FALSE;
+
+ path = g_strdup_printf(SMS_SR_BACKUP_PATH_FILE, imsi, straddr, msg_id);
+
+ unlink(path);
+ g_free(path);
+
+ return TRUE;
+}
+
void status_report_assembly_free(struct status_report_assembly *assembly)
{
g_hash_table_destroy(assembly->assembly_table);
@@ -2698,6 +2846,7 @@ gboolean status_report_assembly_report(struct
status_report_assembly *assembly,
GHashTableIter iter;
gboolean pending;
int i;
+ unsigned int msg_id;
/* We ignore temporary or tempfinal status reports */
if (sr_st_to_delivered(status_report->status_report.st,
@@ -2743,14 +2892,30 @@ gboolean status_report_assembly_report(struct
status_report_assembly *assembly,
}
}
- if (pending == TRUE && node->deliverable == TRUE)
+ msg_id = *(unsigned int *) key;
+
+ if (pending == TRUE && node->deliverable == TRUE) {
+ /*
+ * More status reports expected, and already received
+ * reports completed. Update backup file.
+ */
+ sr_assembly_add_fragment_backup(
+ assembly->imsi, node,
+ &status_report->status_report.raddr,
+ msg_id);
+
return FALSE;
+ }
if (out_delivered)
*out_delivered = node->deliverable;
if (out_id)
- *out_id = *((unsigned int *) key);
+ *out_id = msg_id;
+
+ sr_assembly_remove_fragment_backup(assembly->imsi, node,
+ &status_report->status_report.raddr,
+ msg_id);
g_hash_table_iter_remove(&iter);
@@ -2804,6 +2969,7 @@ void status_report_assembly_add_fragment(
node->mrs[offset] |= bit;
node->expiration = expiration;
node->sent_mrs++;
+ sr_assembly_add_fragment_backup(assembly->imsi, node, to, msg_id);
}
void status_report_assembly_expire(struct status_report_assembly *assembly,
diff --git a/src/smsutil.h b/src/smsutil.h
index eb70b6d..e5ef73a 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -370,7 +370,7 @@ struct id_table_node {
unsigned char total_mrs;
unsigned char sent_mrs;
gboolean deliverable;
-};
+} __attribute__((packed));
Looks just fine to me...
struct status_report_assembly {
const char *imsi;
@@ -437,6 +437,14 @@ gboolean sms_encode(const struct sms *in, int *len, int *tpdu_len,
*/
#define DECLARE_SMS_ADDR_STR(a) char a[25]
+/*
+ * Length is based on the msg_id being 10 digits (max uint)
+ * plus a terminating NUL char. But for more detailed
+ * digit-verification, recerve space for 11 characters plus NUL char.
+ */
+#define DECLARE_SMS_MSGID_STR(a) char a[12]
+#define SMS_MSGID_FMT "%11s"
+
As discussed above, lets remove this part.
As a general rule, if the define / enum / type does not need to be
exported for use by others, then it is preferable to keep it in the .c file.
gboolean sms_decode_address_field(const unsigned char *pdu, int
len,
int *offset, gboolean sc,
struct sms_address *out);
Fix this up and lets get this stuff upstream and stress tested :)
Regards,
-Denis