[asterisk-dev] [Code Review] timerfd timing interface

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/52/#review146
-----------------------------------------------------------

Ship it!


All of my comments are minor, so I don't feel that I need to look at this again.  Commit away.  Nice work.

On a related note, I think we should add a section to the README for Asterisk, or somewhere obvious, that describes the situation with the timing modules.  We should list each one, and some basic guidelines for people to know which one they should use.


/trunk/configure.ac
<http://reviewboard.digium.com/r/52/#comment266>

    As we discussed via email, you're missing AST_EXT_LIB_SETUP() in this file, but I think you already committed that fix.



/trunk/res/res_timing_timerfd.c
<http://reviewboard.digium.com/r/52/#comment267>

    I would change this to:
    
    continuous_timer = {
        .it_value.tv_nsec = 1L,
    };
    
    Then, you can remove the memset() from later in the function.



/trunk/res/res_timing_timerfd.c
<http://reviewboard.digium.com/r/52/#comment268>

    You have trailing whitespace around the patch



/trunk/res/res_timing_timerfd.c
<http://reviewboard.digium.com/r/52/#comment269>

    For some bizarre reason, your module was included in this diff multiple times.  I'm curious how this happened ...


- Russell


On 2008-11-12 17:07:34, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/52/
> -----------------------------------------------------------
> 
> (Updated 2008-11-12 17:07:34)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Kevin Fleming.
> 
> 
> Summary
> -------
> 
> Starting with Linux Kernel version 2.6.25 and glibc version 2.8, there is a timing interface known as timerfd. This review request encompasses an Asterisk timing implementation using the timerfd interface.
> 
> Of particular note in this review request are all things related to menuselect and autoconf. If there's anything there that seems wrong or out of place, please don't hesitate to yell about it. Also, I'm not sure what all the changes to ast_expr2.[hc] are about, but if I've done something wrong with regards to those, please let me know how I can fix those too.
> 
> 
> Diffs
> -----
> 
>   /trunk/build_tools/menuselect-deps.in 156168 
>   /trunk/configure.ac 156168 
>   /trunk/include/asterisk/autoconfig.h.in 156168 
>   /trunk/makeopts.in 156168 
>   /trunk/res/res_timing_timerfd.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/52/diff
> 
> 
> Testing
> -------
> 
> I have tested this by using the "timing test" CLI command and by doing some file playback.
> 
> 
> Thanks,
> 
> Mark
> 
>