[asterisk-dev] [Code Review] 2447: res_parking: Rebuilding parking from the ground up to work with the new bridging model

rmudgett reviewboard at asterisk.org
Mon Apr 29 18:45:30 CDT 2013 

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



/team/group/bridge_construction/CHANGES
<https://reviewboard.asterisk.org/r/2447/#comment16143>

    '(parker)' or '(parkee)'
    'Parker' or 'Parkee'



/team/group/bridge_construction/CHANGES
<https://reviewboard.asterisk.org/r/2447/#comment16144>

    You probably should put a BUGBUG note here about the features.conf support.  It is expected that the features.conf support is to be restored so res_parking will merge the parking config from features.conf with res_parking.conf.



/team/group/bridge_construction/CHANGES
<https://reviewboard.asterisk.org/r/2447/#comment16145>

    Is the deprecated command aliased to the new command?



/team/group/bridge_construction/include/asterisk/features.h
<https://reviewboard.asterisk.org/r/2447/#comment16148>

    Is this include necessary here?  The header does not have any other changes.



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2447/#comment16151>

    In keeping with the opposite order of construction, the ast_bridge_channel_clear_roles() call should happen after the pull method call.



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2447/#comment16149>

    Move the call to ast_bridge_channel_establish_roles() right before the push method so the cheap tests can be done first.



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2447/#comment16150>

    Extraneous blank line.



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2447/#comment16155>

    Break long lines around 90 columns. :)



/team/group/bridge_construction/main/features.c
<https://reviewboard.asterisk.org/r/2447/#comment16156>

    Extraneous blank line



/team/group/bridge_construction/main/features.c
<https://reviewboard.asterisk.org/r/2447/#comment16159>

    I don't think a blank CLI output line is needed here.



/team/group/bridge_construction/main/features.c
<https://reviewboard.asterisk.org/r/2447/#comment16161>

    Extraneous blank line



/team/group/bridge_construction/main/parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16147>

    It would be better to create a function pointer typedef so there is less repetition of the function signature scattered around.  It would also make the register/unregister function prototypes easier to see what is passed.



/team/group/bridge_construction/main/parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16146>

    This should be a WARNING message.



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16152>

    aprking



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16183>

    Is this still valid?



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16182>

    bleh?



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16184>

    This should be silent if the silent option or BLINDTRANSFER channel variable is present.
    
    Also should publish_parked_call_failure().



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16180>

    aprking



/team/group/bridge_construction/res/parking/parking_applications.c
<https://reviewboard.asterisk.org/r/2447/#comment16181>

    Need to add ast_bridge_destroy(retrieval_bridge).  Probably should get as many failure cases out of the way before actually moving the parked call out of the parking lot.



/team/group/bridge_construction/res/parking/parking_bridge.c
<https://reviewboard.asterisk.org/r/2447/#comment16178>

    Should do this after breaking the lot links.



/team/group/bridge_construction/res/parking/parking_bridge.c
<https://reviewboard.asterisk.org/r/2447/#comment16172>

    For completeness, you should call the base push method.



/team/group/bridge_construction/res/parking/parking_bridge.c
<https://reviewboard.asterisk.org/r/2447/#comment16171>

    You should lock the swap bridge_channel.



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16187>

    Unexpected code binding.  The if binds with datastore->data = parked_datastore.



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16188>

    Channel must be locked when calling ast_channel_datastore_add().



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16190>

    Remove this comment.



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16191>

    Make this a BUGBUG comment.  Creating a ;1-;2 channel pair is what I will be working on soon.



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16185>

    Should this not also publish_parked_call_failure() ?



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16154>

    You need to be careful when getting the bridge_channel->bridge pointer.  That pointer can change because of bridge merges and moves.  It is only guaranteed to not change when the bridge or bridge_channel is locked.



/team/group/bridge_construction/res/parking/parking_bridge_features.c
<https://reviewboard.asterisk.org/r/2447/#comment16186>

    Always return 0 here or you will remove the one-touch parking hook.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16173>

    ...has already has...



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16176>

    Delete this comment.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16174>

    Just delete this comment.  It is wrong.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16175>

    Remove the second sentence in this comment.  It is just restating the next two lines.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16177>

    Delete this function.  Passing in NULL for the matching function matches all.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16189>

    Either the callers of this function must lock chan or this function must lock chan around the ast_bridge_features_ds_get() and ast_bridge_features_ds_set().



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16170>

    Only enable one recording method not both.  Previously, parking only enabled AST_FEATURE_AUTOMON.



/team/group/bridge_construction/res/parking/parking_controller.c
<https://reviewboard.asterisk.org/r/2447/#comment16193>

    Function not used.



