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

Terry Wilson reviewboard at asterisk.org
Tue Apr 3 13:06:52 CDT 2012 


> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > Since this application is supposed to be an example that helps people to learn, I think there should be a LOT more comments in this code. If nothing else, add doxygen to everything, even functions like the hash and comparison callbacks. I would also suggest giving some high-level descriptions of what particular blocks of code are doing. In addition, any time you have decided to do something some specific way here, it would be a good idea to list alternatives in case the method used here does not make sense in a particular case.

Yeah, I suppose I should add bunches of comments.


> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > /trunk/apps/app_skel.c, lines 158-161
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line158>
> >
> >     Hm, I see what you've done here, but I think that the way you've done it is not the safest. It requires that you know the size of the global_config array and that you know what items belong in each index. I think the better way to go about it would be the following:
> >     
> >     void *array[NUM_GLOBAL_OBJECTS];
> >     array[GLOBAL_OPTIONS] = new_global;
> >     array[PVT_CONTAINER] = new_pvt_container;
> >     
> >     It takes more lines but it is much clearer and safer.

Yeah, something I missed after I added NUM_GLOBAL_OBJECTS in the first place. Oops!


> 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.

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.


> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > /trunk/apps/app_skel.c, lines 285-286
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line285>
> >
> >     This is an interesting design decision. It's valid, but do you think this will be the norm? I ask because app_skel.c is supposed to be an example and if it's doing something typically not done in our apps, then it's not very helpful.

Yes, I think it should be the norm. The reason is that we don't want partially parsed config files to take effect and are trying to make commits as atomic as possible. In doing this, we are swapping out the entire global config and the entire private/peer container. If we don't fail here instead of skipping, if someone makes an invalid change to a peer, that peer would no longer exist after the reload. Better to alert the user that they messed up and not change the config at all until they can fix it.

One of the situations that exists (but shouldn't, IMHO) is that general options can be set and overridden by peer-specific options. So changing a general option can require all peers to be rebuilt as well. That is why later we also require both general options and all peers to be successfully built before applying the config.


> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > /trunk/apps/app_skel.c, lines 315-316
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line315>
> >
> >     Since this is supposed to be a shining example, "booya" and "0.0.0.0:1234" should probably be defined as constants.

Yeah, I'm working on an API right now to do handle registering config options and their defaults.


> On April 3, 2012, 12:29 p.m., Mark Michelson wrote:
> > /trunk/apps/app_skel.c, line 444
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line444>
> >
> >     hehe "show privates"

I couldn't help myself. :-)


- 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/20120403/9b57bd60/attachment-0001.htm>

More information about the asterisk-dev mailing list