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

[Petteri Tikander <petteri.tikander@ixonos.com>]

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

Hi Denis,

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

OK. Direct lookup would do this same and look nicer. But I think (but not 
sure) that g_hash_table_lookup() does similar looping internally, as my 
addition for while-looping address-tables. Well I was already taking 
g_hash_table_lookup() back, but just making the code uglier when adding 
'international to national'-logic. That's because of iterating msg-id nodes, 
so solution lead to two node-iteration loops.

My idea was to find suitable address&Mr-pair. So if address matches but MR 
doesn't, let's continue (while-loop inside while-loop).
Was your idea actually that this inefficency-problem in looping occurs actually 
in this point: if for some reason in sane networks the address matches, but MR 
doesn't, function continues looping although it shouldn't? And then it loops 
the entire table. In 'insane'-networks this is OK, because function is not 
comparing necessarily the whole address?

> 
> > +	/*
> > +	 * 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...

OK. Let's also taking care of 'international to national'-case.

> 
> > +			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.
> 

OK. Lets' compare single characters in the reverse order.

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

Have a nice week end.

Br, Petteri