Hi Michael, you don't have to alter Msg struct. Just add smscconn_uuid to struct split_parts in gw/msg.h.
Thanks, Alexander Malysh Am 13.09.2010 um 21:31 schrieb Michael Zervakis: > > > Hi, > > I forgot to mention that msg->msg_ConnId uuid is not saved in store since > it's useless because if the process restarts the whole Msg will be resend. At > least this solution will apply sms-resend-* directives for long sms. > > BR, > > -----Original Message----- > From: Alejandro Guerrieri [mailto:[email protected]] > Sent: Monday, September 13, 2010 8:25 PM > To: Alexander Malysh > Cc: Michael Zervakis; [email protected]; [email protected] > Subject: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated > messages) > > > 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. > > > Regards, > > -- > > Alejandro Guerrieri > > [email protected] > > > > > 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:[email protected]] On Behalf Of > >> Alexander Malysh > > >> Sent: Sunday, September 12, 2010 1:06 PM > > >> To: Michael Zervakis > > >> Cc: [email protected]; [email protected] > > >> 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: [email protected] [mailto:[email protected]] On > >>> Behalf Of Konstantin Vayner > > >> > > >>> Sent: Thursday, December 17, 2009 12:28 PM > > >> > > >>> To: Alexander Malysh > > >> > > >>> Cc: [email protected] > > >> > > >>> 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 <[email protected]> > >>> 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 <[email protected]> > >>> 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 */ > > > > > > > >
