[Home]

Summary:ASTERISK-04388: [patch] replace hand-rolled timeval manipulation with generic functions.
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-06-10 03:49:11Date Closed:2008-01-15 15:41:48.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) timeval.20050713.diff
( 1) timeval.diff
Description:Many of the asterisk sources use hand-rolled code to manipulate
struct timevals, including computing differences, sums, conversion
from/to millisecond values etc.

Apart from being ugly and making the code a lot longer than it
should be, the chances of screwing up in some computations (especially
on negative values) are extremely high. In fact, in constructing this
patch i found 3 bugs and possibly a fourth one.

The attached patch introduces a number of function to manipulate
timevals, and replaces most handrolled instances with the new functions.

These are defined in utils.c and include/asterisk/utils.h (see below)

The rest of the patch applies the new methods to the code
as much as possible, though i might have missed some cases.
In any case, it is not necessary to apply all the chunks if you
don't feel confident about some.


These are derived from actual usage. Not sure if we need more.
ast_tv() is necessary as I don't think the compiler lets you construct
a struct on the fly while calling a function e.g.  foo( { sec, usec});

On passing, fixed some bug

frame.c
   dubious piece of code near line 144
   possible rounding artifacts near line 181

cdr/cdr_tds.c
   near line 327, probably there was a bug handling tv_usec=0 (fixed)

channels/chan_vpb.c
   replaced double (leading to rounding errors) with timeval

pbx/pbx_dundi.c
   has lots of busy-wait loops....


****** ADDITIONAL INFORMATION ******

The new methods are:

    struct timeval ast_tvnow()      /* returns the current timeofday */

       this is meant to replace all uses of gettimeofday(&foo, NULL)
       which sometimes are written with bogus error checking code,
       and often are just used to get a temporary value;

   struct timeval ast_tv(int _s, int _us) /* converts s,us to timeval */

       this is meant to construct a timeval from explicit values,
       e.g. when we want to initialize a variable, or we need to
       pass an argument constructed on the fly to a function.

   struct timeval ast_samp2tv(int _nsamp, int _rate)

       This returns the time taken by _nsamp samples at rate _rate.
       and is used in many places in asterisk when the code needs to
       convert from samples to time.
       It can also be used to convert a millisecond value into
       sec/usec (in this case use _rate = 1000)

   long ast_mstvdiff(struct timeval a, struct timeval b)

       computes the difference a-b in milliseconds.
       No overflow checks at the moment, as it was in the
       original code. It can be added later if needed.

   struct timeval ast_tvadd(struct timeval a, struct timeval b)
   struct timeval ast_tvsub(struct timeval a, struct timeval b)

       add or subtract two timevals, returning the result.
       It checks that the arguments are consistent, i.e.
       tv_usec is in the range 0..999999, and prints a
       warning (and fixes the value) otherwise.

   int ast_tvzero(struct timeval a)

       returns true if the timeval is 0.0

   int ast_tvcmp(struct timeval a, struct timeval b)

       compares a and b returning -1 if a<b, 0 if a==b, 1 if a>b
Comments:By: Clod Patry (junky) 2005-06-10 11:53:31

I just missed one part:
in the:
for(;;) {
 struct timeval started = ast_tvnow();
}

this gonna create a struct on each loop, no?

And i really like your ast_tvdiff and ast_tvzero ideas.

Really great job.
By: Luigi Rizzo (rizzo) 2005-06-10 13:06:14

junky: nothing is created - think of struct timeval as it it were an int.
ast_tvnow() returns a scalar value which is copied into variable 'started'.
By: Kevin P. Fleming (kpfleming) 2005-06-24 13:00:05

I'm in the process of reviewing this patch; I'm still not that comfortable with the copying-of-structs method vs. using pointers. Granted, on a 64-bit platform it's a wash. but on the most common platforms it's not. The code is certainly easier to read using this technique, though, and easier to write, so that's a big factor.

I'll continue hashing it over here so we can make a decision one way or the other.
By: Luigi Rizzo (rizzo) 2005-07-04 13:36:41

any progress with this one ?
By: Kevin P. Fleming (kpfleming) 2005-07-11 21:04:56

OK, the items that were holding up this bug have now been dealt with, so this one should be ready for a re-spin. I haven't yet gotten an architecture sign-off from Mark, but I'm about ready to go ahead with it anyway, since he's on vacation and I'd like to get these improvements into the tree :-)
By: Kevin P. Fleming (kpfleming) 2005-07-13 12:20:14

OK, we have an official acceptance from Mark on using this technique, so once you have a new patch for CVS HEAD I'll get it merged.
By: Luigi Rizzo (rizzo) 2005-07-13 17:50:10

just submitted a revised patch on a fresh cvs co tree.

The only change from the above description is that the function that computes a difference in milliseconds is now called ast_tvdiff_ms()
(it replaces a previous one that took arguments by reference - now they
are by name as it makes it simpler to build expressions).

It is possible that after this patch is applied, the time handling logic in
some files can be further simplified.
By: Kevin P. Fleming (kpfleming) 2005-07-15 18:53:24

Committed to CVS HEAD, with some minor changes so that it would actually compile. Fabulous work, thanks for sticking with this one :-)
By: Digium Subversion (svnbot) 2008-01-15 15:41:39.000-0600

Repository: asterisk
Revision: 6146

U   trunk/app.c
U   trunk/apps/app_alarmreceiver.c
U   trunk/apps/app_disa.c
U   trunk/apps/app_dumpchan.c
U   trunk/apps/app_forkcdr.c
U   trunk/apps/app_mp3.c
U   trunk/apps/app_nbscat.c
U   trunk/apps/app_readfile.c
U   trunk/apps/app_sayunixtime.c
U   trunk/apps/app_sms.c
U   trunk/apps/app_talkdetect.c
U   trunk/apps/app_test.c
U   trunk/apps/app_voicemail.c
U   trunk/asterisk.c
U   trunk/cdr/cdr_csv.c
U   trunk/cdr/cdr_tds.c
U   trunk/cdr.c
U   trunk/channel.c
U   trunk/channels/chan_agent.c
U   trunk/channels/chan_iax2.c
U   trunk/channels/chan_phone.c
U   trunk/channels/chan_sip.c
U   trunk/channels/chan_vpb.c
U   trunk/channels/chan_zap.c
U   trunk/cli.c
U   trunk/frame.c
U   trunk/funcs/func_strings.c
U   trunk/include/asterisk/sched.h
U   trunk/include/asterisk/time.h
U   trunk/manager.c
U   trunk/pbx/pbx_dundi.c
U   trunk/pbx/pbx_gtkconsole.c
U   trunk/res/res_agi.c
U   trunk/res/res_features.c
U   trunk/res/res_musiconhold.c
U   trunk/rtp.c
U   trunk/sched.c
U   trunk/translate.c
U   trunk/utils.c

------------------------------------------------------------------------
r6146 | kpfleming | 2008-01-15 15:41:39 -0600 (Tue, 15 Jan 2008) | 2 lines

add a library of timeval manipulation functions, and change a large number of usses to use the new functions (bug ASTERISK-4388)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6146
By: Digium Subversion (svnbot) 2008-01-15 15:41:48.000-0600

Repository: asterisk
Revision: 6156

U   trunk/channels/chan_vpb.c

------------------------------------------------------------------------
r6156 | bkramer | 2008-01-15 15:41:47 -0600 (Tue, 15 Jan 2008) | 2 lines

/ fixed typo bug introduced in fix for bug ASTERISK-4388

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6156