[asterisk-dev] [Code Review] Fix for parking lot settings not being respected with unit test

[Jeff Peeler]

> On 2010-03-03 11:17:50, Mark Michelson wrote:
> > /trunk/main/features.c, line 862
> > <https://reviewboard.asterisk.org/r/539/diff/1/?file=8461#file8461line862>
> >
> >     Is this safe? I ask because you will have args pointing to the pu that is created in park_space_reserve. I'm wondering if it is possible for manage_parkinglot to free this pointer out from under you.
> >     
> >     Or is such a problem protected by the (terribly-named) notquiteyet field of the pu struct?

Technically it is dangerous, but sending in arguments like that is not accessible to the public API. And in this test I am making sure to pull the channel out of the parking lot before the parking thread tries to hang it up.


> On 2010-03-03 11:17:50, Mark Michelson wrote:
> > /trunk/main/features.c, lines 1134-1137
> > <https://reviewboard.asterisk.org/r/539/diff/1/?file=8461#file8461line1134>
> >
> >     Is there a potential for multiple copies of args->pu to be in the list? If not, then you can break at this point instead of continuing.

break it is


> On 2010-03-03 11:17:50, Mark Michelson wrote:
> > /trunk/main/features.c, lines 1142-1149
> > <https://reviewboard.asterisk.org/r/539/diff/1/?file=8461#file8461line1142>
> >
> >     I'm a bit curious why you only unref the parkinglot and free args->pu if you find the pu in the parkinglot.
> >     
> >     Shouldn't you unref the parkinglot and free memory unconditionally?

Well that's part of my ref count problem, I shouldn't be doing anything with the parkinglot there. And no, I shouldn't be freeing the memory there if it wasn't found because the parking thread would have done so... which means more unsafeness. I'll have to think about whether or not it is worth worrying about.


- Jeff


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


On 2010-03-02 20:47:45, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/539/
> -----------------------------------------------------------
> 
> (Updated 2010-03-02 20:47:45)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This verifies a change in parking to ensure the channel variable PARKINGLOT is respected. Kind of overkill for an obvious change, but many more parking scenarios can be added later.
> 
> 
> This addresses bug 16592.
>     https://issues.asterisk.org/view.php?id=16592
> 
> 
> Diffs
> -----
> 
>   /trunk/main/features.c 250288 
> 
> Diff: https://reviewboard.asterisk.org/r/539/diff
> 
> 
> Testing
> -------
> 
> Verified proposed fix works and that all created channels are properly destroyed. It seems though that I do actually have a ref count problem with the dynamic lot used for testing, which I haven't been able to track down yet. I went ahead and posted the review so the rest of the test can be examined.
> 
> 
> Thanks,
> 
> Jeff
> 
>