Hi Paul,
sorry for the delay, here we go with my comments...
1) fix indentation. We don't use tabs use 4 spaces instead
2) I don't think we want to see such things in access log:
+ bb_alog_sms(conn, sms, "CONCATENATED SMS Part Received,
stored");
You should just log the concatenated sms after reassembling or each part if
reassembling failed.
3) why do you use RWLock:
+static RWLock *concat_rwlock;
I see only that you need mutex instead of rwlock.
4) in the init_concat_handler you should init:
+ incoming_concat_msgs = dict_create(1024, destroy_concatMsg);
based on incoming queue length. Otherwise it's possible that Dict will be
very slow.
5) this here should be warning or error, because user want to know what's
wrong:
+ debug("concat.sms",0, "Duplicate message part %d, ref %d, from %s.
Discarded!",
+ part, refnum, octstr_get_cstr(msg->sms.sender));
6) deadlock (goto done with rwlock locked):
+ /* Attempt to save the new one, if that fails, then reply with fail.
*/
+ if (store_save(msg) == -1) {
+ msg_destroy(msg);
+ *pmsg = msg = NULL;
+ ret = concat_error;
+ goto done;
+ } else
+ *pmsg = msg; /* return the message part. */
7) removing messages from the list that are too old should go to smsc2_route
because if you don't receive MOs then you will never clear list and it's
slow down MO handling. Here you should take into account that SMSC may be
just offline so you don't want to drop those messages from the list.
8) please don't use:
+ dict_put(incoming_concat_msgs, key, NULL); /* delete it. */
use dict_remove instead.
9) please also supply userguide patch
Paul Bagyenda wrote:
> Following your comments, here is a MO concatenation patch that now
> resides in bb_smscconn.c, and uses the key as advised below. Same
> code, largely, just integrated in a different place. There is also
> better handling of the message stores...
>
> Tests, comments, votes.
>
> P.
>
> Ps. If you are using the smsc_at2 module, be sure to patch with
> additional patch below.
--
Thanks,
Alex
