[Home]

Summary:ASTERISK-26029: parking: ast_parking_park_call should return parking_space instead of parking_exten
Reporter:Diederik de Groot (dkdegroot)Labels:
Date Opened:2016-05-17 06:37:11Date Closed:2016-05-20 15:41:33
Priority:MinorRegression?Yes
Status:Closed/CompleteComponents:Channels/chan_skinny Features/Parking
Versions:13.9.1 Frequency of
Occurrence
Constant
Related
Issues:
Environment:Attachments:
Description:Occording to the source documentation in:
include/asterisk/parking.h and res/parking/parking_bridge_features.c the call to 'parking_park_call' should return the extension where the call has been parked via the exten parameter. At the moment it is returning the 'parkext' instead, which is a bit useless. Asterisk-11 (and before) used to provide the parkingslot or parking_space information when calling this function.

For example chan_skinny.c is using the returned 'exten' to display the the parkingslot information, but ends up displaying the parkingextension, which is useless.

Note: If this is not possible, would it be possible to add a pbx_buildin_setvar_helper(char, "PARKING_SPACE",.... while the call occupies the parking_space, which gets assigned at the moment when the call returns because of time.

Note: If the change is not made, is there another way to get at the required information ?
Comments:By: Asterisk Team (asteriskteam) 2016-05-17 06:37:11.759-0500

Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution.

A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report.

Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].

By: Richard Mudgett (rmudgett) 2016-05-17 12:26:17.502-0500

The *only* caller of ast_parking_park_call() is chan_skinny.c.  Unfortunately in v12+, the parking space is no longer known when the function returns because other threads are now involved.  Off hand, I don't see a way to get that information synchronously.

By: Diederik de Groot (dkdegroot) 2016-05-18 05:00:56.519-0500

The function documentation (API) should reflect the functionality of the function, right ? Are you gonna fix the docs and just drop the use of 'exten' in chan_skinny.c, or should we look for a solution, as not to break the API ?

We should be able to implement a simple workaround, by using a helper variable like 'PARKING_PLACE'. which get added to the parked call, right ? We should be able query for that value after the function has finished 'succesfully', right ?

By: Richard Mudgett (rmudgett) 2016-05-20 14:40:51.112-0500

{quote}
We should be able to implement a simple workaround, by using a helper variable like 'PARKING_PLACE'. which get added to the parked call, right ?
{quote}
How would you access such a channel variable?  The variable cannot be put on the parking channel as it may no longer exist when the parked channel finally goes into the park holding bridge.  The channel variable could be put on the parked channel but you wouldn't know which channel that is.  You may think that it is the bridged peer channel, but that is not always true.  You can now actually park a multi-party bridge instead of a single channel; in which case a local channel pair is used to link the multi-party bridge with the parking space.

{quote}
We should be able query for that value after the function has finished 'successfully', right?
{quote}
This is also not possible even if you knew which channel to fetch the channel variable value from.  The channel that ultimately gets parked is parked by another thread.  That other thread may or may not have parked the channel by the time the thread executing the function has 'successfully' returned.

{quote}
The function documentation (API) should reflect the functionality of the function, right?
{quote}
Yes it should.

{quote}
Are you gonna fix the docs and just drop the use of 'exten' in chan_skinny.c, or should we look for a solution, as not to break the API?
{quote}
As only chan_skinny is affected and it is an extended support module and support is provided by the Asterisk community, no I'm not going to fix it.  The more I look at what is needed, the more I see why it was likely never done.  I'm running into the same issues with finding which channel ultimately gets parked that I pointed out above.  I will however, provide some pointers for a possible fix.

* {{parking.c:ast_parking_park_call()}} calls {{parking_bridge_features.c:parking_park_call()}} through table callback indirection.
* The ParkAndAnnounce application used to also use the equivalent ast_parking_park_call() function in v11.  It now creates a stasis subscription callback for parking events.  The parked channel event contains the parking space used so it can generate the audio announce message.
* You need to get a stasis subscription setup for the channel that ultimately gets parked and make {{parking_bridge_features.c:parking_park_call()}} wait for the subscription callback to get called.  A subscription already does get made but it is buried in lower layer code so it is difficult to take advantage of it.
* The callback available to {{parking_bridge_features.c:parking_blind_transfer_park()}} could be used to create the necessary subscription.  However, it would only only work single party parking situations.  Parking a multi-party bridge would fail as the callback is called on the wrong channel and too late to setup the subscription.
* The subscription needs to wake up the thread calling {{parking_bridge_features.c:parking_park_call()}} so it can retrieve the parking space.
* The waiting thread needs to give up and stop the subscription after a timeout because there is no guarantee that the subscription event will ever happen.

Or you can just forget about the use of exten in chan_skinny as you have also suggested.


By: Diederik de Groot (dkdegroot) 2016-05-20 15:40:25.710-0500

Hi Richard,

Thank you very much for all the detail provided (I am sure it will be usefull to me in other respects). Looks like a whole slew of work, so a simple statusbar message :-( Because of it being asynchronously and having multiple hazards, i think i should give on this request. We are already monitoring the parking related AMI events, so i should be able to update the statusbar that way (only as a last resort).

It might be a good idea to fix the function documentation, to clarify what it's returning, so you don't get the same request again. And I would deprecate the returned 'exten' param complete, as it's now pretty useless.

By: Diederik de Groot (dkdegroot) 2016-05-20 15:41:33.806-0500

Not going to be fixed, workaround possible.
Function Documentation should be updated
chan_skinny developers could be informed