Re: [RFC PATCH 5/5] smsutil: save pending status reports to imsi file

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10143 bytes --]

Hi Pasi,

> ---
>  src/smsutil.c |  205
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files 
changed,
>  204 insertions(+), 1 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 3db3d28..a30a281 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, @@ -2625,20 +2629,209 @@ 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 *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 = g_new0(struct sms_address, 1);

Why are you mallocing addr here? Seems that this only makes the code more 
complicated.

> +
> +	if (sscanf(addr_dir->d_name, "%[0-9]-%i-%i",
> +			addr->address, (int *) &addr->number_type,
> +			(int *) &addr->numbering_plan) < 3) {

Lets make this consistent with sms assembly, please use SMS_ADDR_FMT.  You're 
also free to use sms_assembly utility functions if that helps you.

> +		g_free(addr);
> +		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) {
> +		g_free(addr);
> +		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) {
> +		g_free(addr);
> +		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);
> +		node->to = *addr;

Please use memcpy for copying structures.

> +		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(addr);
> +	g_free(ids);
> +}
> +

General comment here is that you're duplicating much of the code that inserts 
the information into the data structure.  You can play the same trick as 
sms_assembly which uses a single function to do the heavy lifting, but takes 
an argument whether to store this info on disk.  Obviously during load you 
don't want to re-save this information on disk.  Then your load function 
becomes much smaller and easier to understand.

>  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;

There should be an empty line here

> +		/* Go through different addresses. Each address can relate to
> +		 * 1-n msg_ids. Do not try to load . and .. directories.
> +		 */
> +		g_free(addresses[0]);
> +		g_free(addresses[1]);

This really isn't necessary, the '.' and '..' directories will not pass the 
sscanf test above.

> +
> +		while (2 < len--) {
> +			sr_assembly_load_backup(ret->assembly_table, imsi,
> +								addresses[len]);
> +			g_free(addresses[len]);
> +		}

Empty line here please

> +		g_free(addresses);
> +	}

And another empty line here.  Please put an empty line after each block unless 
it is at the end of the function.
>  	return ret;
>  }
> 
> +static gboolean sr_assembly_add_fragment_backup(const char *imsi,
> +					const struct id_table_node *node,
> +					unsigned int msg_id)

Lets be consistent with how sms assembly works.  E.g. take the main assembly 
object and return void.  Just helps to read the code when it is consistent.

> +{
> +	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)

Same consistency comment here.  Take main assembly object and return void.

> +{
> +	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);
> +}
> +

Is this function really necessary? write_file overwrites the file, so simply 
using add_fragment_backup seems sufficient.

>  void status_report_assembly_free(struct status_report_assembly *assembly)
>  {
>  	g_hash_table_destroy(assembly->assembly_table);
> @@ -2707,10 +2900,14 @@ gboolean status_report_assembly_report(struct
>  status_report_assembly *assembly, * not delete the node yet.
>  		 */
>  		if (pending) {
> +			sr_assembly_update_fragment_backup(assembly->imsi, node,
> +								*msg_id);

It seems to me fragment is the wrong name here.  What about 
'store_node_backup'?

>  			*msg_delivered = FALSE;
>  			return update_history;
>  		} else {
>  			*msg_delivered = node->deliverable;
> +			sr_assembly_remove_fragment_backup(assembly->imsi, node,
> +								*msg_id);

maybe 'remove_node_backup'

> 
>  			g_hash_table_iter_remove(&iter);
> 
> @@ -2747,6 +2944,7 @@ void status_report_assembly_add_fragment(
>  	unsigned int *id_table_key;
> 
>  	id_table = g_hash_table_lookup(assembly->assembly_table, to->address);
> +
>  	/* Create id_table and node */
>  	if (id_table == NULL) {
>  		id_table = g_hash_table_new_full(g_int_hash, g_int_equal,
> @@ -2775,6 +2973,9 @@ void status_report_assembly_add_fragment(
> 
>  		g_hash_table_insert(assembly->assembly_table,
>  					assembly_table_key, id_table);
> +
> +		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
> +
>  		return;
>  	}
> 
> @@ -2793,12 +2994,14 @@ void status_report_assembly_add_fragment(
>  		*id_table_key = msg_id;
>  		g_hash_table_insert(id_table, id_table_key, node);
> 
> +		sr_assembly_add_fragment_backup(assembly->imsi, node, msg_id);
>  		return;
>  	}
>  	/* id_table and node both exists */
>  	node->mrs[offset] |= bit;
>  	node->expiration = expiration;
>  	node->sent_mrs++;
> +	sr_assembly_update_fragment_backup(assembly->imsi, node, msg_id);
>  }
> 
>  void status_report_assembly_expire(struct status_report_assembly
>  *assembly,
> 

Regards,
-Denis