[asterisk-dev] [Code Review] 3438: Implement SIP TImer C in Asterisk

[Matt Jordan]

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


I'm sure I'm missing something obvious, but in what scenario do we forward a request in a manner consistent with a proxy? I'm thinking of those scenarios where an inbound INVITE request is received by Asterisk, and something in the dialplan causes chan_sip to forward the INVITE request to something outside of Asterisk.


/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3438/#comment21354>

    So I'll preface this finding with the fact that auto_congest uses this same exact pattern - so it should work as is. In Asterisk 1.8, however, there is a slightly better way to write this that doesn't rely on deadlock avoidance code.
    
    There's a method now to get the channel safely:
    
    static struct ast_channel *sip_pvt_lock_full(struct sip_pvt *pvt);
    
    It will lock the pvt and return the channel owner of the pvt (a) locked, (b) reference bumped, and (c) with the pvt locked. The benefit here is that the channel is safely ref bumped, which prevents any lifetime issues from creeping into the mix. It always locks the pvt/channel in the correct order to avoid lock inversions. It also doesn't have to perform deadlock avoidance, which means that it will always return the channel associated with the pvt (so long as it existed).
    
    Combined with Corey's finding, I'd write this instead as:
    
    struct sip_pvt *p = (struct sip_pvt *)arg;
    struct ast_channel *chan;
    
    chan = sip_pvt_lock_full(p);
    
    if (p->timercid == -1) {
        ast_channel_unlock(chan);
        ast_channel_unref(chan);
        sip_pvt_unlock(p);
        return 0;
    }
    
    if (chan) {
        append_history(...);
        ast_queue_control(chan, AST_CONTROL_CONGESTION);
        ast_channel_unlock(chan);
        ast_channel_unref(chan);
    
       sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
    }
    
    sip_pvt_unlock(p);
    dialog_unref(p, ...);



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3438/#comment21356>

    Use { } on the single line if statements (even if the code above it egregiously disregards coding guidelines as well :-) )



/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/3438/#comment21357>

    I'd say add a note to CHANGES as well.


- Matt Jordan


On April 11, 2014, 3:41 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3438/
> -----------------------------------------------------------
> 
> (Updated April 11, 2014, 3:41 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> SIP Timer C is defined for proxys that forward messages. In some ways, we forward calls. It is activated when we receive a 100 trying and wait for any other message. If that's not received, timer C triggers and cancels the call attempt.
> 
> This is required in an interoperability test I'm working with.
> 
> Red dots will be handled in the way they deserve.
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/sip.conf.sample 412166 
>   /trunk/channels/sip/include/sip.h 412166 
>   /trunk/channels/chan_sip.c 412166 
> 
> Diff: https://reviewboard.asterisk.org/r/3438/diff/
> 
> 
> Testing
> -------
> 
> Passed interoperability testing with funky test tool.
> 
> 
> Thanks,
> 
> Olle E Johansson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140413/04023a5b/attachment-0001.html>