[asterisk-dev] [Code Review] 4276: res_pjsip_config_wizard: Update to earlier patch for fix test breakage.

[Mark Michelson]

> On Dec. 17, 2014, 8:51 p.m., Mark Michelson wrote:
> > 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?
> 
> George Joseph wrote:
>     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.
>     
>

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.


- Mark


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


On Dec. 17, 2014, 5:24 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4276/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2014, 5:24 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   branches/13/res/res_pjsip_config_wizard.c 429698 
> 
> Diff: https://reviewboard.asterisk.org/r/4276/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141217/bb591b81/attachment.html>