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