[asterisk-dev] [Code Review] 2550: res_parking: Dynamic Parking Lots

[jrose]

> On May 20, 2013, 10:45 p.m., Matt Jordan wrote:
> > /team/group/bridge_construction/res/res_parking.c, lines 627-633
> > <https://reviewboard.asterisk.org/r/2550/diff/1/?file=38290#file38290line627>
> >
> >     It actually feels like this is a valid user error condition. It's very possible that someone configures a parking lot in features.conf (or some other config file) and fat fingers a dynamic parking lot creation in extensions.conf. If you attempt to create a dynamic parking lot and you've already configured a static one, this should log out a WARNING and return.
> >     
> >     Asserts aren't appropriate for code paths that user configuration can go down. Asserts should be used for situations that violate the fundamental design of your application.

No, it's not something that is supposed to be possible.  The only reason to create a parking lot dynamically is if we tried to find a normal parking lot to park to and it didn't exist. I put this assert here in case something comes along eventually that doesn't follow that rule. I've added extra documentation to this effect.

I really think I should add some locking for that process too, so I'm going to go ahead and do that.


> On May 20, 2013, 10:45 p.m., Matt Jordan wrote:
> > /team/group/bridge_construction/res/res_parking.c, lines 809-812
> > <https://reviewboard.asterisk.org/r/2550/diff/1/?file=38290#file38290line809>
> >
> >     Don't prescribe solutions in error messages. The error message should inform the user what the error is - for all we know, they don't want to use either channel variable to avoid anything - maybe they just accidentally entered the same parking lot name.
> >     
> >     I would phrase it differently, something closer to:
> >     
> >     "Failed to create dynamic parking lot %s@%s: parking lot already exists"

Actually, I think I'm just going to get rid of all this code that checks against the template parking lot.  It's just redundant really what with the extension generating code in https://reviewboard.asterisk.org/r/2545/ also checking these details against the PBX itself.


- jrose


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


On May 17, 2013, 10:13 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2550/
> -----------------------------------------------------------
> 
> (Updated May 17, 2013, 10:13 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21644
>     https://issues.asterisk.org/jira/browse/ASTERISK-21644
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Dynamic Parking Lots. Parking lots that are made based on other parking lots as well as a combination of some variables. They get a little bit of overlap extension checking that they do before actually trying to register any extensions...
> 
> For the most part, they function like normal parking lots. The exception is that they get broken down if they ever become empty. In that sense, they behave as though they have already been disabled, it's just that you can keep parking to them while they are still alive.
> 
> Some of the stuff in this review references functionality that is going to be provided by the extensions registration update once that is finished.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 389006 
>   /team/group/bridge_construction/res/parking/parking_applications.c 389006 
>   /team/group/bridge_construction/res/parking/parking_bridge.c 389006 
>   /team/group/bridge_construction/res/parking/parking_ui.c 389006 
>   /team/group/bridge_construction/res/parking/res_parking.h 389006 
>   /team/group/bridge_construction/res/res_parking.c 389006 
> 
> Diff: https://reviewboard.asterisk.org/r/2550/diff/
> 
> 
> Testing
> -------
> 
> Created dynamic parking lots with a number of different variables, made sure I could keep parking additional people to them as long as they didn't break down, made sure they were destroyed when they became empty, made sure they didn't leak memory in any obvious ways (tracked by res_parking.c, parking_bridge.c, and parking_controller.c) made sure they were visible in the UI as long as they were alive.
> 
> 
> Thanks,
> 
> jrose
> 
>

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