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