[asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.

[rmudgett]

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


A once over pass thourgh of the patch.

You should go through and fixup any coding guidelines problems in the changes you have made.


branches/12/include/asterisk/chanvars.h
<https://reviewboard.asterisk.org/r/3970/#comment23834>

    Protect these macros with the "do {} while (0)" trick.
    
    I'm not sure of the usefulness of these AST_VAR_LIST_xxx macros.  You are simply hiding the if test which is for the most part already in the code.



branches/12/include/asterisk/phoneprov.h
<https://reviewboard.asterisk.org/r/3970/#comment23828>

    You should extern this declaration in the header and initialize it in a c file.  As it is, every file that includes this header has an initialized copy.



branches/12/main/chanvars.c
<https://reviewboard.asterisk.org/r/3970/#comment23835>

    Rather than a relatively expensive safe traversal do this:
    
    while ((var = AST_LIST_REMOVE_HEAD()) {
      ast_var_delete(var);
    }



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23829>

    This is an interesting way of doing the standard ao2 container callback functions.
    
    Did you fix the formatting from the wiki page.  Directly cutting and pasting the template function from the wiki gives you spaces instead of tabs.



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23831>

    curly on its own line



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23832>

    No need for the backslash on this line as it ends the macro.



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23833>

    idem



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23837>

    if test is redundant because you defined AST_VAR_LIST_INSERT_TAIL to have the if test inside it.
    
    The next 7 changes are the same as here.
    
    I looks like just about everywhere you used the new macros that there is now a redundant if test.



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23839>

    curlies



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23840>

    Is the cast necessary?



branches/12/res/res_phoneprov.c
<https://reviewboard.asterisk.org/r/3970/#comment23841>

    privider is ref leaked
    
    Use:
    for (; (provider = ao2_it_next()); ao2_ref(provider, -1))


- rmudgett


On Sept. 18, 2014, 3:42 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3970/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2014, 3:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The big piece missing for me to finally transition to pjsip was the ability to mirror the auto provisioning features of res_phoneprov.  The first step (this patch) is to make res_phoneprov more modular so other modules (like pjsip) can provide configuration information instead of res_phoneprov relying solely on users.conf and sip.conf.  To accomplish this a new ast_phoneprov public API is now exposed which allows config providers to register themselves, set defaults (server profile, etc) and add user extensions.
> 
> ast_phoneprov_provider_register registers the provider and provides callbacks for loading default settings and loading users.
> ast_phoneprov_provider_unregister clears the defaults and users.
> ast_phoneprov_add_extension should be called once for each user/extension by the provider's load_users callback to add them.
> ast_phoneprov_delete_extension deletes one extension.
> ast_phoneprov_delete_extensions deletes all extensions for the provider.
> 
> res_phoneprov actually registers itself as the provider for sip/users and is always available and is the default.
> 
> Writing a new provider...
> Since res_phoneprov is also it's own provider, examples of what a new provider would have to do are in load_users() in res_phoneprov.c.  Those functions gather the information from users.conf and sip.conf and call the ast_provider_register and ast_phoneprov_add_extension apis.
> 
> So...
> The provider creates a callback function which calls the ast_phoneprov_add_extension api for each user.  
> It then calls ast_phoneprov_provider_register with the callback.
> res_phoneprov then calls the callback to cause the actual load.
> During normal http server ops, all work is done by res_phoneprov and the provider is never called again unless a reload is needed.
> If the provider wants to reload it can simply unregister and reregister or it can call its own load_users callback.
> If res_phoneprov wants to reload, it iterates over its registry and calls the providers callback.
> 
> NOTE:  If res_phoneprov is actually unloaded, it has no way to know what providers were registered (other than itself) so a subsequent load will have nothing but it's own users.  
> 
> Additional changes...
> I added a few convenience functions to chanvars for creating lists and finding and deleting entries.  No existing code was touched.
> 
> Next steps...
> A provider for res_pjsip.
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_phoneprov.exports.in PRE-CREATION 
>   branches/12/res/res_phoneprov.c 422737 
>   branches/12/main/chanvars.c 422737 
>   branches/12/include/asterisk/phoneprov.h PRE-CREATION 
>   branches/12/include/asterisk/chanvars.h 422737 
>   branches/12/configs/phoneprov.conf.sample 422737 
> 
> Diff: https://reviewboard.asterisk.org/r/3970/diff/
> 
> 
> Testing
> -------
> 
> I ran through several scenarios including the use of PP_EACH_USER and PP_EACH_EXTENSION to make sure that all existing functionality was preserved.  I actually use it with Grandstream phones and everything worked exactly as expected.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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