[asterisk-dev] [Code Review] Improve timing interface to remember which provider provided a timer

[Russell Bryant]

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

Ship it!


Pending testing and the one minor issue I found, this looks good to me.



/trunk/bridges/bridge_softmix.c
<http://reviewboard.digium.com/r/211/#comment1607>

    The changes here look fine.  These comments are actually for Josh, since it highlighted this bit of code.
    
    Josh, this is an interesting function.  I presume the point is to ensure that a timing interface is available before bridge_softmix gets used.  However, while this test verifies that a timing interface is available at this instant, it does not ensure that a timing interface will _continue_ to be available.
    
    All you need to do to remove this (admittedly theoretical, and highly unlikely) race condition, is not close this timer until the bridge is torn down.  Perhaps you could just add a void *tech_pvt to the ast_bridge struct.  It could be potentially useful for other things down the road, as well. 



/trunk/bridges/bridge_softmix.c
<http://reviewboard.digium.com/r/211/#comment1608>

    Again, actually a comment for Josh.  :-)
    
    Is there a reason you need to set the rate back to 0 here when you're about to close the timer anyway?



/trunk/main/channel.c
<http://reviewboard.digium.com/r/211/#comment1609>

    If a timer is not allocated, tmp->timingfd should be set to -1.  There is some code that uses that value to check to see if it is valid.  (ast_channel_set_fd() specifically, maybe other places ..)


- Russell


On 2009-03-26 18:46:08, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/211/
> -----------------------------------------------------------
> 
> (Updated 2009-03-26 18:46:08)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The ability to load/unload timing interfaces is nice, but it means that when a timer is allocated, it may come from provider A, but later provider B becomes the 'preferred' provider. If this happens, all timer API calls on the timer that was provided by provider A will actually be handed to provider B, which will say WTF and return an error.
> 
> This patch changes the timer API to include a pointer to the provider of the timer handle so that future operations on the timer will be forwarded to the proper provider.
> 
> 
> This addresses bug 14697.
>     http://bugs.digium.com/view.php?id=14697
> 
> 
> Diffs
> -----
> 
>   /trunk/bridges/bridge_softmix.c 184495 
>   /trunk/channels/chan_iax2.c 184495 
>   /trunk/include/asterisk/channel.h 184495 
>   /trunk/include/asterisk/timing.h 184495 
>   /trunk/main/channel.c 184495 
>   /trunk/main/timing.c 184495 
> 
> Diff: http://reviewboard.digium.com/r/211/diff
> 
> 
> Testing
> -------
> 
> Compile testing only.
> 
> 
> Thanks,
> 
> Kevin
> 
>