[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] A new higher-level API for working with Asterisk configs, with exam
From: "Matt Jordan" <reviewboard () asterisk ! org>
Date: 2012-05-31 14:14:18
Message-ID: 20120531141418.8796.20921 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1873/#review6356
-----------------------------------------------------------
Since you've modified the configure script, do you also need to modify configure.ac?
/trunk/tests/test_config.c
<https://reviewboard.asterisk.org/r/1873/#comment11870>
This test is a little large for all that it does. For clarity, it'd be nice if \
it was broken up a bit.
If you decide to not go that route, then an explanation as to what all is being \
tested in here is appropriate. For example: do you cover parsing of off nominal \
inputs (a string value into a double data type)? Why is there a loop that iterates \
four times in the test? What all is it covering?
A person shouldn't have to step through every line of code to get a high level \
view of what a test does.
Additionally, it appears as if you need a unit test that verifies parsing of \
multiple config files into a single configuration object (i.e., users.conf/sip.conf)
/trunk/tests/test_config.c
<https://reviewboard.asterisk.org/r/1873/#comment11868>
These off nominal paths need to ensure that resources are still cleaned up
- Matt
On May 29, 2012, 11:51 a.m., Terry Wilson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1873/
> -----------------------------------------------------------
>
> (Updated May 29, 2012, 11:51 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
> -----
>
> /configure UNKNOWN
> /trunk/Makefile 367839
> /trunk/apps/app_skel.c 367839
> /trunk/configs/app_skel.conf.sample PRE-CREATION
> /trunk/configure.ac 367839
> /trunk/include/asterisk/astobj2.h 367839
> /trunk/include/asterisk/config.h 367839
> /trunk/include/asterisk/config_options.h PRE-CREATION
> /trunk/include/asterisk/stringfields.h 367839
> /trunk/include/asterisk/utils.h 367839
> /trunk/main/asterisk.exports.in 367839
> /trunk/main/astobj2.c 367839
> /trunk/main/config.c 367839
> /trunk/main/config_options.c PRE-CREATION
> /trunk/main/udptl.c 367839
> /trunk/makeopts.in 367839
> /trunk/tests/test_config.c 367839
>
> Diff: https://reviewboard.asterisk.org/r/1873/diff
>
>
> Testing
> -------
>
> Lots of testing with malloc debug, valgrind, etc.
>
>
> Thanks,
>
> Terry
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1873/">https://reviewboard.asterisk.org/r/1873/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Since you've \
modified the configure script, do you also need to modify configure.ac?</pre> <br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://reviewboard.asterisk.org/r/1873/diff/5/?file=28381#file28381line430" \
style="color: black; font-weight: bold; text-decoration: \
underline;">/tests/test_config.c</a> <span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">AST_TEST_DEFINE(config_options_test)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">430</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">case</span> <span class="n">TEST_INIT</span>:</pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">431</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">info</span><span class="o">-></span><span class="n">name</span> <span \
class="o">=</span> <span class="s">"config_options_test"</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">432</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">info</span><span class="o">-></span><span class="n">category</span> \
<span class="o">=</span> <span class="s">"/config/"</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">433</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">info</span><span class="o">-></span><span class="n">summary</span> <span \
class="o">=</span> <span class="s">"Config opptions unit test"</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">434</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">info</span><span class="o">-></span><span class="n">description</span> \
<span class="o">=</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">435</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="s">"Tests the Config Options API"</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">436</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">return</span> <span class="n">AST_TEST_NOT_RUN</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">437</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">case</span> <span class="n">TEST_EXECUTE</span>:</pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">438</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">break</span><span class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">439</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="p">}</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This test \
is a little large for all that it does. For clarity, it'd be nice if it was \
broken up a bit.
If you decide to not go that route, then an explanation as to what all is being \
tested in here is appropriate. For example: do you cover parsing of off nominal \
inputs (a string value into a double data type)? Why is there a loop that iterates \
four times in the test? What all is it covering?
A person shouldn't have to step through every line of code to get a high level \
view of what a test does.
Additionally, it appears as if you need a unit test that verifies parsing of multiple \
config files into a single configuration object (i.e., users.conf/sip.conf)</pre> \
</div> <br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://reviewboard.asterisk.org/r/1873/diff/5/?file=28381#file28381line516" \
style="color: black; font-weight: bold; text-decoration: \
underline;">/tests/test_config.c</a> <span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">AST_TEST_DEFINE(config_options_test)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">516</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="o">!</span><span \
class="p">(</span><span class="n">item</span> <span class="o">=</span> <span \
class="n">ao2_find</span><span class="p">(</span><span class="n">cfg</span><span \
class="o">-></span><span class="n">items</span><span class="p">,</span> <span \
class="s">"item"</span><span class="p">,</span> <span \
class="n">OBJ_KEY</span><span class="p">)))</span> <span \
class="p">{</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">517</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">ast_test_status_update</span><span class="p">(</span><span \
class="n">test</span><span class="p">,</span> <span class="s">"could not look up \
'item'</span><span class="se">\n</span><span class="s">"</span><span \
class="p">);</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">518</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">return</span> <span class="n">AST_TEST_FAIL</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">519</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="p">}</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">520</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="o">!</span><span \
class="p">(</span><span class="n">item_defaults</span> <span class="o">=</span> <span \
class="n">ao2_find</span><span class="p">(</span><span class="n">cfg</span><span \
class="o">-></span><span class="n">items</span><span class="p">,</span> <span \
class="s">"item_defaults"</span><span class="p">,</span> <span \
class="n">OBJ_KEY</span><span class="p">)))</span> <span \
class="p">{</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">521</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">ast_test_status_update</span><span class="p">(</span><span \
class="n">test</span><span class="p">,</span> <span class="s">"could not look up \
'item_defaults'</span><span class="se">\n</span><span \
class="s">"</span><span class="p">);</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">522</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">return</span> <span class="n">AST_TEST_FAIL</span><span \
class="p">;</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">523</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="p">}</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">These off \
nominal paths need to ensure that resources are still cleaned up</pre> </div>
<br />
<p>- Matt</p>
<br />
<p>On May 29th, 2012, 11:51 a.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated May 29, 2012, 11:51 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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)) {...}
...
}</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Lots of testing with malloc debug, valgrind, etc.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/configure <span style="color: grey">(UNKNOWN)</span></li>
<li>/trunk/Makefile <span style="color: grey">(367839)</span></li>
<li>/trunk/apps/app_skel.c <span style="color: grey">(367839)</span></li>
<li>/trunk/configs/app_skel.conf.sample <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/configure.ac <span style="color: grey">(367839)</span></li>
<li>/trunk/include/asterisk/astobj2.h <span style="color: grey">(367839)</span></li>
<li>/trunk/include/asterisk/config.h <span style="color: grey">(367839)</span></li>
<li>/trunk/include/asterisk/config_options.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/stringfields.h <span style="color: \
grey">(367839)</span></li>
<li>/trunk/include/asterisk/utils.h <span style="color: grey">(367839)</span></li>
<li>/trunk/main/asterisk.exports.in <span style="color: grey">(367839)</span></li>
<li>/trunk/main/astobj2.c <span style="color: grey">(367839)</span></li>
<li>/trunk/main/config.c <span style="color: grey">(367839)</span></li>
<li>/trunk/main/config_options.c <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/main/udptl.c <span style="color: grey">(367839)</span></li>
<li>/trunk/makeopts.in <span style="color: grey">(367839)</span></li>
<li>/trunk/tests/test_config.c <span style="color: grey">(367839)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1873/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic