Am 13.09.2010 um 19:25 schrieb Alejandro Guerrieri: > The uuid would be different each time you start the service right? > > What would happen if Kannel is restarted and there were pending parts to > send? The new uuid wouldn't match with the pending segments.
we have to resend all parts because split struct is not persistent... > > Regards, > -- > Alejandro Guerrieri > aguerri...@kannel.org > > > > On 13/09/2010, at 19:16, Alexander Malysh wrote: > >> Hi, >> >> great idea to use uuid to uniquely identify smscconn. we can use this for >> http admin as well. >> >> Thanks, >> Alexander Malysh >> >> Am 13.09.2010 um 19:04 schrieb Michael Zervakis: >> >>> Hi, >>> >>> I figured out that we can use the same mechanism as in Msg struct to >>> uniquelly identify each SMSCconn. In the attached patch the smscconn struct >>> was modified to include a uuid_t field and Msg struct to store the smscconn >>> uuid_t if we get a temporary error. Smsc_rout2 was also modified to respect >>> msg->msg_ConnId if set. >>> >>> SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. error -> >>> write the conn->uuid_t and put it into global queue for resend -> router >>> reads msg->msg_ConnId and sends failed part via same connection (if still >>> active) >>> >>> >>> -----Original Message----- >>> From: Alexander Malysh [mailto:malys...@googlemail.com] On Behalf Of >>> Alexander Malysh >>> Sent: Sunday, September 12, 2010 1:06 PM >>> To: Michael Zervakis >>> Cc: devel@kannel.org; pon...@appcell.net >>> Subject: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated >>> messages) >>> >>> >>> Hi, >>> >>> >>> as I already wrote, this patch is not enough. Please see ML wy... >>> >>> >>> Thanks, >>> >>> Alexander Malysh >>> >>> >>> >>> Am 09.09.2010 um 19:04 schrieb Michael Zervakis: >>> >>> >>>> Hi, >>> >>>> >>> >>>> This patch is a possible solution fog Bug #529. The Msg definition was >>>> modified so we can store the conn->id originally selected by router. >>> >>>> >>> >>>> SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. error -> >>>> write the conn->id and put it into global queue for resend -> router >>>> reads msg->sms.msg_ConnId and sends failed part via same connection (if >>>> still active) >>> >>>> >>> >>>> >>> >>>> Regards, >>> >>>> >>> >>>> >>> >>>> From: devel-boun...@kannel.org [mailto:devel-boun...@kannel.org] On Behalf >>>> Of Konstantin Vayner >>> >>>> Sent: Thursday, December 17, 2009 12:28 PM >>> >>>> To: Alexander Malysh >>> >>>> Cc: devel@kannel.org >>> >>>> Subject: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated >>>> messages) >>> >>>> >>> >>>> >>> >>>> why remembering smsc-id in sms.smsc_id is not enough? >>> >>>> how does smsbox remember routing when i submit a message with predefined >>>> smsc id from http (sendsms) ? >>> >>>> >>> >>>> On Thu, Dec 17, 2009 at 12:10 PM, Alexander Malysh <amal...@kannel.org> >>>> wrote: >>> >>>> >>> >>>> >>> >>>> Am 17.12.2009 um 10:43 schrieb Konstantin Vayner: >>> >>>> >>> >>>> >>> >>>> >>> >>>> so the best option would be to requeue the part via same smsc, right ? >>> >>>> >>> >>>> >>> >>>> yes, but it's not easy todo. You have to remember SMSC pointer not only >>>> SMSC-name/id and then teach all routing parts >>> >>>> >>> >>>> to respect it... >>> >>>> >>> >>>> >>> >>>> >>> >>>> cause requeueing all parts may also get extra messages to the handset >>>> despite it not being able to reconstruct (not to mention the extra money >>>> ;) ) >>> >>>> >>> >>>> On Thu, Dec 17, 2009 at 11:33 AM, Alexander Malysh <amal...@kannel.org> >>>> wrote: >>> >>>> >>> >>>> Hi, >>> >>>> >>> >>>> >>> >>>> unfortunately this will not work as expected (the rule is: _all_ parts if >>>> multipart message have to be send via the same SMSC)... >>> >>>> >>> >>>> >>> >>>> example: >>> >>>> >>> >>>> >>> >>>> SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. >>>> error -> you put it into global queue for resend -> 2 part sent via SMSC-B >>>> -> handset rejects it >>> >>>> >>> >>>> >>> >>>> We have only two possibility here: >>> >>>> >>> >>>> 1) if temp error occurs put the _whole_ message into resend queue and >>>> resend then _all_ parts (very easy todo) >>> >>>> >>> >>>> 2) remember smsc which was used for first parts and resend it via the same >>>> smsc (complicated but save money :) ) >>> >>>> >>> >>>> >>> >>>> Thanks, >>> >>>> >>> >>>> Alexander Malysh >>> >>>> >>> >>>> >>> >>>> Am 16.12.2009 um 18:17 schrieb Konstantin Vayner: >>> >>>> >>> >>>> >>> >>>> >>> >>>> Bug report: http://redmine.kannel.org/issues/show/529 >>> >>>> >>> >>>> Quote from gw/bb_smscconn.c : >>> >>>> >>> >>>> static void handle_split(SMSCConn *conn, Msg *msg, long reason) >>> >>>> { >>> >>>> struct split_parts *split = msg->sms.split_parts; >>> >>>> >>> >>>> /* >>> >>>> * If temporarely failed, try again immediately but only if connection >>>> active. >>> >>>> * Because if connection is not active we will loop for ever here >>>> consuming 100% CPU >>> >>>> * time due to internal queue cleanup in smsc module that call >>>> bb_smscconn_failed. >>> >>>> */ >>> >>>> if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>>> SMSCCONN_ACTIVE && >>> >>>> smscconn_send(conn, msg) == 0) { >>> >>>> /* destroy this message because it will be duplicated in smsc module */ >>> >>>> msg_destroy(msg); >>> >>>> return; >>> >>>> } >>> >>>> >>> >>>> (end quote) >>> >>>> >>> >>>> So, if an smsc is alive and throws temporary error every time you try to >>>> submit such a message, we enter endless loop of attempting to resend it.... >>> >>>> >>> >>>> >>> >>>> Suggested patch follows (also attached). >>> >>>> Sorry its not cvs diff - having firewall issues accessing pserver now so i >>>> ran diff vs snapshot generated yesterday >>> >>>> I will be able to produce a normal cvs diff tomorrow morning if it is >>>> needed >>> >>>> >>> >>>> >>> >>>> --- kannel-snapshot/gw/bb_smscconn.c 2009-11-15 16:12:28.000000000 +0200 >>> >>>> +++ gateway-cvs/gw/bb_smscconn.c 2009-12-16 19:47:32.000000000 +0200 >>> >>>> @@ -203,18 +203,6 @@ >>> >>>> struct split_parts *split = msg->sms.split_parts; >>> >>>> /* >>> >>>> - * If temporarely failed, try again immediately but only if >>>> connection active. >>> >>>> - * Because if connection is not active we will loop for ever here >>>> consuming 100% CPU >>> >>>> - * time due to internal queue cleanup in smsc module that call >>>> bb_smscconn_failed. >>> >>>> - */ >>> >>>> - if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>>> SMSCCONN_ACTIVE && >>> >>>> - smscconn_send(conn, msg) == 0) { >>> >>>> - /* destroy this message because it will be duplicated in smsc >>>> module */ >>> >>>> - msg_destroy(msg); >>> >>>> - return; >>> >>>> - } >>> >>>> - - /* >>> >>>> * if the reason is not a success and status is still success >>> >>>> * then set status of a split to the reason. >>> >>>> * Note: reason 'malformed','discarded' or 'rejected' has higher priority! >>> >>>> @@ -303,7 +291,7 @@ >>> >>>> void bb_smscconn_send_failed(SMSCConn *conn, Msg *sms, int reason, Octstr >>>> *reply) >>> >>>> { >>> >>>> - if (sms->sms.split_parts != NULL) { >>> >>>> + if (reason != SMSCCONN_FAILED_TEMPORARILY && sms->sms.split_parts != >>>> NULL) { >>> >>>> handle_split(conn, sms, reason); >>> >>>> octstr_destroy(reply); >>> >>>> return; >>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> >>>> <bb_smscconn.diff> >>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> >>>> Index: gw/bb_smscconn.c >>> >>>> =================================================================== >>> >>>> --- gw/bb_smscconn.c (revision 4838) >>> >>>> +++ gw/bb_smscconn.c (working copy) >>> >>>> @@ -207,11 +207,34 @@ >>> >>>> * Because if connection is not active we will loop for ever here >>>> consuming 100% CPU >>> >>>> * time due to internal queue cleanup in smsc module that call >>>> bb_smscconn_failed. >>> >>>> */ >>> >>>> - if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>>> SMSCCONN_ACTIVE && >>> >>>> - smscconn_send(conn, msg) == 0) { >>> >>>> + /*if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) >>>> == SMSCCONN_ACTIVE && >>> >>>> + smscconn_send(conn, msg) == 0) { */ >>> >>>> /* destroy this message because it will be duplicated in smsc module >>>> */ >>> >>>> - msg_destroy(msg); >>> >>>> + /* msg_destroy(msg); >>> >>>> return; >>> >>>> + }*/ >>> >>>> + >>>> + >>>> + >>> >>>> + if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>>> SMSCCONN_ACTIVE) >>> >>>> + { >>> >>>> + if (sms_resend_retry >= 0 && msg->sms.resend_try >= >>>> sms_resend_retry) /*check how many times the split message has been sent */ >>> >>>> + { >>>> + warning(0, "SMPP[%s] :Maximum retries for message [%s] exceeded >>>> resend %ld times, discarding it!", octstr_get_cstr(conn->id), >>>> octstr_get_cstr(msg->sms.msgdata),msg->sms.resend_try); >>> >>>> + handle_split(conn, msg, SMSCCONN_FAILED_DISCARDED); >>>> + /* msg_destroy(msg); */ >>> >>>> + return; >>> >>>> + } >>> >>>> + >>>> + warning(0, "SMPP[%s] :Split message failed temporarily sending >>>> back to sms_router", octstr_get_cstr(conn->id)); >>> >>>> + >>>> + msg->sms.resend_try = (msg->sms.resend_try > 0 ? >>>> msg->sms.resend_try + 1 : 1); >>> >>>> + debug("bb.sms.splits", 0, "SMPP[%s] :Resend counter of message set >>>> to %ld", octstr_get_cstr(conn->id),msg->sms.resend_try); >>> >>>> + time(&msg->sms.resend_time); >>> >>>> + msg->sms.msg_ConnId = octstr_duplicate(conn->id); >>>> + >>>> + gwlist_produce(outgoing_sms, msg); /* write it back to global >>>> queue */ >>> >>>> + return; >>> >>>> } >>> >>>> >>> >>>> /* >>> >>>> @@ -1229,6 +1252,28 @@ >>> >>>> bp_load = bo_load = queue_length = 0; >>> >>>> >>> >>>> conn = NULL; >>> >>>> + /* if msg was a failed part then route to the defined SMSC connection >>>> */ >>> >>>> + if (msg->sms.msg_ConnId != NULL) >>> >>>> + { >>>> + for (i=0; i < gwlist_len(smsc_list); i++) >>> >>>> + { >>> >>>> + conn = gwlist_get(smsc_list, i); >>> >>>> + smscconn_info(conn, &info); >>> >>>> + queue_length += (info.queued > 0 ? info.queued : 0); >>> >>>> + >>>> + if(octstr_compare(msg->sms.msg_ConnId, conn->id) == 0) >>> >>>> + { >>>> + if((smscconn_usable(conn,msg) != -1) && (info.status == >>>> SMSCCONN_ACTIVE && info.queued < max_queue)) >>> >>>> + { >>> >>>> + best_preferred = conn; >>>> + debug("bb.sms",0,"Message with pre-set connection to >>>> SMSC[%s] found",octstr_get_cstr(msg->sms.msg_ConnId)); >>> >>>> + continue; >>> >>>> + } >>> >>>> + } >>> >>>> + } >>> >>>> + } >>> >>>> + else >>> >>>> + { >>> >>>> for (i=0; i < gwlist_len(smsc_list); i++) { >>> >>>> conn = gwlist_get(smsc_list, (i+s) % gwlist_len(smsc_list)); >>> >>>> >>> >>>> @@ -1265,6 +1310,8 @@ >>> >>>> bo_load = info.load; >>> >>>> } >>> >>>> } >>> >>>> + } >>> >>>> + >>>> queue_length += gwlist_len(outgoing_sms); >>> >>>> if (max_outgoing_sms_qlength > 0 && !resend && >>> >>>> queue_length > gwlist_len(smsc_list) * max_outgoing_sms_qlength) { >>> >>>> Index: gw/msg-decl.h >>> >>>> =================================================================== >>> >>>> --- gw/msg-decl.h (revision 4838) >>> >>>> +++ gw/msg-decl.h (working copy) >>> >>>> @@ -85,6 +85,7 @@ >>> >>>> OCTSTR(msgdata) >>> >>>> INTEGER(time) >>> >>>> OCTSTR(smsc_id) >>> >>>> + OCTSTR(msg_ConnId); /* field required to reroute concatenated Msg >>>> parts via the same SMSC connection. Bug 529*/ >>> >>>> OCTSTR(smsc_number) >>> >>>> OCTSTR(foreign_id) >>> >>>> OCTSTR(service) >>> >>> >>> Index: bb_smscconn.c >>> =================================================================== >>> --- bb_smscconn.c (revision 4843) >>> +++ bb_smscconn.c (working copy) >>> @@ -201,17 +201,46 @@ >>> static void handle_split(SMSCConn *conn, Msg *msg, long reason) >>> { >>> struct split_parts *split = msg->sms.split_parts; >>> + char id[UUID_STR_LEN + 1]; >>> >>> + >>> /* >>> * If temporarely failed, try again immediately but only if connection >>> active. >>> * Because if connection is not active we will loop for ever here >>> consuming 100% CPU >>> * time due to internal queue cleanup in smsc module that call >>> bb_smscconn_failed. >>> */ >>> - if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>> SMSCCONN_ACTIVE && >>> - smscconn_send(conn, msg) == 0) { >>> + /*if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) >>> == SMSCCONN_ACTIVE && >>> + smscconn_send(conn, msg) == 0) { */ >>> /* destroy this message because it will be duplicated in smsc module >>> */ >>> - msg_destroy(msg); >>> + /* msg_destroy(msg); >>> return; >>> + }*/ >>> + >>> + >>> + if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == >>> SMSCCONN_ACTIVE) >>> + { >>> + if (sms_resend_retry >= 0 && msg->sms.resend_try >= >>> sms_resend_retry) /*check how many times the split message has been sent */ >>> + { >>> + uuid_unparse(msg->sms.id, id); >>> + warning(0, "SMPP[%s] :Maximum retries for message [%s] exceeded >>> resend %ld times, discarding it!", octstr_get_cstr(conn->id), id, >>> msg->sms.resend_try); >>> + handle_split(conn, msg, SMSCCONN_FAILED_DISCARDED); >>> + return; >>> + } >>> + >>> + >>> + uuid_unparse(msg->sms.id, id); >>> + warning(0, "SMPP[%s] :Split message [%s] failed temporary sending >>> it to sms_router but with fixed connection", octstr_get_cstr(conn->id), >>> id); >>> + >>> + msg->sms.resend_try = (msg->sms.resend_try > 0 ? >>> msg->sms.resend_try + 1 : 1); /* inc its retries */ >>> + debug("bb.sms.splits", 0, "SMPP[%s] :Resend counter of message [%s] >>> set to %ld", octstr_get_cstr(conn->id), id, msg->sms.resend_try); >>> + time(&msg->sms.resend_time); /* take resend time - this will be >>> used in sms_router */ >>> + >>> + >>> + if(uuid_is_null(msg->msg_ConnId) == 1) >>> + uuid_copy(msg->msg_ConnId, conn->smscconn_uuid); >>> + >>> + gwlist_produce(outgoing_sms, msg); /* send it back to global queue >>> */ >>> + return; >>> } >>> >>> /* >>> @@ -1187,8 +1216,8 @@ >>> long bp_load, bo_load; >>> int i, s, ret, bad_found, full_found; >>> long max_queue, queue_length; >>> - char *uf; >>> - >>> + char *uf, id[UUID_STR_LEN + 1]; >>> + >>> /* XXX handle ack here? */ >>> if (msg_type(msg) != sms) { >>> error(0, "Attempt to route non SMS message through smsc2_rout!"); >>> @@ -1229,6 +1258,29 @@ >>> bp_load = bo_load = queue_length = 0; >>> >>> conn = NULL; >>> + /*if msg was a split and failed temporary then use stored connection */ >>> + if(uuid_is_null(msg->msg_ConnId) != 1) >>> + { >>> + for (i=0; i < gwlist_len(smsc_list); i++) >>> + { >>> + conn = gwlist_get(smsc_list, i); >>> + smscconn_info(conn, &info); >>> + queue_length += (info.queued > 0 ? info.queued : 0); >>> + >>> + if(uuid_compare(msg->msg_ConnId, conn->smscconn_uuid) == 0) >>> + { >>> + if((smscconn_usable(conn,msg) != -1) && (info.status == >>> SMSCCONN_ACTIVE && info.queued < max_queue)) >>> + { >>> + best_preferred = conn; >>> + uuid_unparse(msg->sms.id, id); >>> + debug("bb.sms",0,"Message [%s] with pre-set connection >>> found", id); >>> + continue; >>> + } >>> + } >>> + } >>> + } >>> + else >>> + { >>> for (i=0; i < gwlist_len(smsc_list); i++) { >>> conn = gwlist_get(smsc_list, (i+s) % gwlist_len(smsc_list)); >>> >>> @@ -1265,6 +1317,8 @@ >>> bo_load = info.load; >>> } >>> } >>> + } >>> + >>> queue_length += gwlist_len(outgoing_sms); >>> if (max_outgoing_sms_qlength > 0 && !resend && >>> queue_length > gwlist_len(smsc_list) * max_outgoing_sms_qlength) { >>> Index: msg.c >>> =================================================================== >>> --- msg.c (revision 4843) >>> +++ msg.c (working copy) >>> @@ -98,6 +98,7 @@ >>> msg = gw_malloc_trace(sizeof(Msg), file, line, func); >>> >>> msg->type = type; >>> + uuid_clear(msg->msg_ConnId); >>> #define INTEGER(name) p->name = MSG_PARAM_UNDEFINED; >>> #define OCTSTR(name) p->name = NULL; >>> #define UUID(name) uuid_generate(p->name); >>> @@ -113,7 +114,7 @@ >>> Msg *new; >>> >>> new = msg_create(msg->type); >>> - >>> + uuid_copy(new->msg_ConnId, msg->msg_ConnId); >>> #define INTEGER(name) p->name = q->name; >>> #define OCTSTR(name) \ >>> if (q->name == NULL) p->name = NULL; \ >>> Index: msg.h >>> =================================================================== >>> --- msg.h (revision 4843) >>> +++ msg.h (working copy) >>> @@ -78,7 +78,7 @@ >>> >>> typedef struct { >>> enum msg_type type; >>> - >>> + uuid_t msg_ConnId; >>> #define INTEGER(name) long name; >>> #define OCTSTR(name) Octstr *name; >>> #define UUID(name) uuid_t name; >>> Index: smscconn.c >>> =================================================================== >>> --- smscconn.c (revision 4843) >>> +++ smscconn.c (working copy) >>> @@ -157,13 +157,16 @@ >>> Octstr *denied_prefix_regex; >>> Octstr *preferred_prefix_regex; >>> Octstr *tmp; >>> - >>> + char id[UUID_STR_LEN + 1]; >>> + >>> if (grp == NULL) >>> return NULL; >>> >>> conn = gw_malloc(sizeof(*conn)); >>> memset(conn, 0, sizeof(*conn)); >>> >>> + uuid_generate(conn->smscconn_uuid); >>> + >>> conn->why_killed = SMSCCONN_ALIVE; >>> conn->status = SMSCCONN_CONNECTING; >>> conn->connect_time = -1; >>> @@ -295,7 +298,10 @@ >>> gw_assert(conn->send_msg != NULL); >>> >>> bb_smscconn_ready(conn); >>> - >>> + >>> + uuid_unparse(conn->smscconn_uuid, id); >>> + debug("smscconn",0,"Adding smsc connection [%s] with id [%s]", >>> octstr_get_cstr(conn->id), id); >>> + >>> return conn; >>> } >>> >>> Index: smscconn_p.h >>> =================================================================== >>> --- smscconn_p.h (revision 4843) >>> +++ smscconn_p.h (working copy) >>> @@ -146,6 +146,8 @@ >>> #include "smscconn.h" >>> >>> struct smscconn { >>> + /* variable added in order to uniquely identify each smsc connection >>> */ >>> + uuid_t smscconn_uuid; >>> /* variables set by appropriate SMSCConn driver */ >>> smscconn_status_t status; /* see smscconn.h */ >>> int load; /* load factor, 0 = no load */ >> >> >
