[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
Tue Apr 30 12:08:56 CDT 2013
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/res_parking.h, lines 59-71
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36526#file36526line59>
> >
> > This should be moved after struct parking_lot_cfg. I'm surprised you are not getting a compiler warning for undefined struct.
Yeah, I'm not sure why that wasn't giving a warning either. Rather strange.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/res_parking.c, line 278
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36527#file36527line278>
> >
> > OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY
static int parking_lot_cfg_hash_fn(const void *obj, const int flags)
{
const struct parking_lot_cfg *entry;
const char *key;
switch (flags & (OBJ_POINTER | OBJ_KEY | OBJ_PARTIAL_KEY)) {
case OBJ_POINTER:
entry = obj;
return ast_str_hash(entry->name);
default:
key = obj;
return ast_str_hash(key);
}
}
The only thing that perturbs me about this is that now it will hit the default if it is none of those things. Of course, that should never be the case.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_controller.c, lines 216-219
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36523#file36523line216>
> >
> > Only enable one recording method not both. Previously, parking only enabled AST_FEATURE_AUTOMON.
I'll go with AUTOMIXMON for now since that's what most new bridging stuff uses to record with. It might be appropriate to add an option for that later on, but that'd be scope-creepy right now.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_controller.c, line 111
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36523#file36523line111>
> >
> > Just delete this comment. It is wrong.
It's poorly worded. I've changed it to:
/* Increment the wrap on each pass until we find an empty space */
I think it's good to have some clarification of what's going on here since the wrap value isn't self explanatory.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_controller.c, line 94
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36523#file36523line94>
> >
> > ...has already has...
Changed to 'lot already has next_space set'
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_bridge_features.c, line 345
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36522#file36522line345>
> >
> > Always return 0 here or you will remove the one-touch parking hook.
That was actually somewhat my intention for when a few of the more elaborate cancellation conditions happen in park_feature_helper (assertion failure, failure to subscribe to a stasis topic which should always exist), but I suppose it isn't really harmful to leave the feature in place.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_bridge_features.c, lines 325-328
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36522#file36522line325>
> >
> > Should this not also publish_parked_call_failure() ?
Nah, it should.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_bridge_features.c, lines 169-171
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36522#file36522line169>
> >
> > Unexpected code binding. The if binds with datastore->data = parked_datastore.
This is something that seems to have gotten lost. I'm not really sure what happened here. I'm guessing this would have caused subscription leaks.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_applications.c, line 449
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36520#file36520line449>
> >
> > aprking
pinkies!
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_applications.c, line 388
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36520#file36520line388>
> >
> > bleh?
leftover madness.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_applications.c, line 389
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36520#file36520line389>
> >
> > This should be silent if the silent option or BLINDTRANSFER channel variable is present.
> >
> > Also should publish_parked_call_failure().
publish_parked_call_failure is strictly for blind transfers to parking lots so that the channel that did the parking will know when to exit the bridge. That doesn't apply if the actual application for parking is executed because the channel won't be around anymore if this was entered on blind transfer and if it is entered on attended transfer, the attended transferer will hang itself up anyway.
But yeah, I can see the use for silence and BLINDTRANSFER stuff. I'll need to rework park common so that I can get the silence out of there after all.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/res/parking/parking_applications.c, line 355
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36520#file36520line355>
> >
> > Is this still valid?
Yes, at least partially. The BLINDTRANSFER variable is currently only respected by the Parking application for the purpose of comeback goto stuff and not for stasis message generation. I've updated this comment to reflect that.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/main/parking.c, lines 161-162
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36518#file36518line161>
> >
> > This should be a WARNING message.
I pointed this out in another review, but that's not how we do things with any of the installable functions in app.c and I don't see any compelling reason for this to be different.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/main/features.c, lines 7065-7066
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36517#file36517line7065>
> >
> > I don't think a blank CLI output line is needed here.
It would leave a blank space with the parking spaces included. It makes sense to keep that blank space.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/include/asterisk/features.h, line 29
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36512#file36512line29>
> >
> > Is this include necessary here? The header does not have any other changes.
I'm not sure when or why that got added.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/CHANGES, lines 172-173
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36507#file36507line172>
> >
> > Is the deprecated command aliased to the new command?
No, should it be? If so, we can add it to the to-do list. It shouldn't block this review though.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/CHANGES, lines 143-145
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36507#file36507line143>
> >
> > 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.
David suggested I shouldn't do this sort of thing in the CHANGELOG. We'll leave this finding open for now and we can add it to the to-do list.
I'm not sure if there is any intention to merge the two configs. I'm still more leaning towards the presence of res_parking.conf causing features.conf parking lots to be ignored. But that isn't a large priority to address right now.
> On April 29, 2013, 11:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/main/parking.c, lines 43-49
> > <https://reviewboard.asterisk.org/r/2447/diff/3/?file=36518#file36518line43>
> >
> > 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.
Alrighty, I think I got this right.
- jrose
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2447/#review8392
-----------------------------------------------------------
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/20130430/71480f20/attachment-0001.htm>
More information about the asterisk-dev
mailing list