[asterisk-dev] [Code Review] 2543: Gulp blind and attended transfer support

Thu May 16 16:43:42 CDT 2013

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

In general, there is a huge lack of logging in this review (one count of ast_log that I could find). Transfers are always destined to be the place where bugs will occur most. Therefore, this should be absolutely riddled with log messages. Way more than you think you should need. We'll be glad they're there once people start reporting transfer bugs.

/team/group/bridge_construction/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2543/#comment16866>

    PJSIP documentation stipulates that the string passed into pjsip_parse_uri() MUST be null-terminated. So use pj_strdup2_with_null() instead of pj_strdup2().

/team/group/bridge_construction/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2543/#comment16868>

    Did you ever run any tests where you called the Ringing() application and then the Transfer() application?

    I suspect that if you did that, you might end up with a case where you end up sending a 302 with two Contact headers. The reason is that pjsip_inv_end_session() will reuse the latest request/response that was sent. If the previous response sent had a Contact header, then I suspect that adding a new one will just tack on a second one.

    I suggest erasing any Contact header already present in the packet before adding the new Contact header to the response.

/team/group/bridge_construction/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2543/#comment16888>

    So here's a painful case.

    What happens if we are communicating with a buggy client that does not send us a BYE when a transfer completes?

    I'm pretty sure that PJSIP will eventually time out the implicit REFER subscription (based on some default timeout), but will the INVITE dialog ever terminate? I know that if session timers are enabled, it will, but what if session timers aren't enabled?

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16887>

    This has some flaws. notification->state is the state of the subscription when this task was queued, not the actual current state. So just because notification->state is not TERMINATED, it doesn't guarantee that the synchronous task from the state change callback isn't currently running. Thus the deadlock can still occur.

    Second, the deadlock described isn't necessarily going to occur even if the TERMINATED state is accurately detected because this callback can run in either the refer_progress's serializer or a session's serializer.

    So let's think through this a bit.

    Right now, a refer_progress gets its own serializer allocated. This is used for sending progress notifications and for setting the progress's subscription NULL when the subscription is terminated.

    Well, consider this. What if the refer_progress just used a reference to the session serializer from the session on which the REFER was received? The progress notifications are sent in the same dialog, so it makes sense to do this.

    This way, the evsub state change to TERMINATED will happen in one of two cases

    1) An incoming BYE terminates the subscription
    2) The REFER subscription times out

    In case 1) The evsub state change callback will run in the session serializer. Since this is the same serializer where event notifications are sent, you won't have to push a synchronous task to set the progress->sub NULL. You can just set it in place.

    For case 2, I think you could just push an asynchronous task that does everything that your state change callback does now, to include setting progress->sub NULL.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16886>

    Combine these two ifs with &&

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16870>

    This return point causes a return value of NULL to be ambiguous.

    Callers of refer_progress_alloc() assume that a NULL return means that progress was not requested (i.e. no error), but it can also indicate that a failure occurred when attempting to allocate the progress structure (i.e. an error).

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16872>

    Since these messages are sent in the session's dialog, why not use the session's serializer?

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16871>

    May as well define this string at the top of the function and replace the previous pj_strnicmp2 with a call to pj_strnicmp using this constant string.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16885>

    This use of "transferee" is non-standard and liable to cause confusion. This session is actually also the transferer, but it is bridged with the transfer target rather than the transferee.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16880>

    Just a word of warning: Due to review feedback in an open review of mine, this callback will have an extra parameter to it which tells if the transfer is single-party or multi-party.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16883>

    +1 to sending remote attended transfers to a common extension.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16881>

    I think the INVALID case should get a 400 response instead of a 500. They tried an invalid operation.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16882>

    Coding guidelines say to align the case statements with the switch.

    i.e.

    switch(foo) {
    case bar:
        do_stuff();
        break;
    case baz:
        do_different_stuff();
        break;
    }

    This is done other places in this review as well.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16877>

    This comment seems out of place since it isn't by a synchronous task and we are operating in the session's serializer here.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16879>

    ast_channel_move() and ast_bridge_impart() can fail.

/team/group/bridge_construction/res/res_sip_refer.c
<https://reviewboard.asterisk.org/r/2543/#comment16869>

    Rather than using channel state here, I'd suggest using session->inv_session->state. Only add the Replaces on PJSIP_INV_STATE_CALLING.

    This way, you won't add Replaces headers on reinvites that we send.

- Mark Michelson

On May 15, 2013, 6:32 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2543/
> -----------------------------------------------------------
> 
> (Updated May 15, 2013, 6:32 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> These changes add support for the following:
> 
> 1. Blind transfers originating from a remote device
> 2. Blind transfers originated to a remote device
> 3. Redirection originated to a remote device (before session has been answered)
> 4. Attended transfers originating from a remote device when both legs are local
> 5. Attended transfers originating from a remote device when only one leg is local (additional dialplan logic required)
> 6. Replacement of a session using INVITE with replaces
> 7. Transfer progress notification using a frame hook
> 8. norefersub support
> 
> * This patch relies on pimp my SIP, bridge_construction, and more_transfer.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/res/res_sip_refer.c PRE-CREATION 
>   /team/group/bridge_construction/include/asterisk/res_sip_session.h 388524 
>   /team/group/bridge_construction/channels/chan_gulp.c 388524 
>   /team/group/bridge_construction/res/res_sip_session.c 388524 
>   /team/group/bridge_construction/res/res_sip_session.exports.in 388524 
> 
> Diff: https://reviewboard.asterisk.org/r/2543/diff/
> 
> 
> Testing
> -------
> 
> Testing the various scenarios using SIPP and phones.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130516/6f9e51d0/attachment-0001.htm>