[asterisk-dev] [Code Review]: Add config parsing example code to app_skel

[Terry Wilson]

> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > /trunk/apps/app_skel.c, lines 222-229
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line222>
> >
> >     This got me thinking a bit about a certain situation. What if someone specifies a silly config like the following:
> >     
> >     [pvt1]
> >     description=blah
> >     permit=127.0.0.1/32
> >     
> >     [pvt2]
> >     description=blahblahblah
> >     
> >     [pvt1]
> >     description=blahblah
> >     allow=!all,ulaw
> >     
> >     Now in this case, the user has specified the same category name twice. Presumably, this was done on purpose so that the configuration options would stack.
> >     
> >     First off, does the text configuration loading backend do anything to combine these two categories into one "supercategory"? Or are we presented with two identically-named categories?
> >     
> >     If the former, then what you have here will work fine.
> >     
> >     If the latter, then there's a small issue. Since you don't check in the temporary pvt container to see if a skel_pvt with the same name already exists, then you'll end up with two skel_pvts with the same name in the container. Since skel_pvt_cmp returns both CMP_MATCH and CMP_STOP when a match is found, it means one of the two will be impossible to locate. Moreover, it means the configuration options will not stack in the way the user presumably intended.
> >     
> >     LATER EDIT: I just now noticed that it's possible to set name=something in skel_pvts as well. This further muddies this issue since you can have two differently-named categories that have the same name=something option specified. Maybe you should consider removing that option.
> 
> Terry Wilson wrote:
>     Setting name in there is a bug. I always assumed that the backend handles merging contexts like that. If it doesn't, then it is very silly. I'll investigate.

Ok, so I've checked and it doesn't merge the categories for you automatically. As Paul said, this is definitely a bug in the config since (+) should be used. But, it is important for us to catch it when it does happen and fail the reload. Good catch.


- Terry


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


On April 2, 2012, 2:50 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1841/
> -----------------------------------------------------------
> 
> (Updated April 2, 2012, 2:50 p.m.)
> 
> 
> Review request for Asterisk Developers, rmudgett and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch adds config parsing to app_skel to serve as an example of how to properly write config handling using the ao2 global object API for thread-safe reloads. I also made a couple of whitespace changes even though they technically should have gone in a separate patch, but they didn't get in the way of seeing what changed, etc.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_skel.c 360984 
>   /trunk/configs/app_skel.conf.sample PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1841/diff
> 
> 
> Testing
> -------
> 
> "skel show config/privates" and "memory show summary app_skel.c/config.c"
> 
> 
> Thanks,
> 
> Terry
> 
>

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