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

[Mark Michelson]

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


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.


/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10863>

    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.



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10860>

    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.



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10861>

    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.



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10859>

    Since this is supposed to be a shining example, "booya" and "0.0.0.0:1234" should probably be defined as constants.



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10864>

    hehe "show privates"


- Mark


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/29c7033d/attachment-0001.htm>