[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
Fri Apr 27 14:36:25 CDT 2012
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > I like a lot of what I am seeing.
> >
> > I don't see how private non-config state relates to private config data. I also do not see how a running module can update its config without disrupting activities even if a specific private object's config does not change.
> I like a lot of what I am seeing.
Yay!
> I don't see how private non-config state relates to private config data.
Private non-config state is stored in the private structure, which is linked in the private container with the same key (or at least a related key) that the private config data is linked to the private config container. The important thing is to be able to write a function that takes a private, and looks up the config data for it based on the data in the private. In practice, this is likely going to be done by just having a "name" field in both of them that is hashed for storing things in their respective containers. I would imagine that people would generally write a little helper function like struct pvt_config *get_config_data(struct *pvt) which does the lookup and returns the refcounted config data.
> I also do not see how a running module can update its config without disrupting activities even if a specific private object's config does not change.
Since each use of the config data will require looking up the refcounted config data, then any code using that data will continue to use that refcounted data even after a reload. When the function releases the refcount, the next call will require looking up the data again and therefor getting the new config data.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 148-151
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line148>
> >
> > I thought boolean options required a custom handler as well.
Boolean options will only require a custom handler if they are stored as a bitfield. If you just store then as ints, it isn't a problem.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 317-325
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line317>
> >
> > Doing the find before checking if preload is a waste. Should do the find after preload check.
Will do.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, line 424
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line424>
> >
> > This line is redundant. Done at the error label.
oops!
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 463-466
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line463>
> >
> > You should not quit. Just give warning and ignore option. The option is either misspelled or not supported in this Asterisk version.
> > Also you can indicate the file name var->file in the message.
> >
> > How about:
> > Unknown option '%s' in category '%s' at line %d of file %s.
No. I specifically want to quit. If someone edits and screws up their config, they should fix it. We shouldn't just warn and then have screwed up data. Tell them it is broken, and don't do the reload. There is absolutely no good reason to do a reload when we *know* that the data is bad.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 467-470
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line467>
> >
> > Put an ast_debug here to announce the problem.
Ok.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 471-474
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line471>
> >
> > You can indicate the file name var->file in the message.
Ah, handy. :-)
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, lines 416-419
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line416>
> >
> > This check is redundant because
> > CONFIG_STATUS_FILEMISSING is NULL.
It is redundant now. It isn't redundant if someone redefines what CONFIG_STATUS_FILEMISSING is at some point in the future. If we have a #define, we should treat it like a #define, not magically assume what the value really is underneath.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, line 153
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27361#file27361line153>
> >
> > Why is this commented out. Does this not work?
It works, but isn't needed. Basically just showing that it was available. I tested it, and never took it out.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, lines 149-154
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27361#file27361line149>
> >
> > This could be stored in the global ao2 array. It has a destructor. The ao2 object representing this struct does not need to be created with a lock. (ao2_alloc_options(size, destructor, AO2_ALLOC_OPT_LOCK_NOLOCK))
It could, but I don't see a lot of benefit to it. The info struct has to be passed around, it is read-only, and for it to be stored in the array, I'd have to allocate it as an ao2 object which would make defining the fields uglier than just initializing them like this.
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, lines 266-268
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27361#file27361line266>
> >
> > Each of these ao2 global array elements must be independent of each other. Otherwise, the brief time that you are updating them will result in inconsistency.
It don't think it would be a problem if I had an ao2_global_obj_replace_all/multiple function that kept the internal lock for the array locked for all of the updates--basically treating it as a transaction. Either you can write it, or I will. :-)
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/main/config_options.c, line 429
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27371#file27371line429>
> >
> > This is always true.
ugh. indeed. and if preload (a C99 flexible array) isn't set at all, it doesn't get a default initializer either. I suppose I can do something like creating a default initializer macro that sets .preload = { 0, } and also takes care of .module = AST_MODULE. Then I can check info->preload[0].
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/include/asterisk/config_options.h, lines 59-62
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27365#file27365line59>
> >
> > The need for static strings for name, context, matchfield, and matchvalue must be documented! It will be bad if they are not!
Ok, I'll note to add that to the docs when I write them. :-)
> On April 26, 2012, 7 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, lines 539-540
> > <https://reviewboard.asterisk.org/r/1873/diff/1/?file=27361#file27361line539>
> >
> > aco_info_destroy is not called on decline.
Correct! I shall fix that.
- Terry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1873/#review6080
-----------------------------------------------------------
On April 16, 2012, 1:45 p.m., Terry Wilson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1873/
> -----------------------------------------------------------
>
> (Updated April 16, 2012, 1:45 p.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/main/config_options.c PRE-CREATION
> /trunk/main/config.c 362149
> /trunk/main/asterisk.exports.in 362149
> /trunk/main/astobj2.c 362149
> /trunk/include/asterisk/utils.h 362149
> /trunk/include/asterisk/stringfields.h 362149
> /trunk/include/asterisk/config_options.h PRE-CREATION
> /trunk/include/asterisk/config.h 362149
> /trunk/include/asterisk/astobj2.h 362149
> /trunk/configs/app_skel.conf.sample PRE-CREATION
> /trunk/apps/app_skel.c 362149
> /trunk/main/udptl.c 362149
>
> 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/20120427/8e5d661d/attachment-0001.htm>
More information about the asterisk-dev
mailing list