Re: {PATCH] final smsc-id passing in ack msg type

Hi Stipe,

did you really tested this patch? I don't think so...

in smsc2_route(...):
/* function to route outgoing SMS'es
  *
  * If finds a good one, puts into it and returns SMSCCONN_SUCCESS
  * If finds only bad ones, but acceptable, queues and
  * returns SMSCCONN_QUEUED  (like all acceptable currently  
disconnected)
  * if message acceptable but queues full returns  
SMSCCONN_FAILED_QFULL and
  * message is not destroyed.
  * If cannot find nothing at all, returns SMSCCONN_FAILED_DISCARDED and
  * message is NOT destroyed (otherwise it is)
  */

and then at the end of the function:
     msg_destroy(msg);
     return SMSCCONN_SUCCESS;
}

so you have 100% bum in bb_boxc.

Beside implementation error I don't think this will work as you would  
like.
As far as message accepted by bearerbox you want return smsc-id in  
ack. That's ok.
But what do you want todo if the same message is then temporarily  
undelivered by this smsc and
you make retry? What is todo there, you already returned "wrong" smsc- 
id to smsbox.
So this feature just doesn't make any sense for me.

Thanks,
Alex

Am 20.03.2009 um 14:53 schrieb Stipe Tolj:

> Hi list,
>
> I'd like to introduce an easy concept where bearerbox can inform  
> smsbox
> connections about the final smsc-id the msg has been assigned to.
>
> Bearerbox simply re-writes the msg->sms.smsc_id value as soon as the
> smsc2_rout() routing has occurred and we're drilled down to the  
> assigned smsc
> connection.
>
> That value is used then in a new field for the ack msg type:
>
>    mack->ack.smsc_id = octstr_duplicate(msg->sms.smsc_id);
>
> and the ack is send as usual to the boxc connection. The smsbox can  
> then (as
> example usage) put that final routing decision to the HTTP response  
> body:
>
>    answer = octstr_format("0: Accepted for delivery by smsc-id '%s'",
>
> Please apply against CVS HEAD, test and comment.
>
> Thanks,
> Stipe
>
> BTW, the core of the patch is pretty minimal, while the patch file  
> includes some
> source code re-formating to comply to our CodingStyle.
>
> -- 
> -------------------------------------------------------------------
> Kölner Landstrasse 419
> 40589 Düsseldorf, NRW, Germany
>
> tolj.org system architecture      Kannel Software Foundation (KSF)
> http://www.tolj.org/              http://www.kannel.org/
>
> mailto:st_{at}_tolj.org           mailto:stolj_{at}_kannel.org
> -------------------------------------------------------------------
> ### Eclipse Workspace Patch 1.0
> #P gateway-cvs-head
> Index: gw/bb_boxc.c
> ===================================================================
> RCS file: /home/cvs/gateway/gw/bb_boxc.c,v
> retrieving revision 1.94
> diff -u -r1.94 bb_boxc.c
> --- gw/bb_boxc.c	2 Mar 2009 01:35:38 -0000	1.94
> +++ gw/bb_boxc.c	20 Mar 2009 02:25:47 -0000
> @@ -215,8 +215,9 @@
>     int rc;
>
>     /*
> -     * save modifies ID and time, so if the smsbox uses it, save
> -     * it FIRST for the reply message!!!
> +     * Make sure we use the same UUID and time for the
> +     * ack response message towards smsbox.
> +     * A smsbox connection logic may depend on it.
>      */
>     mack = msg_create(ack);
>     gw_assert(mack != NULL);
> @@ -225,31 +226,43 @@
>
>     store_save(msg);
>
> +    /* call our message router */
>     rc = smsc2_rout(msg, 0);
> -    switch(rc) {
> +
> +    /* provide the routing information via ack
> +     * to the attached smsbox connection */
> +    mack->ack.smsc_id = octstr_duplicate(msg->sms.smsc_id);
> +
> +    switch (rc) {
>         case SMSCCONN_SUCCESS:
> -           mack->ack.nack = ack_success;
> -           break;
> +            mack->ack.nack = ack_success;
> +            break;
> +
>         case SMSCCONN_QUEUED:
> -           mack->ack.nack = ack_buffered;
> -           break;
> +            mack->ack.nack = ack_buffered;
> +            break;
> +
>         case SMSCCONN_FAILED_DISCARDED: /* no router at all */
>         case SMSCCONN_FAILED_QFULL: /* queue full */
> -           warning(0, "Message rejected by bearerbox, %s!",
> -                             (rc == SMSCCONN_FAILED_DISCARDED) ?  
> "no router" : "queue full");
> -           /*
> -            * first create nack for store-file, in order to delete
> -            * message from store-file.
> -            */
> -           store_save_ack(msg, (rc == SMSCCONN_FAILED_QFULL ?  
> ack_failed_tmp : ack_failed));
> -           mack->ack.nack = (rc == SMSCCONN_FAILED_QFULL ?  
> ack_failed_tmp : ack_failed);
> -
> -           /* destroy original message */
> -           msg_destroy(msg);
> -           break;
> +            warning(0, "Message rejected by bearerbox, %s!",
> +                    (rc == SMSCCONN_FAILED_DISCARDED) ? "no  
> router" : "queue full");
> +
> +            /*
> +             * First create nack for store-file, in order to delete
> +             * message from store-file.
> +             */
> +            store_save_ack(msg, (rc == SMSCCONN_FAILED_QFULL ?  
> ack_failed_tmp : ack_failed));
> +            mack->ack.nack = (rc == SMSCCONN_FAILED_QFULL ?  
> ack_failed_tmp : ack_failed);
> +
> +            /* destroy original message */
> +            msg_destroy(msg);
> +            break;
> +
> +        default:
> +            break;
>     }
>
> -    /* put ack into incoming queue of conn */
> +    /* put ack into incoming queue of conn (smsbox connection) */
>     send_msg(conn, mack);
>     msg_destroy(mack);
> }
> Index: gw/smsbox.c
> ===================================================================
> RCS file: /home/cvs/gateway/gw/smsbox.c,v
> retrieving revision 1.282
> diff -u -r1.282 smsbox.c
> --- gw/smsbox.c	14 Jan 2009 11:11:46 -0000	1.282
> +++ gw/smsbox.c	20 Mar 2009 02:25:50 -0000
> @@ -184,33 +184,45 @@
>         octstr_destroy(os);
>         return;
>     }
> -    /* XXX  this should be fixed so that we really wait for DLR
> -     *      SMSC accept/deny before doing this - but that is far
> -     *      more slower, a bit more complex, and is done later on
> +
> +    /*
> +     * TODO: move from 'semi-waiting' to 'full-waiting' method.
> +     *
> +     * Actually this waiting mechanism is 'semi-waiting'. All that we
> +     * are sure of is that bearerbox was able to assign the message
> +     * in the lower smsc module layer to a specific smsc connection,
> +     * and queued for delivery. This is no real ACK state from the
> +     * SMSC side itself.
> +     *
> +     * This would imply waiting for the corresponding  
> submit_sm_resp PDU
> +     * i.e. of the SMPP protocol. This coudl be only done  
> efficiently via
> +     * asynchronous callback passing of the messages. -- st.
>      */
>
>     switch (msg->ack.nack) {
> -      case ack_success:
> -        status = HTTP_ACCEPTED;
> -        answer = octstr_create("0: Accepted for delivery");
> -        break;
> -      case ack_buffered:
> -        status = HTTP_ACCEPTED;
> -        answer = octstr_create("3: Queued for later delivery");
> -        break;
> -      case ack_failed:
> -        status = HTTP_FORBIDDEN;
> -        answer = octstr_create("Not routable. Do not try again.");
> -        break;
> -      case ack_failed_tmp:
> -        status = HTTP_SERVICE_UNAVAILABLE;
> -        answer = octstr_create("Temporal failure, try again later.");
> -        break;
> -      default:
> -	error(0, "Strange reply from bearerbox!");
> -        status = HTTP_SERVICE_UNAVAILABLE;
> -        answer = octstr_create("Temporal failure, try again later.");
> -        break;
> +        case ack_success:
> +            status = HTTP_ACCEPTED;
> +            answer = octstr_format("0: Accepted for delivery by  
> smsc-id '%s'",
> +                        octstr_get_cstr(msg->ack.smsc_id));
> +            break;
> +        case ack_buffered:
> +            status = HTTP_ACCEPTED;
> +            answer = octstr_format("3: Queued for later delivery by  
> smsc-id '%s'",
> +                         octstr_get_cstr(msg->ack.smsc_id));
> +            break;
> +        case ack_failed:
> +            status = HTTP_FORBIDDEN;
> +            answer = octstr_create("Not routable. Do not try  
> again.");
> +            break;
> +        case ack_failed_tmp:
> +            status = HTTP_SERVICE_UNAVAILABLE;
> +            answer = octstr_create("Temporal failure, try again  
> later.");
> +            break;
> +        default:
> +            error(0, "Strange reply from bearerbox!");
> +            status = HTTP_SERVICE_UNAVAILABLE;
> +            answer = octstr_create("Temporal failure, try again  
> later.");
> +            break;
>     }
>
>     http_send_reply(client, status, sendsms_reply_hdrs, answer);
> @@ -244,32 +256,35 @@
>         else if (msg == NULL) /* just to be sure, may not happens */
>             break;
>
> -	if (msg_type(msg) == admin) {
> -	    if (msg->admin.command == cmd_shutdown) {
> -		info(0, "Bearerbox told us to die");
> -		program_status = shutting_down;
> -	    } else if (msg->admin.command == cmd_restart) {
> -		info(0, "Bearerbox told us to restart");
> -		restart = 1;
> -		program_status = shutting_down;
> -	    }
> -	    /*
> -	     * XXXX here should be suspend/resume, add RSN
> -	     */
> -	    msg_destroy(msg);
> -	} else if (msg_type(msg) == sms) {
> -	    if (total == 0)
> -		start = time(NULL);
> -	    total++;
> -	    gwlist_produce(smsbox_requests, msg);
> -	} else if (msg_type(msg) == ack) {
> -	    if (!immediate_sendsms_reply)
> -		delayed_http_reply(msg);
> -	    msg_destroy(msg);
> -	} else {
> -	    warning(0, "Received other message than sms/admin, ignoring!");
> -	    msg_destroy(msg);
> -	}
> +        if (msg_type(msg) == admin) {
> +            if (msg->admin.command == cmd_shutdown) {
> +                info(0, "Bearerbox told us to die");
> +                program_status = shutting_down;
> +            } else if (msg->admin.command == cmd_restart) {
> +                info(0, "Bearerbox told us to restart");
> +                restart = 1;
> +                program_status = shutting_down;
> +            }
> +            /*
> +             * TODO: here should be suspend/resume, add RSN
> +             */
> +            msg_destroy(msg);
> +        }
> +        else if (msg_type(msg) == sms) {
> +            if (total == 0)
> +                start = time(NULL);
> +            total++;
> +            gwlist_produce(smsbox_requests, msg);
> +        }
> +        else if (msg_type(msg) == ack) {
> +            if (!immediate_sendsms_reply)
> +                delayed_http_reply(msg);
> +            msg_destroy(msg);
> +        }
> +        else {
> +            warning(0, "Received other message than sms/admin,  
> ignoring!");
> +            msg_destroy(msg);
> +        }
>     }
>     secs = difftime(time(NULL), start);
>     info(0, "Received (and handled?) %d requests in %d seconds "
> Index: gw/smscconn.c
> ===================================================================
> RCS file: /home/cvs/gateway/gw/smscconn.c,v
> retrieving revision 1.59
> diff -u -r1.59 smscconn.c
> --- gw/smscconn.c	12 Jan 2009 16:46:56 -0000	1.59
> +++ gw/smscconn.c	20 Mar 2009 02:25:50 -0000
> @@ -499,6 +499,7 @@
> {
>     int ret = -1;
>     List *parts = NULL;
> +    const Octstr *cid;
>
>     gw_assert(conn != NULL);
>     mutex_lock(conn->flow_mutex);
> @@ -506,6 +507,17 @@
>         mutex_unlock(conn->flow_mutex);
>         return -1;
>     }
> +
> +    /*
> +     * We're done now with routing and have our smsc connection
> +     * assigned to be used. In order to get accurate logging of
> +     * the smsc-id the messages goes, and for the ACK/NACK msg
> +     * towards smsbox we replace the value here.
> +     */
> +    if (conn && (cid = smscconn_id(conn))) {
> +        octstr_destroy(msg->sms.smsc_id);
> +        msg->sms.smsc_id = octstr_duplicate(cid);
> +    }
>
>     /* if this a retry of splitted message, don't unify prefix and  
> don't try to split */
>     if (msg->sms.split_parts == NULL) {
> Index: gw/msg-decl.h
> ===================================================================
> RCS file: /home/cvs/gateway/gw/msg-decl.h,v
> retrieving revision 1.37
> diff -u -r1.37 msg-decl.h
> --- gw/msg-decl.h	14 Jan 2009 11:11:46 -0000	1.37
> +++ gw/msg-decl.h	20 Mar 2009 02:25:47 -0000
> @@ -118,6 +118,7 @@
> 		INTEGER(nack);
> 		INTEGER(time);
> 		UUID(id);
> +        OCTSTR(smsc_id);
> 	})
>
> MSG(wdp_datagram,