From: Denis Kenzior <denkenz@gmail.com>
To: ofono@ofono.org
Subject: Re: [RFC_PATCH 4/4] smsutil: added function for data handling of status report
Date: Tue, 17 Aug 2010 14:07:03 -0500 [thread overview]
Message-ID: <4C6ADDD7.5010300@gmail.com> (raw)
In-Reply-To: <1281942533-9126-4-git-send-email-petteri.tikander@ixonos.com>
[-- 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
next prev parent reply other threads:[~2010-08-17 19:07 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C6ADDD7.5010300@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@ofono.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox