Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Oct. 9, 2014, 12:41 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 424963 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 424175 branches/12/main/chanvars.c 424175 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 424175 branches/12/configs/phoneprov.conf.sample 424175 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/#review13404 --- branches/12/include/asterisk/phoneprov.h https://reviewboard.asterisk.org/r/3970/#comment23888 Document the return value for this typedef. branches/12/include/asterisk/phoneprov.h https://reviewboard.asterisk.org/r/3970/#comment23889 This documentation conflicts with the prototype. branches/12/main/chanvars.c https://reviewboard.asterisk.org/r/3970/#comment23890 Note that there should be an empty line after declarations. Apply this below as well. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23891 s/whi/who/ branches/12/res/res_phoneprov.exports.in https://reviewboard.asterisk.org/r/3970/#comment23893 This is unnecessary since there are no symbols that match it. - opticron On Sept. 18, 2014, 7:01 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, 7:01 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 423501 branches/12/main/chanvars.c 423501 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 423501 branches/12/configs/phoneprov.conf.sample 423501 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 30, 2014, 10:02 a.m.) Review request for Asterisk Developers. Changes --- Addressed opticron's comments. 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 (updated) - branches/12/res/res_phoneprov.exports.in PRE-CREATION branches/12/res/res_phoneprov.c 424175 branches/12/main/chanvars.c 424175 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 424175 branches/12/configs/phoneprov.conf.sample 424175 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/#review13410 --- branches/12/include/asterisk/chanvars.h https://reviewboard.asterisk.org/r/3970/#comment23912 guidelines: add space: while (0) branches/12/include/asterisk/chanvars.h https://reviewboard.asterisk.org/r/3970/#comment23913 idem branches/12/main/chanvars.c https://reviewboard.asterisk.org/r/3970/#comment23910 void is needed between the parens to match the protototype. This is C not C++ branches/12/main/chanvars.c https://reviewboard.asterisk.org/r/3970/#comment23911 guidelines: space between while and paren branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23931 Please change this to take the name of the hash function instead of doing a token pasting. If I didn't know about this macro or how it took parameters, it would be difficult to find the function definition given the function name where the continer is allocated. I actually did have some difficulty finding the function. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23932 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23914 This could be done the same way you did others: AST_VAR_LIST_INSERT_TAIL(profile-headp, ast_var_assign(args.varname, args.varval)); This would also eliminate the variable variable. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23919 Existing bug: The stringfields in exten are not freed. ast_string_field_free_memory(exten) should call: delete_extension(exten) branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23920 Remove these two unref_profile lines. They cause an unbalance in the profile ref count. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23921 OBJ_SEARCH_OBJECT branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23922 ast_free is NULL tolerant branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23923 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23924 The handling of phoneprov_cfg is incorrect. When ast_config_load() returns you have to check for the special pointer values: #define CONFIG_STATUS_FILEUNCHANGED (void *)-1 #define CONFIG_STATUS_FILEINVALID (void *)-2 before you can treat phoneprov_cfg as a normal pointer that ast_config_destroy() can handle. Passing those special pointers to ast_config_destroy() will cause a crash. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23925 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23926 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23928 guidelines: Function start curly on its own line branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23927 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23929 idem branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23930 Check for failure - rmudgett On Sept. 30, 2014, 11:02 a.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 30, 2014, 11:02 a.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
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 30, 2014, 1:41 p.m.) Review request for Asterisk Developers. Changes --- Addressed Richard's comments. 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 (updated) - branches/12/res/res_phoneprov.exports.in PRE-CREATION branches/12/res/res_phoneprov.c 424175 branches/12/main/chanvars.c 424175 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 424175 branches/12/configs/phoneprov.conf.sample 424175 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/#review13416 --- Ship it! Minor nit I'm hesitant about this going into v12 and v13 at this late a date especially since v13 is a LTS and currently feature frozen. branches/12/res/res_phoneprov.c https://reviewboard.asterisk.org/r/3970/#comment23939 ; ?? - rmudgett On Sept. 30, 2014, 2:41 p.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 30, 2014, 2:41 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 424175 branches/12/main/chanvars.c 424175 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 424175 branches/12/configs/phoneprov.conf.sample 424175 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
On Sept. 30, 2014, 1:57 p.m., rmudgett wrote: Minor nit I'm hesitant about this going into v12 and v13 at this late a date especially since v13 is a LTS and currently feature frozen. I know but I think anyone who uses the current phoneprov implementation (including me) won't be able to migrate to pjsip without the refactor and the new module. I just didn't have a chance to work on it before the cutoff. I'll fix the stray semicolon before I commit. - George --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/#review13416 --- On Sept. 30, 2014, 1:41 p.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 30, 2014, 1:41 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 424175 branches/12/main/chanvars.c 424175 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 424175 branches/12/configs/phoneprov.conf.sample 424175 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 18, 2014, 2:42 p.m.) Review request for Asterisk Developers. Changes --- Updated summary Repository: Asterisk Description (updated) --- 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
---
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
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
On Sept. 18, 2014, 4:11 p.m., rmudgett wrote: branches/12/res/res_phoneprov.c, lines 128-129 https://reviewboard.asterisk.org/r/3970/diff/4/?file=67281#file67281line128 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. Yep, I reformatted it after pasting it. On Sept. 18, 2014, 4:11 p.m., rmudgett wrote: branches/12/res/res_phoneprov.c, line 983 https://reviewboard.asterisk.org/r/3970/diff/4/?file=67281#file67281line983 Is the cast necessary? Yes. The compiler complains note: expected ‘void *’ but argument is of type ‘ast_string_field’ - George --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/#review13348 --- On Sept. 18, 2014, 6:01 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, 6:01 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 423501 branches/12/main/chanvars.c 423501 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 423501 branches/12/configs/phoneprov.conf.sample 423501 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 18, 2014, 6:01 p.m.) Review request for Asterisk Developers. Changes --- Addressed Richard's comments. 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 (updated) - branches/12/res/res_phoneprov.exports.in PRE-CREATION branches/12/res/res_phoneprov.c 423501 branches/12/main/chanvars.c 423501 branches/12/include/asterisk/phoneprov.h PRE-CREATION branches/12/include/asterisk/chanvars.h 423501 branches/12/configs/phoneprov.conf.sample 423501 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 5, 2014, 6:01 p.m.) Review request for Asterisk Developers. Changes --- Removed the load_defaults process. It's actually easier to let the provider load and apply it's own defaults. Cleaned up some error messages. Added language to phoneprov.conf.sample concerning defaults and providers. Repository: Asterisk Description (updated) --- 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. Since res_phoneprov is a module that may or may not be loaded, the apis are implemented as inlines that check that res_phoneprov is actually loaded before calling the concrete function. A new include file was added: include/asterisk/phoneprov.h 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_defaults() and load_users() in res_phoneprov.c. Those functions gather the information from users.conf and sip.conf and call the ast_provider_set_defaults 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 (updated) - 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3970: res_phoneprov: Refactor phoneprov to allow pluggable config providers.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3970/ --- (Updated Sept. 3, 2014, 6:28 p.m.) Review request for Asterisk Developers. Changes --- Addressed some issues I found while writing the pjsip provider and removed some silliness around testing if the module is loaded. 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_provider_set_defaults should be called by the provider's load_defaults callback to push the defaults. 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. Since res_phoneprov is a module that may or may not be loaded, the apis are implemented as inlines that check that res_phoneprov is actually loaded before calling the concrete function. A new include file was added: include/asterisk/phoneprov.h 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_defaults() and load_users() in res_phoneprov.c. Those functions gather the information from users.conf and sip.conf and call the ast_provider_set_defaults and ast_phoneprov_add_extension apis. So... The provider creates 2 callback functions which call the ast_provider_set_defaults and ast_phoneprov_add_extension apis. It then calls ast_phoneprov_provider_register with those 2 callbacks. res_phoneprov then calls the callbacks 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_defaults and load_users callbacks. If res_phoneprov wants to reload, it iterates over its registry and calls the providers callbacks. 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 (updated) - branches/12/res/res_phoneprov.c 422556 branches/12/main/chanvars.c 422556 branches/12/include/asterisk/chanvars.h 422556 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: http://lists.digium.com/mailman/listinfo/asterisk-dev
