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
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
