[asterisk-dev] [Code Review]: A new higher-level API for working with Asterisk configs, with example code in app_skel.c and udptl.c

Terry Wilson reviewboard at asterisk.org
Wed May 16 14:37:00 CDT 2012



> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/Makefile, line 197
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27899#file27899line197>
> >
> >     No compiley on gcc 4.5.1.  Need to check that the appropriate compiler version is in place before enabling this warning.
> 
> Terry Wilson wrote:
>     I'll see if I can figure out how to do that. I hate build system stuff.

Done.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/configs/app_skel.conf.sample, lines 15-20
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27901#file27901line15>
> >
> >     In addition to having a pvt that contains no information, add a pvt that contains a subset of the allowed fields (such as only bit1 and bit2)
> 
> Terry Wilson wrote:
>     Will do.

Done.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/config_options.h, line 149
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27904#file27904line149>
> >
> >     If this is for internal use only, you may want to create an opaque struct and hide the void *new_config on it.
> 
> Terry Wilson wrote:
>     Well, right now it is only kind of internal. :-/ The pre-apply callback might need to use it. I'll see about wrapping/hiding it.

Fixed.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/config_options.h, line 255
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27904#file27904line255>
> >
> >     I'd prefer an additional process config option as well, that would allow for passing in an in memory ast_config object.  This would be particularly useful for unit testing, but could also be useful in things like app_voicemail, where meta data of voicemails is stored in ast_config objects and may potentially be manipulated prior to loading into in memory objects.
> 
> Terry Wilson wrote:
>     That could be interesting. I'll add it.

Added.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/utils.h, line 866
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27906#file27906line866>
> >
> >     Put a comment on RAII_VAR describing its usage
> 
> Terry Wilson wrote:
>     Will do.

Done.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/main/config_options.c, lines 96-97
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27910#file27910line96>
> >
> >     A size of 80 may not be sufficient to hold the error message returned from regerror.
> >     
> >     The proper way to handle this would be to first call regerror with an errbuff_size of 0 to determine the size of the buffer needed, then call regerror again with that buffer size.  While that complicates this code to some extent, it would be very useful to report the full error returned from regcomp.

Done.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/main/config_options.c, lines 143-146
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27910#file27910line143>
> >
> >     Complain loudly if this happens
> 
> Terry Wilson wrote:
>     Will do.

Done.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/main/config_options.c, lines 189-209
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27910#file27910line189>
> >
> >     We may want to use an AST_LIST traversal here instead - without a SENTINEL, this will seg fault.  Since ensuring that a SENTINEL is placed on the end of the list is in the domain of the user of the framework, the current implementation allows for more coding errors then the AST_LIST macros would allow.
> >     
> >     Can we get into a situation where we don't get a match?  If so, we should probably throw out a warning.
> >     
> >     
> >
> 
> Terry Wilson wrote:
>     As stated above, can't use designated initializers if we use AST_LIST. It would make it a bit more work for users to have to append each type to a list instead of just initializing it. It will most likely segfault immediately if someone doesn't include the sentinel, then it should be easy for them to catch.
>     
>     What about just making a macro that just does it correctly. ACO_TYPES(...) { __VA_ARGS__, SENTINEL } and every example they will look at on how to do things will use that macro. .types = ACO_TYPES(general_opts, peer_opts), etc.?

Added macro.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/main/udptl.c, lines 85-86
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27911#file27911line85>
> >
> >     If these are going to be used in multiple implementation files (which I imagine they are), we may as well #define them in utils.h and give it a prefix of AST_.
> 
> Terry Wilson wrote:
>     Yeah, I think it exists other places as well.

asterisk.h already has a __stringify macro that is identical that optional_api.h uses. I'll just use that for now.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/config_options.h, line 435
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27904#file27904line435>
> >
> >     Blob!
> 
> Terry Wilson wrote:
>     Doh!

Fixed.


> On May 15, 2012, 1:57 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/config_options.h, lines 348-353
> > <https://reviewboard.asterisk.org/r/1873/diff/3/?file=27904#file27904line348>
> >
> >     Use \code and \endcode around code samples (applies to all comments in this header)
> 
> Terry Wilson wrote:
>     Ok. I think 4 space indention is equivalent, but \code \encode makes it easier to see.

