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

jrose reviewboard at asterisk.org
Mon Apr 15 16:07:41 CDT 2013 

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

(Updated April 15, 2013, 9:07 p.m.)


Review request for Asterisk Developers, David Lee, kmoore, Matt Jordan, Mark Michelson, and rmudgett.


Changes
-------

Addresses Kinsey's review as well as some of my own findings. Also addresses some finding noted commented by Richard via email since reviewboard was acting up for him earlier. The contents of and responses to that review are as follows:

> features.h:
>
> line 195
> The changes to features.h for this prototype are moved to the
> bridging_basic.h header file. Also you will not need to call this
> function. Instead you would need to call ast_bridge_features_ds_set() and
> possibly ast_bridge_features_ds_get() if you need to read-modify-write.

HOLD since this relies on https://reviewboard.asterisk.org/r/2446
 

> parking.h:
> line 74
> What a long line. Ever hear of breaking long lines?

CHECK

> bridging.c:
> line 3140-3142
> This should not be done here. The clear roles should be moved into the
> pull and the establish roles should be moved into the push. The only
> thing you should worry about when pulling a channel from a bridge and
> pushing it into a new bridge is pointing the channel to the new bridge.
> Otherwise, this must be done by the merge as well.

CHECK
 

> parking.c:
> line 141
> This should be a WARNING at least since they tried to park a call and the
> module is not loaded.

I'm going to disagree on this one on the basis of precedent. This is similar to how
the voicemail installable functions work which also use verbose messages in this exact
same manner. See ast_app_has_voicemail in app.c and the functions below for examples.


> res_parking.c:
> line 51
> There is not much difference between struct parking_lot_state and struct
> parking_lot_cfg. This is not good because any new config options need to
> be added to both structs.

HOLD (Changing this is going to require a somewhat large scale effort. Since it isn't critical, this should wait until the initial merger is finished)
 

> line 70
> This struct member should just be parked_users and not state that the
> container is a list.

CHECK


> line 77
> This is the name of the parking lot. Describing it as a state name is
> misleading.

HOLD (This should be changed along with the change to store configuration within the parking lot objects rather than duplicating configuration. I also like it this way for the time being since searching for parking_lot_state is easier than searching for parking_lot when parking_lot_cfg structs are present in the file as well)


> line 213-235
> Since the container is sorted, this function should just be:
> return CMP_MATCH;
> The sort function above has already filtered the matches.

CHECK


> parking/parking_applications.c:
> line 238-254
> Find the parking lot name then find the parking lot. Your error messages
> will also benefit. Look at the current parking code.
> You have done this code differently in another place when there is no need.

CHECK

 
> line 263-380
> Same. Find parking lot name first then find parking log.

CHECK


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 (updated)
-----

  /team/group/bridge_construction/CHANGES 385818 
  /team/group/bridge_construction/bridges/bridge_builtin_features.c 385818 
  /team/group/bridge_construction/configs/res_parking.conf.sample PRE-CREATION 
  /team/group/bridge_construction/include/asterisk/bridging.h 385818 
  /team/group/bridge_construction/include/asterisk/config_options.h 385818 
  /team/group/bridge_construction/include/asterisk/features.h 385818 
  /team/group/bridge_construction/include/asterisk/parking.h PRE-CREATION 
  /team/group/bridge_construction/main/bridging.c 385818 
  /team/group/bridge_construction/main/bridging_roles.c 385818 
  /team/group/bridge_construction/main/config_options.c 385818 
  /team/group/bridge_construction/main/features.c 385818 
  /team/group/bridge_construction/main/parking.c PRE-CREATION 
  /team/group/bridge_construction/res/Makefile 385818 
  /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/20130415/9ae1d2bc/attachment-0001.htm>

More information about the asterisk-dev mailing list