[asterisk-dev] [Code Review] 2545: res_parking: Generate extensions when parkext is set for a parking lot. Generate extension to dial parker on parked call timeout.

Tue May 21 17:31:47 CDT 2013

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, lines 630-636
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line630>
> >
> >     remove_parking_lot_extensions will remove all contexts registered by the parking lot, including their extensions.
> >     
> >     How is their user going to configure their own extensions if you've destroyed all contexts that they will reside in?

This function will only destroy functions registered by the parking lot's registrar. Functions created by pbx configuration aren't hit by it. I blame the poor function name and general lack of documentation from ast_context_destroy() for the misunderstanding.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, line 631
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line631>
> >
> >     This will blow away all contexts registered by this lot's registrar. If a registrar has multiple lots, this will destroy all prior lots created.
> >     
> >     So, if res_parking has two parking lots defined for it, creation of the second parking lot will remove the first.
> >     
> >     You should remove only the lot being created.

ast_context_destroy doesn't actually destroy contexts when the registrar is provided. It just destroys extensions in the specified context (or all contexts if context is null) belonging to that registrar.

It's actually somewhat annoying because it means we are leaving vestigial contexts around.  It doesn't even destroy the context if it becomes empty as a result of removing the extensions.  But this is probably something that should be fixed in pbx.c rather than worked around here (it's also a mostly harmless behavior).

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/parking/parking_bridge_features.c, lines 421-427
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37979#file37979line421>
> >
> >     You changed the PARKER variable here. Previously it was the flattened peername; now it's the trimmed version.
> >     
> >     Since this was missed here, you may be better off introducing your own trimmed variable as opposed to repurposing an existing one.

No, this is something I implemented incorrectly before. The PARKER variable was always meant to be just the trimmed peername and the flattened peername is only used to generate the extension and set where we go when comebacktoorigin is on.

The flattened peername variable is useful to people running their dial plan though because without it they'll have to do string manipulation to find the park-dial extension that gets created. But it's actually the flattened peername that will be the new value.

I'll make this a new channel variable and for now I'll call it PARKER_FLAT for lack of a better term.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, lines 722-724
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line722>
> >
> >     create_parking_lot_extensions has multiple paths where it fails (for example, if the context can't be created). In such a case, it feels odd to let the lot succeed. We should probably allow that function to return success/failure, and, if it fails, destroy the lot.
> >     
> >     Also, you should probably create the extensions before linking it into the parking lot container. Otherwise, entities can technically look up the parking lot but won't have the dialplan support they may be epecting.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, lines 678-679
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line678>
> >
> >     This can fail. Check for failure and clean up appropriately if it does.
> >     
> >     If you configure Asterisk to add hints and it can't do so, that should be treated as an error.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, lines 665-670
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line665>
> >
> >     Same here. This feels like a serious error.

Yeah, these are pretty bad. Sorry, this construction came from features.c and I didn't think about it as much as I should have.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/parking/parking_bridge_features.c, lines 375-384
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37979#file37979line375>
> >
> >     I wouldn't use the term peer. "extra" also doesn't provide much context to what you're removing here. The first thing I thought of was "What, SIP?"
> >     
> >     You're trimming the random characters off of the parker's channel name - I'd name it to reflect that purpose (I'm also surprised we don't have a general utility function for this already)

For now I'll call it:
channel_name_to_dial_string

If you think I should move it into channel.h or something, let me know. As far as I know there is no other implementation of this so far.

> On May 17, 2013, 11:56 a.m., Matt Jordan wrote:
> > /team/jrose/bridge_projects/res/res_parking.c, lines 617-620
> > <https://reviewboard.asterisk.org/r/2545/diff/1/?file=37982#file37982line617>
> >
> >     I'm not sure you're getting a lot by having this be a separate function. It only wraps ast_context_destroy, and as other findings point out, sometimes you don't want to remove all contexts associated with a lot's registrar.

It does a little more now that I've changed everything.

- jrose

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

On May 15, 2013, 7:53 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2545/
> -----------------------------------------------------------
> 
> (Updated May 15, 2013, 7:53 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21645
>     https://issues.asterisk.org/jira/browse/ASTERISK-21645
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Extension generation and removal for:
> Park a call to the parkext in the parking context
> Retrieve parked calls from the space numbers in the parking context
> Hints for ParkedCall extensions
> 
> Extension generation (but no removal yet) for:
> Dial the peer which parked the call on parking timeout in the park-dial context
> 
> Extension removal for the park-dial extensions will eventually be handled when reloads are implemented. Since they can't reliably be said to belong to
> the parking lot, they are registered to the base registrar for the parking system instead.
> 
> Applying bridge features in the way features.c did (as arguments to dial) is no longer necessary on account of how features are handled now.
> 
> Each parking lot has its own registrar which is 'res_parking/<parking lot name>'
> The registrar used for non-lot-specific extensions is just 'res_parking'
> 
> When a parking lot is destroyed or reloaded, all of its registered extensions are flushed and rebuilt.
> 
> Note: ParkedCall extensions no longer belong to the parked user involved with them unlike how old parking works. This means that if a parking lot is reloaded and the new configuration specifies that the slot the old call was parked at no longer exists, the extension to retrieve that call will go away even though the call is still parked there. This will resolve itself when the parked call times out (provided timeout hasn't been disabled), and manually created ParkedCall extensions can still retrieve that call if the parking space option isn't provided.
> 
> 
> Diffs
> -----
> 
>   /team/jrose/bridge_projects/res/parking/parking_bridge_features.c 388603 
>   /team/jrose/bridge_projects/res/parking/parking_controller.c 388603 
>   /team/jrose/bridge_projects/res/parking/res_parking.h 388603 
>   /team/jrose/bridge_projects/res/res_parking.c 388603 
> 
> Diff: https://reviewboard.asterisk.org/r/2545/diff/
> 
> 
> Testing
> -------
> 
> Checked that extensions are generated and destroyed for Park and ParkedCall as expected with multiple parking lots.
> Overlaps are currently allowed. If two parking lots occupy the same space and parkext exclusive is in use, it's possible for one parking lot's extensions to be overwritten in favor of another. When parking lot extensions are overwritten by other parking lot extensions, warnings are issued.
> Checked that extensions are generated and appropriately called in park-dial when a parked call times out.
> Checked that the timeout is set to the parking lot's parkdialtimeout value.
> 
> 
> Thanks,
> 
> jrose
> 
>

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