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