[asterisk-dev] [Code Review] 2615: res_parking: Add dynamic parking back in

[Matt Jordan]

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



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

    If you're exposing the lock for the container, you might as well make the container itself global - which is a bad thing.
    
    I'd do this differently - rather than expose information that should be completely managed by the parking core, make the call to parking_create_dynamic_lot do two things:
    
    (1) Since this is the only place it is called, it should first check to see if the lot exists already and if so, return an error. That allows you to hide the call to parking_lot_find_by_name behind parking_create_dynamic_lot
    
    (2) Have parking_create_dynamic_lot lock the container itself



/trunk/res/res_parking.c
<https://reviewboard.asterisk.org/r/2615/#comment17654>

    You can probably just return cfg->global->parkeddynamic here



/trunk/res/res_parking.c
<https://reviewboard.asterisk.org/r/2615/#comment17655>

    These errors don't actually say what parameter was in error, but instead try to provide help. I'd rather we try to tell the user exactly what went wrong, rather than try to tell them how to fix it, i.e., be prescriptive rather than descriptive.
    
    Some suggestions:
    
    "Invalid parking range %s specified in PARKINGDYNPOS: could not parse minimum/maximum slot range", dyn_range
    
    For your second, I'd break it up and check each independently so that you can provide error messages that match the error.
    
    If dyn_end < dyn_start:
    
    "Invalid parking range %s specified for PARKINGDYNPOS: end parking slot must be greater than starting parking slot"
    
    etc.


- Matt Jordan


On June 21, 2013, 4:27 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2615/
> -----------------------------------------------------------
> 
> (Updated June 21, 2013, 4:27 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21644
>     https://issues.asterisk.org/jira/browse/ASTERISK-21644
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add Dynamic Parking Lots.
> 
> 1. If parkeddynamic is enabled and a call is being parked into a parking lot which doesn't already exist (for instance, the name of the parking lot is specified in the application arguments or the channel parking lot variables) then a new parking lot will be created based on an existing parking lot specified by the channel variable PARKINGDYNAMIC. PARKINGDYNCONTEXT, PARKINGDYNEXTEN, and PARKINGDYNPOS will change specific configuration values for the newly created parking lot.
> 
> 2. Dynamic parking lots will stick around after they are created. This behavior is consistent with how they were implemented in 11. A small change to this however comes with reloads. On reload, if a dynamic parking lot exists and currently contains no calls then it will be removed. If it has calls in it however, then the parking lot will remain and still function normally.
> 
> As a side note, I changed the parking lot container to use a mutex lock instead of an RWLock. I wasn't using the Read/write aspect of the lock for anything at the time and it was causing problems for some reason.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_parking.c 392115 
>   /trunk/res/parking/res_parking.h 392115 
>   /trunk/res/parking/parking_ui.c 392115 
>   /trunk/res/parking/parking_applications.c 392115 
>   /trunk/CHANGES 392115 
> 
> Diff: https://reviewboard.asterisk.org/r/2615/diff/
> 
> 
> Testing
> -------
> 
> Created dynamic parking lots, reloaded with calls in them and without calls in them, parked multiple people in them at a time, made sure they would fail to be created if their extensions overlapped, made sure they cleared out extensions that they register appropriately on destruction. 
> 
> 
> Thanks,
> 
> jrose
> 
>

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