[asterisk-dev] [Code Review] Fix timeout antipattern throughout the code

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2135/
-----------------------------------------------------------

(Updated Oct. 30, 2012, 10:56 a.m.)


Review request for Asterisk Developers.


Changes
-------

May this be the final round of updates.


Summary
-------

This is an interesting one. I filed issue ASTERISK-20375 a while back and David Lee fixed it. The problem was that in my setup, a call to ast_waitfor_nandfds() returned in less than a millisecond. The result was that the input-output value passed into ast_waitfor_nandfds() would never decrement the value properly. Thus a 3000 ms timeout would take *days* to actually time out.

David discovered that a similar pattern for determining the passage of time had been used throughout the code. This patch is an attempt to fix all of those. I've created a function in time.h called ast_remaining_ms() that will tell the amount of time remaining given a starting timestamp and the duration that the original timeout was. This is useful for determining the timeout to pass to an ast_waitfor() or related function.

Most of the changes here were straightforward, but there were some tricky bits. Focus strongly on the following changes:

1) wait_for_answer() in app_dial.c
2) wait_for_answer() in app_queue.c
3) generic_fax_exec() in res_fax.c
4) places where I have started setting "res" instead of "ms" or something similar in ast_waitfor() calls. Ensure that I have not tainted a return value. I found a few of these places while testing and fixed what I found on my own, but I could have missed some places.
5) places where a timeout is supposed to restart. I've tried to find these places, but I may have missed some.

In addition, there has been a change made to the return of ast_waitfor(). Prior to this patch, if a negative timeout were passed to ast_waitfor(), it would be impossible to return a negative value. This meant that it was impossible to detect if an error had occurred. I've changed ast_waitfor() to be able to return a negative value if a negative timeout is passed in, but only if the underlying call to ast_waitfor_nandfds() returns NULL.

I did this because I found many places in the code with this construct:

if (ast_waitfor(chan, -1) < 0) {
    ast_log(LOG_ERROR, "ERMAHGERD!\n");
    return HORRIBLE_ERROR_CONDITION;
}

The problem is that the error return could never actually be reached. It should be possible with my change. Please let me know if my change could cause problems though.


This addresses bug ASTERISK-20414.
    https://issues.asterisk.org/jira/browse/ASTERISK-20414


Diffs (updated)
-----

  /branches/1.8/apps/app_dial.c 375414 
  /branches/1.8/apps/app_jack.c 375414 
  /branches/1.8/apps/app_meetme.c 375414 
  /branches/1.8/apps/app_queue.c 375414 
  /branches/1.8/apps/app_record.c 375414 
  /branches/1.8/apps/app_waitforring.c 375414 
  /branches/1.8/channels/chan_agent.c 375414 
  /branches/1.8/channels/chan_dahdi.c 375414 
  /branches/1.8/channels/chan_iax2.c 375414 
  /branches/1.8/channels/sig_analog.c 375414 
  /branches/1.8/channels/sig_pri.c 375414 
  /branches/1.8/include/asterisk/channel.h 375414 
  /branches/1.8/include/asterisk/time.h 375414 
  /branches/1.8/main/channel.c 375414 
  /branches/1.8/main/pbx.c 375414 
  /branches/1.8/main/rtp_engine.c 375414 
  /branches/1.8/main/utils.c 375414 
  /branches/1.8/res/res_fax.c 375414 

Diff: https://reviewboard.asterisk.org/r/2135/diff


Testing
-------

I've placed many test calls through items that used timeouts like Dial and Queue with a timeout. I've also ensured that other apps that use ast_waitfor() such as Answer() and Echo() still function properly.


Thanks,

Mark

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121030/16b2bfb1/attachment.htm>