This is an automatically generated e-mail. To reply, visit:

A once over pass thourgh of the patch.

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


    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.


    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.


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


    This is an interesting way of doing the standard ao2 container callback 
    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.


    curly on its own line


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




    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.




    Is the cast necessary?


    privider is ref leaked
    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

-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:

Reply via email to