/team/group/bridge_construction/res/parking/parking_manager.c
<https://reviewboard.asterisk.org/r/2447/#comment16198>

    Callers of this function need to check for NULL return value.



/team/group/bridge_construction/res/parking/parking_manager.c
<https://reviewboard.asterisk.org/r/2447/#comment16197>

    Remove res and the code testing it.



/team/group/bridge_construction/res/parking/res_parking.h
<https://reviewboard.asterisk.org/r/2447/#comment16164>

    This should be moved after struct parking_lot_cfg.  I'm surprised you are not getting a compiler warning for undefined struct.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16163>

    This can be deleted as a NULL cmp function at container creation matches everything.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16167>

    This is not a list container.  It is a hash container with 37 buckets.  The name is wrong.  Suggest changing it to parking_lots.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16166>

    OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16165>

    This is an odd expression.
    if (lot->mode != PARKINGLOT_DISABLED) {
    }



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16153>

    This function can return NULL.  The two callers of this function put the return value into ast_strdupa() which cannot tolerate a NULL.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2447/#comment16179>

    I think you need to use ast_bridge_destroy() on the parking bridge to get rid of it.


- rmudgett


On April 26, 2013, 10:45 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2447/
> -----------------------------------------------------------
> 
> (Updated April 26, 2013, 10:45 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee, kmoore, Matt Jordan, Mark Michelson, and rmudgett.
> 
> 
> Bugs: ASTERISK-21059, ASTERISK-21272 and ASTERISK-21353
>     https://issues.asterisk.org/jira/browse/ASTERISK-21059
>     https://issues.asterisk.org/jira/browse/ASTERISK-21272
>     https://issues.asterisk.org/jira/browse/ASTERISK-21353
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> As a result of all the new bridging changes, certain applications such as parking, queues, confbridge, and a few others require significant reworking within Asterisk to make usable.
> Parking was less than salvagable, so I've rebuilt it from the ground up to use a lot of new stuff including config hooks, the new bridging architecture, stasis, and just generally to
> be an independent module with its own scope that doesn't get tangled into a giant morass within features.c
> 
> Parking configuration works somewhat differently now and extensions aren't necessarily automatically generated (it's optional and hasn't been implemented), but the intention with the final result is that if you were using parking before without too much additional dialplan manipulation, that should all just work once your parking lots are migrated to the new configuration file.
> 
> Currently supported:
> parking from the PBX with the Park application (all arguments are supported)
> parking within a call using the one touch parking feature
> parking within a call using DTMF blind transfers (attended transfers work, but it's basically just like they called the application itself).
> Picking up parked calls using the PBX
> Multiple parking lots
> All options available to parking lots are currently configurable. Some aren't currently doing anything, namely parkext and hints.
> 
> To do list:
> * Dialplan generation for parking lots with parkext (included 'Park' and 'ParkedCall' applications)
> * Dialplan generation for comebacktoorigin (park-dial extensions)
> * Hints
> * Implement 'Park' manager action
> * Dynamic parking lots and the default parking lot
> * Scraping the greasy remnants of parking out of features.c
> * CEL events
> * Unit tests and testsuite tests
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 386687 
>   /team/group/bridge_construction/bridges/bridge_builtin_features.c 386687 
>   /team/group/bridge_construction/configs/res_parking.conf.sample PRE-CREATION 
>   /team/group/bridge_construction/include/asterisk/bridging.h 386687 
>   /team/group/bridge_construction/include/asterisk/config_options.h 386687 
>   /team/group/bridge_construction/include/asterisk/features.h 386687 
>   /team/group/bridge_construction/include/asterisk/parking.h PRE-CREATION 
>   /team/group/bridge_construction/main/bridging.c 386687 
>   /team/group/bridge_construction/main/bridging_roles.c 386687 
>   /team/group/bridge_construction/main/config_options.c 386687 
>   /team/group/bridge_construction/main/features.c 386687 
>   /team/group/bridge_construction/main/parking.c PRE-CREATION 
>   /team/group/bridge_construction/res/Makefile 386687 
>   /team/group/bridge_construction/res/parking/parking_applications.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/parking_bridge.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/parking_bridge_features.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/parking_controller.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/parking_manager.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/parking_ui.c PRE-CREATION 
>   /team/group/bridge_construction/res/parking/res_parking.h PRE-CREATION 
>   /team/group/bridge_construction/res/res_parking.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2447/diff/
> 
> 
> Testing
> -------
> 
> This is currently being tested as it's developed and breaks in minor ways frequently. Unit tests and testsuite tests are on the to do list.
> 
> 
> Thanks,
> 
> jrose
> 
>

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

More information about the asterisk-dev mailing list