[PATCH] UCP/EMI keepalive/UCP31 bug.

[Andreas Fink]

On 05.08.2008, at 12:17, Vincent CHAVANIS wrote:

> Hi all,
>
> A bug still exists on the UCP/EMI driver.
> -Keepalive was wrong because the sender thread was awaken keepalive  
> + 1
> When you set keepalive=5 you will in reality have keepalive=6

so you say we where off by one second?

>
> -Then, the patch also fixes the UCP31 at login.
> We don't need to send it beacause UCP60 already make an activity on  
> the SMSC line. (The issue is that we need to wait_for_ack in order  
> to send MT = waste of time). Note that UCP31 is an alert operation  
> which can be used for a TCP keepalive.

This is not true. UCP31 is not in fact a keepalive. its used to alert  
the SMSC that we're ready to receive incoming SMS.
Login by itself COULD do this but not necessarily. Also there might be  
some situation where login is not done at all.

> From the point of view of <Bruno Rodrigues> who dit the previous  
> patch (2002-03-14) "a bug when Kannel couldn't connect to the server  
> on the first time, and then on a later attempt, the last_activity  
> was wrong and Kannel wouldn't send the 31's, or would send them at  
> the wrong time. [...] As a work around I made Kannel always send a  
> 31 after a successful login to make sure the timers were reset."
> This does not seems to happens anymore.

I would leave it in there. It doesn't hurt and probably helps some  
specific SMSC implementations.

> Any comments ?
>
> @@ -158,7 +159,7 @@
> #define  
> emi2_needs_keepalive 
> (conn)                                                     \
> (emi2_can_send(conn)  
> &&                                                                        \
>  (PRIVDATA(conn)->keepalive > 0)  
> &&                                                    \
> - (time(NULL) > (PRIVDATA(conn)->last_activity_time + PRIVDATA(conn)- 
> >keepalive)))
> + (time(NULL) >= (PRIVDATA(conn)->last_activity_time +  
> PRIVDATA(conn)->keepalive)))

so this is keepalive always being +1... sounds good to me.


> /*
>  * Send an EMI message and update the last_activity_time field.
> @@ -366,7 +373,7 @@
>             connect_error = 1;
>             continue;
>         }
> -        privdata->last_activity_time = 0; /* to force keepalive  
> after login */
> +        privdata->last_activity_time = time (NULL); /* to *NOT*  
> force keepalive after login */

as I said above, I would skip that one.

>                } else if (emimsg->ot == 31) {
> +                    PRIVDATA(conn)->last_activity_time = time (NULL);
>                    /* XXX Process error codes here
>                    if (octstr_get_char(emimsg->fields[0], 0) == 'N') {
>                        long errorcode;

not sure why this is needed. Would sound like it would have never  
worked otherwise...

> @@ -1269,7 +1317,7 @@
>  */
> static double emi2_get_timeouttime (SMSCConn *conn, Connection  
> *server)
> {
> -    double ka_timeouttime = PRIVDATA(conn)->keepalive ?  
> PRIVDATA(conn)->keepalive + 1 : DBL_MAX;
> +    double ka_timeouttime = PRIVDATA(conn)->keepalive ?  
> PRIVDATA(conn)->keepalive : DBL_MAX;

shouldn't timeout be longer than keepalive? could above change not  
create a race condition to the fact that the keepalive was "just about  
to be sent" but timeout hit first?

>
>     double idle_timeouttime = (PRIVDATA(conn)->idle_timeout &&  
> server) ? PRIVDATA(conn)->idle_timeout : DBL_MAX;
>     double result = ka_timeouttime < idle_timeouttime ?  
> ka_timeouttime : idle_timeouttime;
> @@ -1626,7 +1674,7 @@
>     privdata->listening_socket = -1;
>     privdata->can_write = 1;
>     privdata->priv_nexttrn = 0;
> -    privdata->last_activity_time = 0;
> +    privdata->last_activity_time = time (NULL); /* to *NOT* force  
> keepalive after login */

as I said I would leave keepalive at login.





Andreas Fink

Fink Consulting GmbH
Global Networks Schweiz AG
BebbiCell AG
IceCell ehf

---------------------------------------------------------------
Tel: +41-61-6666330 Fax: +41-61-6666331  Mobile: +41-79-2457333
Address: Clarastrasse 3, 4058 Basel, Switzerland
E-Mail:  andreas at fink.org
www.finkconsulting.com www.global-networks.ch www.bebbicell.ch
---------------------------------------------------------------
ICQ: 8239353 MSN: msn1 at gni.ch AIM: smsrelay Skype: andreasfink
Yahoo: finkconsulting SMS: +41792457333

http://a-fink.blogspot.com/   A developers view about iPhone SDK





-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.kannel.org/pipermail/devel/attachments/20080805/b2b0edd0/attachment-0001.html