Fixed.


- Terry


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


On May 15, 2012, 12:40 a.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1873/
> -----------------------------------------------------------
> 
> (Updated May 15, 2012, 12:40 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review supersedes the ones at reviews 1840 and 1841. There is still a lot of cleanup/documentation work to do. This review is mostly about the overall idea/method. Doing the finishing work before someone comes up with why it is all a bad idea seems like...a bad idea. If you want to comment on non-big-picture stuff, feel free, but most important at this stage is whether or not this is a good idea at all.
> 
> The goal of this patch is to come up with a way to safely handle the loading and reloading a config data. Data should be consistent at all times, even if there is invalid data in the config file. Current modules tend to store the config-related data in private structures and modify it in-place as the config file is parsed. If there is a problem, the data is left in an inconsistent state.
> 
> This solution decouples config-related data from the non-config-related state held in the various private structures. It should atomically swap the global, private, and private config-related data. It also adds a higher-level API for registering the various configuration options that exist at module load, with default callback handlers for common types and the ability to create custom handlers for other types. If the higher-level API is used, a few callback functions are defined and for the most part, config loading and reloading is done with a single function call. If the high-level API is not sufficient, it can either be modified as time-goes on, or a module can use the lower-level config option API functions themselves, keeping to the same overall format of swapping out config objects, etc. for thread safe reloads.
> 
> This patch also makes significant use of the RAII_VAR macro which uses the gcc "cleanup" attribute to make sure that ref counts are cleaned up on return, etc.
> 
> There needs to be a lot more documentation, unit tests, etc. But I should probably hold off on doing any of that until people have had a chance to look at the basic idea, etc. There are some configs that won't work with the high-level API as-is. Anything that uses categories that have the same name (chan_mobile) would need an option added that allows that. Things like ConfBridge with options that are user-definable DTMF codes would need a "catch-all" or pattern-matching for option names. Both would be fairly easy to implement.
> 
> Rationale
> --------
> Why not store config data directly in the privates?
> Because updating the data at the time of parsing can leave things in an indeterminate state on reload.
> 
> What about just storing the config data directly in the privates, and creating new privates as you parse and swap out for the old one?
> Swapping out the entire private structure would lose any non-config-related state in the private structure.
> 
> What about using a copy function for the private's non-config-related state?
> Having to define(and keep it updated as new fields are added) a copy function for every private structure (and essentially for every type stored in that structure) that needs to properly handle reloads sounds like a huge pain to me.
> 
> What about instead of having separate containers for privates and private configs, you just store a pointer to the private config in the private structure itself?
> There are two problems I see with this. 1) To ensure data is consistent when accessing multiple fields, one would need to hold a reference to the cfg in the private. But, since it is just a pointer, it encourages people to use it directly without grabbing a reference. By separating the containers, one must look up the config object and get a reference to it to be able to use it. 2) If there is a problem in the middle of switching out the cfg pointers, you end up with some privates with new configs and some with old.
> 
> Overview of how it works: You basically have the global aco_info struct that defines information pertaining to the whole config. Then there are aco_types which define category-level things like regex for what categories are supported for the type, allocation/lookup functions, whether it is for a single global object, or objects in containers, etc. Below that are aco_options, which define the options available for a given type. For example:
> 
> struct aco_info cfg_info = {
>    .module = AST_MODULE,
>    .filename = "app_skel.conf"
>    .apply_config = skel_apply_config,
>    .preload = {"general", SENTINEL }, /* If you need to load some contexts in order */
> };
> 
> struct skel_global_cfg {
> ...
> };
> 
> struct skel_pvt_cfg {
> ...
> };
> 
> struct skel_pvt {
> ...
> };
> 
> enum {
>     GLOBAL_OPTIONS = 0,
>     PVT_CFG_CONTAINER,
>     PVT_CONTAINER,
>     /* Must be declared last */
>     NUM_GLOBAL_OBJECTS,
> };
> static AO2_GLOBAL_OBJ_STATIC(global_config, NUM_GLOBAL_OBJECTS);
> AST_MUTEX_DEFINE_STATIC(reload_lock);
> 
> /* Required for global */
> void *skel_global_cfg_alloc(const char*cat);
> 
> /* Required for privates (container-stored objects) */
> void *skel_pvt_cfg_alloc(const char *cat);
> void *skel_pvt_find_or_create(const char *cat);
> void *skel_pvt_find_in_container(struct ao2_container *cont, const char *cat);
> int skel_pvt_containers_alloc(struct ao2_container **newpvts, struct ao2_container **newcfgs);
> 
> /* Optional for privates */
> int skel_pvt_cfg_post_init(void *cfg); /* Could be used to inherit global settings...ew. */
> int  skel_pvt_cfg_pre_link(void *cfg); /* Could be used for final verification that things look a-ok */
> 
> static int apply_config(void)
> {   
>     RAII_VAR(void *, new_global, aco_info_new_global_get(&cfg_info, "global"), ao2_cleanup);
>     RAII_VAR(struct ao2_container *, new_pvts, aco_info_new_privates_get(&cfg_info, "private"), ao2_cleanup);
>     RAII_VAR(struct ao2_container *, new_cfgs, aco_info_new_configs_get(&cfg_info, "private"), ao2_cleanup);
>     
>     if (!(new_global && new_pvts && new_cfgs)) {
>         return -1;
>     }
>     /* Do any fixup for global configs here, individual privates could be fixed up via the pre-link callback */
>     
>     ao2_global_obj_replace_unref(global_config, GLOBAL_OPTIONS, new_global);
>     ao2_global_obj_replace_unref(global_config, PVT_CONTAINER, new_pvts);
>     ao2_global_obj_replace_unref(global_config, PVT_CFG_CONTAINER, new_cfgs);
> 
>     return 0;
> }
> 
> static int process_config(int reload)
> {
>     ast_mutex_lock(&reload_lock);
>     if (aco_process_config(&cfg_info, reload)) {...};
>     ast_mutex_unlock(&reload_lock);
> ...
> }
> 
> static int reload(void)
> {
>     if (process_config(1)) {...}
> }
> static int load_module(void)
> {
>   ...
>     aco_info_init(&cfg_info));
>     global_type = aco_type_global_alloc("global", CONTEXT_ALLOW, "general", (aco_type_alloc) skel_global_alloc);
>     priv_type = aco_type_private_alloc("private", CONTEXT_DENY, "general", NULL, NULL, (aco_type_alloc) skel_pvt_cfg_alloc, skel_containers_alloc, skel_find_or_create_pvt, skel_find_pvt, NULL, NULL)
> 
>     aco_type_register(&cfg_info, global_type);
>     aco_type_register(&cfg_info, priv_type);
> 
>     aco_option_register(&cfg_info, "foo", global_type, "booya", OPT_STRINGFIELD_T, 0, STRFLDSET(struct skel_global_config, foo));
> ...
>     if (process_config(0)) {...}
> ...
> }
> 
> 
> Diffs
> -----
> 
>   /trunk/Makefile 366506 
>   /trunk/apps/app_skel.c 366506 
>   /trunk/configs/app_skel.conf.sample PRE-CREATION 
>   /trunk/include/asterisk/astobj2.h 366506 
>   /trunk/include/asterisk/config.h 366506 
>   /trunk/include/asterisk/config_options.h PRE-CREATION 
>   /trunk/include/asterisk/stringfields.h 366506 
>   /trunk/include/asterisk/utils.h 366506 
>   /trunk/main/asterisk.exports.in 366506 
>   /trunk/main/astobj2.c 366506 
>   /trunk/main/config.c 366506 
>   /trunk/main/config_options.c PRE-CREATION 
>   /trunk/main/udptl.c 366506 
> 
> Diff: https://reviewboard.asterisk.org/r/1873/diff
> 
> 
> Testing
> -------
> 
> Lots of testing with malloc debug, valgrind, etc.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120516/1b131df0/attachment-0001.htm>


More information about the asterisk-dev mailing list