<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/4276/">https://reviewboard.asterisk.org/r/4276/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 17th, 2014, 8:51 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 think the key for me understanding and accepting this change is to decipher why:
"When the module is loaded again, a reload of res_pjsip is triggered which causes the observer to do a reload as opposed to a load of the file"
this happens. Why does a reload of res_pjsip cause the observer to perform a reload instead of a load?</pre>
</blockquote>
<p>On December 17th, 2014, 9:32 p.m. UTC, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, res_pjsip can't (practically) be unloaded right? So all it knows is reload. If you call reload on res_pjsip, it calls load on all object types for all wizards with the reloaded flag set. That's correct.
Meanwhile, if the res_pjsip_config_wizard module has been unloaded and then loaded again, it's starting fresh and empty. It then calls reload on res_pjsip to trigger the construction of all the objects. res_pjsip then does it's thing and calls load w/ reloaded on all the wizards. Now the res_pjsip_config_wizard object_type_loaded observer gets called with the reloaded flag set. This sets the FILEUNCHANGED flag in the config load which returns FILEUNCHANGED so no ob jects get created. This happens because config has cached the mtime of the pjsip_wizard.conf file and saw no change.
In order for this to work, when res_pjsip_config_wizard unloads, it has to clear the config mtime cache for pjsip_wizard.conf. When it loads again, config load will not find it in the cache and return a valid config even tough the FILEUNCHANGED flag was set.
</pre>
</blockquote>
<p>On December 17th, 2014, 10:05 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for the explanation. My thought on this is that res_pjsip_config_wizard shouldn't necessarily translate the fact that pjsip.conf was reloaded to mean that pjsip_wizard.conf also needs a reload. The two config files are managed by separate modules, and the fact that one module reloaded its config doesn't necessarily translate to mean that the other module should be reloading its config. res_pjsip_config_wizard should be performing a load operation instead if it has not previously loaded the config file. Since you're already maintaining state of the previous config in the object_type_wizard struct, would it not make sense to use its presence to know whether you should set CONFIG_FLAG_FILEUNCHANGED? Something like:
if (otw->last_config) {
ast_set_flag(&flags, CONFIG_FLAG_FILEUNCHANGED);
}
My main reason for trying to keep the change within res_pjsip_config_wizard.c is that I don't like the idea of exposing internals of the config system for just one use case in the whole of Asterisk unless it is absolutely necessary. Exposing the ability to clear a file from the cache doesn't really make sense when there's already a method of loading a file without caring about what the cache currently contains.</pre>
</blockquote>
<p>On December 17th, 2014, 10:14 p.m. UTC, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It's not looking at pjsip.conf at all and res_pjsip_config_wizard has gone away completely so it can't save any state. I suppose that when it loads I can do a config load for each object type with a NOCACHE flag, then just destroy the config. I think that clears the mtime cache. That's a kludge though for the fact that you can't clear the config cache. Once the wizard loads though, it's res_pjsip that controls when the wizard loads its objects.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Right, the fact that the state doesn't exist is exactly why I'm suggesting to check it. When res_pjsip_config_wizard is loaded, none of the object type wizards it creates will have a previous config saved on them. Therefore, the next time that the object type wizard's loaded() callback is called, we know that there is no previous state held, and so even if the reloaded parameter is true, we should not set CONFIG_FLAG_FILEUNCHANGED.</pre>
<br />
<p>- Mark</p>
<br />
<p>On December 17th, 2014, 5:24 p.m. UTC, George Joseph wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Joshua Colp.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated Dec. 17, 2014, 5:24 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">The earlier patch fixed the test breakage but caused a problem when res_pjsip_config_wizard is unloaded then loaded again.
When res_pjsip_config_wizard is unloaded, config still has the mtime of pjsip_wizard.conf cached. When the module is loaded again, a reload of res_pjsip is triggered which causes the observer to do a reload as opposed to a load of the file. The result is FILEUNCHANGED and the wizard never loads any objects. The file has to be cached for normal operation so the easiest solution was to expose the config cache_remove function and call it when res_pjsip_config_wizard is unloaded. Now if res_pjsip_config_wizard is loaded, it gets a good config instead of FILEUNCHANGED even though it's a reload.
</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;">Tested the unloading and re-loading of the module as well as config reload and pjsip reload to make sure the correct objects were updated.
</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>branches/13/res/res_pjsip_config_wizard.c <span style="color: grey">(429698)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4276/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>