Re: [RFC PATCH 1/2] smsutil: national to international conversion in status report

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Petteri,

On 09/15/2010 02:25 AM, Petteri Tikander wrote:
> ---
>  src/smsutil.c |   70 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 26c7951..2d47289 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -2826,14 +2826,18 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  	unsigned int offset = status_report->status_report.mr / 32;
>  	unsigned int bit = 1 << (status_report->status_report.mr % 32);
>  	struct id_table_node *node = NULL;
> -	const char *straddr;
> +	const char *straddr, *sent_addr;
> +	struct sms_address addr;
>  	GHashTable *id_table;
>  	gpointer key, value;
>  	gboolean delivered;
> -	GHashTableIter iter;
> +	GHashTableIter iter_addr, iter;
>  	gboolean pending;
>  	int i;
>  	unsigned int msg_id;
> +	unsigned int n_digits;
> +	unsigned int len_sent_addr;
> +	unsigned int len_rec_addr;
>  
>  	/* We ignore temporary or tempfinal status reports */
>  	if (sr_st_to_delivered(status_report->status_report.st,
> @@ -2841,20 +2845,54 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		return FALSE;
>  
>  	straddr = sms_address_to_string(&status_report->status_report.raddr);
> -	id_table = g_hash_table_lookup(assembly->assembly_table, straddr);
>  
> -	/* key (receiver address) does not exist in assembly */
> -	if (id_table == NULL)
> -		return FALSE;
> +	g_hash_table_iter_init(&iter_addr, assembly->assembly_table);
>  
> -	g_hash_table_iter_init(&iter, id_table);
> -	while (g_hash_table_iter_next(&iter, &key, &value)) {
> -		node = value;

So my thinking is that for efficiency purposes we should try the direct
lookup first.  We don't want to pay the penalty of walking the entire
hash table every time for networks that are 'sane'.  If that fails, fall
back to the 'fuzzy' lookup.  This would also make the code a bit easier
to follow I think...

> +	/*
> +	 * Go through all addresses. Each address can relate to
> +	 * 1-n msg_ids.
> +	 */
> +	while (g_hash_table_iter_next(&iter_addr, (gpointer) &sent_addr,
> +					(gpointer) &id_table)) {
> +		/*
> +		 * Some networks can change address to international format,
> +		 * although address is sent in the national format.
> +		 * So notify this special case by checking only
> +		 * last six digits. If address contains less than six digits,
> +		 * compare only existing digits.
> +		 */
> +		if ((straddr[0] == '+') && (sent_addr[0] != '+')) {

In theory it could also be vice-versa...

> +			len_sent_addr = strlen(sent_addr);
> +			len_rec_addr = strlen(straddr);
> +			n_digits = (len_sent_addr > 6) ? 6 : len_sent_addr;
>  
> -		if (node->mrs[offset] & bit)
> -			break;
> +			if (strcmp(&straddr[len_rec_addr - n_digits],
> +					&sent_addr[len_sent_addr - n_digits]))

Perhaps we should simply start at the last position of both strings and
compare in reverse.  If the match is MIN(6, strlen(sent_addr))
characters, then accept this as a 'fuzzy' match.

> +				continue;
> +		}
> +		/*
> +		 * In other cases the whole number received in the status report
> +		 * should match with the number originally sent.
> +		 */
> +		else if (strcmp(straddr, sent_addr))
> +				continue;
> +
> +		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;
>  
> -		node = NULL;
> +			node = NULL;
> +		}
> +
> +		/*
> +		 * Received address with MR matched with one of the stored
> +		 * addresses and MR, so no need to continue searching.
> +		 */
> +		if (node)
> +			break;
>  	}
>  
>  	/* Unable to find a message reference belonging to this address */
> @@ -2881,6 +2919,8 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  
>  	msg_id = *(unsigned int *) key;
>  
> +	sms_address_from_string(&addr, sent_addr);
> +
>  	if (pending == TRUE && node->deliverable == TRUE) {
>  		/*
>  		 * More status reports expected, and already received
> @@ -2888,7 +2928,7 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		 */
>  		sr_assembly_add_fragment_backup(
>  					assembly->imsi, node,
> -					&status_report->status_report.raddr,
> +					&addr,
>  					msg_id);
>  
>  		return FALSE;
> @@ -2901,14 +2941,14 @@ gboolean status_report_assembly_report(struct status_report_assembly *assembly,
>  		*out_id = msg_id;
>  
>  	sr_assembly_remove_fragment_backup(assembly->imsi,
> -					&status_report->status_report.raddr,
> +					&addr,
>  					msg_id);
>  
>  	g_hash_table_iter_remove(&iter);
>  
>  	if (g_hash_table_size(id_table) == 0)
>  		g_hash_table_remove(assembly->assembly_table,
> -				status_report->status_report.raddr.address);
> +					sent_addr);
>  
>  	return TRUE;
>  }

Otherwise it looks good.

Regards,
-Denis