[asterisk-dev] [Code Review] 2638: res_parking: Gut parker snapshots and replace them with ParkerDialString

[Matt Jordan]

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



/trunk/include/asterisk/parking.h
<https://reviewboard.asterisk.org/r/2638/#comment17666>

    I wouldn't make this all one word - parker_dial_string is more in line with our preferred variable naming guidelines



/trunk/include/asterisk/parking.h
<https://reviewboard.asterisk.org/r/2638/#comment17667>

    The parameter is parker_dial_string, not parker_dialstring



/trunk/main/cel.c
<https://reviewboard.asterisk.org/r/2638/#comment17668>

    This test has no purpose.
    
    "If a is not NULL pass a otherwise pass NULL" is equivalent to just passing a.



/trunk/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2638/#comment17669>

    While this is probably the only way to handle determining the dial string from two possible and competing locations, I'd prefer to see a bit of logging here.
    
    If, for whatever reason, someone sets the ATTENDEDTRANSFER channel variable but expects that the BLINDTRANSFER variable to be used, there is nothing telling them that we prefer the ATTENDEDTRANSFER variable over the BLINDTRANSFER variable.
    
    I'd change this to do the following (in pseudocode):
    
    lock channel
    attended_transfer = pbx_getvar(ATTENDEDTRANSFER);
    blind_transfer = pbx_getvar(BLINDTRANSFER);
    unlock channel
    
    if (attended_transfer) {
      ast_verb(5, "Setting dial string to %s from ATTENDEDTRANSFER value", attended_transfer)
      dial_string = attended_transfer;
    } else if (blind_transfer) {
      ast_verb(5, "Setting dial string to %s from BLINDTRANSFER value", blind_transfer);
      dial_string = blind_transfer;
    }
    
    ...
    
    I'd make sure that we do log it out at a very high verbose level (so that people watching dialplan execution will see it), and that it does tell someone which variable it used to construct the dial string to callback when a channel times out of parking.
    



/trunk/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2638/#comment17670>

    Looking at this now, this all feels rather dangerous.
    
    ast_strdup is allocating memory on the heap. We don't check that these allocations don't fail, and we don't return an error when they do. This means consumers of this functions aren't provided a success/failure, and they don't know if each of these are pointing to valid memory, pointing to garbage, or pointing to NULL.
    
    Currently, because we don't return on error, they *should* all be NULL or valid when done, but that still means that callers have to check each value themselves. This feels like a lot of off-nominal conditions that could be handled better.
    
    This should return a configuration struct containing the items from the datastore. That struct should be allocated by this function, and this function should either return NULL on error, or the configuration struct on success. Consumers of the function should free the memory allocated by this function when they are done with the configuration information.
    
    



/trunk/res/parking/res_parking.h
<https://reviewboard.asterisk.org/r/2638/#comment17671>

    This should actually be in channel.h


- Matt Jordan


On June 20, 2013, 9:44 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2638/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 9:44 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21877
>     https://issues.asterisk.org/jira/browse/ASTERISK-21877
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> We decided parker snapshots were misleading and unuseful for all but a few uses of parked call messages and events. As such, they've been replaced with just having parker dial strings for handling call control on timeout.
> 
> If a channel directly parks another channel, then setting the park dial string is fairly trivial.  We just use the copy the name of the channel that parked the call and then flatten it while creating the parked user. If the call was transferred to the park application somehow, then it must be read from either the BLINDTRANSFER variable or from the newly added ATTENDEDTRANSFER variable which is basically just the BLINDTRANSFER variable for attended transfers.
> 
> This effectively wipes out one of the bug bug comments mentioned in ASTERISK-21877 regarding not having a snapshot of who parked the call on so-called 'deep parks' where a park application is deeper than the first priority of the extension a call is transferred to. The other bug bug comment remains at large, but mmichelson has a patch for bridge roles in the works which should make fixing that trivial.
> 
> 
> Diffs
> -----
> 
>   /trunk/bridges/bridge_builtin_features.c 392357 
>   /trunk/include/asterisk/parking.h 392357 
>   /trunk/main/bridging.c 392357 
>   /trunk/main/cel.c 392357 
>   /trunk/main/features.c 392357 
>   /trunk/main/parking.c 392357 
>   /trunk/res/parking/parking_applications.c 392357 
>   /trunk/res/parking/parking_bridge.c 392357 
>   /trunk/res/parking/parking_bridge_features.c 392357 
>   /trunk/res/parking/parking_controller.c 392357 
>   /trunk/res/parking/parking_manager.c 392357 
>   /trunk/res/parking/parking_ui.c 392357 
>   /trunk/res/parking/res_parking.h 392357 
> 
> Diff: https://reviewboard.asterisk.org/r/2638/diff/
> 
> 
> Testing
> -------
> 
> All kinds of different transfers (DTMF blind, DTMF attended, SIP blind, SIP attended, SIP attended with hangups before park, etc).
> 
> The only noteworthy case where the parker was anything other than who transferred the call was the following:
> 
> SIP/A calls SIP/B
> SIP/A performs attended transfer to extension f
> 
> f,1,NoOp
> f,2,Wait(10)
> f,3,Park()
> 
> SIP/A hangs up during the wait
> 
> This causes the park dial string for SIP/B's parked user to be SIP/B unfortunately, but this is more of a transfer related issue than a parking related issue.  It's also one that Richard says was also the case in previous versions, so this isn't a change in behavior.
> 
> This could be addressed by setting the ATTENDEDTRANSFER variable when the masquerade occurs to put B in the PBX where A leaves off.
> 
> 
> Thanks,
> 
> jrose
> 
>

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