[asterisk-dev] [Code Review] parkinglots with different parking extensions

[Russell Bryant]

> On 2009-12-15 03:01:59, Russell Bryant wrote:
> > /trunk/main/features.c, lines 4268-4277
> > <https://reviewboard.asterisk.org/r/417/diff/1/?file=7155#file7155line4268>
> >
> >     This is missing an ao2_iterator_destroy().  However, it looks like you're fixing a bug in parking lot configuration handling that is not necessarily related to your new feature.  I think we should address this configuration handling bug in a separate patch since there is another method I would prefer to use to ensure that parking lots that are no longer in configuration get removed from memory.
> 
> mnick wrote:
>     how would you approach this ?
>     I thought it would be the easiest way to delete the parkinglots and create them again new

The problem with this type of approach is that you end up with a period of time where a configured parking lot, even if it is still in your configuration, is no longer present in memory.  So, parking a call may fail if it happens during a reload.

The approach used in other places is to first "mark" (set a flag) all objects currently in memory.  As you reload configuration, if you find configuration for an object that was already in memory, you un-mark it.  New objects do not have the flag set.  Then, at the end, you remove only the objects that are still marked.  Those are the ones that are no longer in your configuration.

Again, though, your new feature is not related to that fact that configuration handling for parking lots was broken.  You can address it if you would like, but it's really a different problem that we should probably deal with in a separate patch.


> On 2009-12-15 03:01:59, Russell Bryant wrote:
> > /trunk/main/features.c, lines 4058-4059
> > <https://reviewboard.asterisk.org/r/417/diff/1/?file=7155#file7155line4058>
> >
> >     You can simplify this down to a single snprintf().
> 
> mnick wrote:
>     how would you do this ?

snprintf(device, sizeof(device), "park:%d@%s", numext, context);


- Russell


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


On 2009-10-29 09:45:22, mnick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/417/
> -----------------------------------------------------------
> 
> (Updated 2009-10-29 09:45:22)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In features.conf you can define one parking extension: for example
> [general]
> parkext = 700
> parkpos = 701-750
> ...
> 
> you can also define new parkinglots like:
> [parkinglot_park2]
> context = park2
> parkpos = 801 - 850
> 
> but it is not possible to set a new 'parkext' to 'parkinglot_park2' (like: parkext = 800)
> 
> 
> Now it should be possible to have more than one parking extension. 
> [parkinglot_park2]
> parkext = 800
> parkpos = 801 - 850
> context = park2
> 
> [parkinglot_sales]
> parkext = 900
> parkpos = 901 - 950
> context = sales
> 
> 
> [Please ignore the 'my_dump_chan()' and 'my_dump_parkinglot()' functions, they're just for testing]
> 
> 
> This addresses bug 14882.
>     https://issues.asterisk.org/view.php?id=14882
> 
> 
> Diffs
> -----
> 
>   /trunk/main/features.c 226458 
> 
> Diff: https://reviewboard.asterisk.org/r/417/diff
> 
> 
> Testing
> -------
> 
> I have tried various scenario's with different parkinglot's and context's. 
> But I am not sure if I've covered all cases.
> 
> 
> Thanks,
> 
> mnick
> 
>