[asterisk-dev] [Code Review] 4311: res_pjsip: make it unloadable

[Mark Michelson]

> On Jan. 5, 2015, 10:47 p.m., Mark Michelson wrote:
> > branches/13/res/res_pjsip/include/res_pjsip_private.h, lines 123-141
> > <https://reviewboard.asterisk.org/r/4311/diff/1/?file=70133#file70133line123>
> >
> >     I unnastand the need for these functions, but I feel like they could be named more appropriately.
> >     
> >     First, due to a lack of proper namespacing in C, you have to be careful defining functions that are called pjsip_*. It is possible that such a function could be added to PJSIP itself and cause a conflict with us. Note that this applies to other functions besides the ones I've highlighted here.
> >     
> >     Second, the names do not do a great job of differentiating between the ast_sip_* versions of the functions. This may be hard to describe in a brief function name.
> 
> Kevin Harwell wrote:
>     How does sip_register_service_no_ref sound?  I'd like to avoid prefixing it with "ast_" since that is usually reserved for exported functions and these are internal.
>     
>     if sip_register_blah is too generic then maybe internal_sip_register?  Thoughts/suggestions?

The "internal" version sounds fine to me.


- Mark


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


On Jan. 2, 2015, 7:46 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4311/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2015, 7:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24485
>     https://issues.asterisk.org/jira/browse/ASTERISK-24485
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The res_pjsip module was previously unloadable. With this patch it can now be unloaded.
> 
> This patch is based off the original patch on the issue by Corey Farrell with a few modifications. Removed a few changes not required to make the module unloadable and also fixed a bug that would cause asterisk to crash on unloading.
> 
> 
> Diffs
> -----
> 
>   branches/13/res/res_pjsip/pjsip_outbound_auth.c 430178 
>   branches/13/res/res_pjsip/pjsip_options.c 430178 
>   branches/13/res/res_pjsip/pjsip_global_headers.c 430178 
>   branches/13/res/res_pjsip/pjsip_distributor.c 430178 
>   branches/13/res/res_pjsip/pjsip_configuration.c 430178 
>   branches/13/res/res_pjsip/location.c 430178 
>   branches/13/res/res_pjsip/include/res_pjsip_private.h 430178 
>   branches/13/res/res_pjsip/config_transport.c 430178 
>   branches/13/res/res_pjsip/config_auth.c 430178 
>   branches/13/res/res_pjsip.c 430178 
>   branches/13/main/stasis_message_router.c 430178 
> 
> Diff: https://reviewboard.asterisk.org/r/4311/diff/
> 
> 
> Testing
> -------
> 
> Made it so res_pjsip was the only pjsip module loaded and then issued an unload and noted it unloaded successfully (also loaded/unloaded it several times from the CLI). Also when loaded and with REF_DEBUG enabled issued a "core stop gracefully" and made sure there were no ref leaks for the module.
> 
> Also tested unloading with other dependent pjsip modules loaded and noted that the module would not unload (as it should since dependencies are currently loaded). And then shutdown asterisk and made sure it did not crash or anything.
> 
> Started asterisk with nominal and off nominal module and pjsip configurations to make sure things behaved appropriately (no crashes and such) and then attempted to, or successfully unload the res_pjsip module. Also made sure Asterisk continued to shutdown without incident.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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