<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;">I've made a few comments here with regards to specifics, but I think the better approach here is for me to comment on things more generally. In other words, I have not gone and ensured that everything is perfect in every detail, nor have I bothered to give critiques for things like function names or composition.
First, go read my comments so that you can see individual critiques.
In general, I like this approach. Everything that uses a config file can follow the same rules for loading configuration and everyone will be consistent. My biggest gripe is that the documentation for this is pretty much nonexistent. I'm using your examples in app_skel.c and udptl.c to try to figure out how this would be used, but you can see from my comments below that there are some unanswered questions:
* What if a type cannot be discerned based on the category name alone (e.g. IAX2 peers and users)?
* What does a module do if a single type can be configured by multiple files (e.g. SIP peers using sip.conf and users.conf)?
* What are all the hook functions used for? I can see where in the code they're called, but what's a use case you could see for each of the individual callbacks?
This is a lot to take in, and considering those are my only questions from this, I'm liking it a lot. That being said, if this were being considered for inclusion I'd take a much closer look and make suggestions with regards to more specific matters.</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/1/?file=27365#file27365line43" style="color: black; font-weight: bold; text-decoration: underline;">/include/asterisk/config_options.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum aco_type_t {</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">43</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm">/*! \brief Whether a context regex is a blackist or a whitelist */</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">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">enum</span> <span class="n">aco_context_op</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">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">CONTEXT_DENY</span> <span class="o">=</span> <span class="mi">0</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">46</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">CONTEXT_ALLOW</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">47</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;">So I take it that the idea here is that you can specify that an aco_type should process any category in a configuration file whose name either matches or does not match a regex. So for instance, if you have a "general" section and then every other section is used to configure "foos" then the aco_type corresponding to a "foo" would have some sort of regex blacklist for "general".
I don't think this is quite enough to be able to distinguish different types of configurable options. For instance, in iax.conf, you can't tell the difference between an iax2_user and an iax2_peer just based on the context name. You also have to look at the "type=" option to determine that.</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/1/?file=27365#file27365line49" style="color: black; font-weight: bold; text-decoration: underline;">/include/asterisk/config_options.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum aco_type_t {</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">49</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">void</span> <span class="o">*</span><span class="p">(</span><span class="o">*</span><span class="n">aco_type_alloc</span><span class="p">)(</span><span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">category</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">50</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">int</span> <span class="p">(</span><span class="o">*</span><span class="n">aco_type_containers_alloc</span><span class="p">)(</span><span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">**</span><span class="n">newpvts</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">**</span><span class="n">newcfgs</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">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">void</span> <span class="o">*</span><span class="p">(</span><span class="o">*</span><span class="n">aco_type_find_or_create_pvt</span><span class="p">)(</span><span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">context</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">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">void</span> <span class="o">*</span><span class="p">(</span><span class="o">*</span><span class="n">aco_type_find_pvt</span><span class="p">)(</span><span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">*</span><span class="n">newcontainer</span><span class="p">,</span> <span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">context</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">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">int</span> <span class="p">(</span><span class="o">*</span><span class="n">aco_type_post_cfg_init</span><span class="p">)(</span><span class="kt">void</span> <span class="o">*</span><span class="n">newcfg</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">54</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="kt">int</span> <span class="p">(</span><span class="o">*</span><span class="n">aco_type_prelink</span><span class="p">)(</span><span class="kt">void</span> <span class="o">*</span><span class="n">newcfg</span><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 really need more documentation to help understand what they would be useful for. Obviously they are hooks into the various stages of configuration, but names like "post_cfg_init" and "prelink" don't give much insight into when they are called or what they might be used for. Unlike the other hooks in this list, you don't have any examples of these being used.</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/1/?file=27365#file27365line68" style="color: black; font-weight: bold; text-decoration: underline;">/include/asterisk/config_options.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum aco_type_t {</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">68</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">filename</span><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;">What happens if a configurable object uses data from multiple files? For instance, chan_sip loads both sip.conf and users.conf when creating SIP peers.</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/1/?file=27365#file27365line189" style="color: black; font-weight: bold; text-decoration: underline;">/include/asterisk/config_options.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum aco_type_t {</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">189</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param struct_type The type of the struct being configured (e.g. struct skel_pvt_config)</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;">There is no struct type parameter in this macro.</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/1/?file=27365#file27365line247" style="color: black; font-weight: bold; text-decoration: underline;">/include/asterisk/config_options.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum aco_type_t {</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">247</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm">/* This is sneaky. On the very first argument, we set "in" to N, the number of arguments, so</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">248</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * that the accumulation both works properly for the first argument (since "in" can't be empty) and</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">249</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * we get the number of arguments in our varargs as a bonus */</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">250</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define ARGMAP_(N, func, func_arg, x, ...) PASTE(ARGMAP_, N)(func, func_arg, N, x, __VA_ARGS__)</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;">What is "in" ?</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/1/?file=27371#file27371line91" style="color: black; font-weight: bold; text-decoration: underline;">/main/config_options.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">static void config_option_destroy(void *obj)</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">91</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_int_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_uint_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">93</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_double_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">94</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_sockaddr_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">95</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_stringfield_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">96</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_bool_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">97</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_acl_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</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">98</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">ast_config_option_codec_fn</span><span class="p">(</span><span class="k">const</span> <span class="k">struct</span> <span class="n">aco_option</span> <span class="o">*</span><span class="n">opt</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_variable</span> <span class="o">*</span><span class="n">var</span><span class="p">,</span> <span class="kt">void</span> <span class="o">*</span><span class="n">obj</span><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;">One type of function you're missing here is one to copy a string into a buffer (think CV_STR from config.h).</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/1/?file=27372#file27372line85" style="color: black; font-weight: bold; text-decoration: underline;">/main/udptl.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ASTERISK_FILE_VERSION(__FILE__, "$Revision$")</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">85</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">udptlstart</span> <span class="o">=</span> <span class="n">DEFAULT_UDPTLSTART</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">85</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define STR(x) STR1(x)</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">udptlend</span> <span class="o">=</span> <span class="n">DEFAULT_UDPTLEND</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define STR1(x) #x</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;">A similar macro called __stringify exists in optional_api.h. It seems like this sort of macro would be useful to have in utils.h or strings.h</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On April 16th, 2012, 1:45 p.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 April 16, 2012, 1:45 p.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>/trunk/main/config_options.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/config.c <span style="color: grey">(362149)</span></li>
<li>/trunk/main/asterisk.exports.in <span style="color: grey">(362149)</span></li>
<li>/trunk/main/astobj2.c <span style="color: grey">(362149)</span></li>
<li>/trunk/include/asterisk/utils.h <span style="color: grey">(362149)</span></li>
<li>/trunk/include/asterisk/stringfields.h <span style="color: grey">(362149)</span></li>
<li>/trunk/include/asterisk/config_options.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/config.h <span style="color: grey">(362149)</span></li>
<li>/trunk/include/asterisk/astobj2.h <span style="color: grey">(362149)</span></li>
<li>/trunk/configs/app_skel.conf.sample <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/apps/app_skel.c <span style="color: grey">(362149)</span></li>
<li>/trunk/main/udptl.c <span style="color: grey">(362149)</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>