[asterisk-dev] [Code Review] 3043: pjsip: CLI commands

[Kevin Harwell]

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



branches/12/main/config.c
<https://reviewboard.asterisk.org/r/3043/#comment19707>

    I think this might be a memory leak.  I don't see where it is being released.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19710>

    Make this return a const char * since the string can't be modified.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19711>

    format_string and separator are being allocated, but never freed.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19712>

    print_name is not destroyed.  One option would be to declare it as RAII_VAR(char *, print_name, NULL, ast_free).  This would automatically free the memory when it goes out of scope.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19715>

    I believe the endpoint_snapshot needs to be unreffed once it is no longer needed.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19713>

    print_name is allocated, but not released.



branches/12/res/res_pjsip/pjsip_configuration.c
<https://reviewboard.asterisk.org/r/3043/#comment19714>

    This will cause problems if the res_pjsip_endpoint_identifier_ip module is not loaded.  I ran into the same issue with AMI.  For endpoints I added an ast_sip_endpoint_formatter object and associate register functions, etc...(pjsip.[ch]) that contains a call to format AMI output for registered formatters.  You could do the same thing here but add another callback to the object, for instance "format_cli" that would work for CLI.  See also the "endpoint_identify_formatter" in the module.



branches/12/res/res_pjsip_endpoint_identifier_ip.c
<https://reviewboard.asterisk.org/r/3043/#comment19708>

    It looks like fields is never being properly cleaned up after creating the memory.  Actually this could probably just be created as a stack variable.


- Kevin Harwell


On Dec. 5, 2013, 12:27 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3043/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2013, 12:27 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22610
>     https://issues.asterisk.org/jira/browse/ASTERISK-22610
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Implements the following cli commands:
> pjsip list aors
> pjsip list auths
> pjsip list channels
> pjsip list contacts
> pjsip list endpoints
> pjsip show aor(s)
> pjsip show auth(s)
> pjsip show channels
> pjsip show endpoint(s)
> 
> Also...
> Minor modifications made to the AMI command implementations to facilitate reuse.
> New function ast_variable_list_sort added to config.c and config.h to implement variable list sorting.
> New api ast_sip_for_each_identify added to module res_pjsip_endpoint_identifier_ip. Required new files res_pjsip_endpoint_identifier_ip.h and res_pjsip_endpoint_identifier_ip.exports.in.
> 
> Implementation of the summary statistics is still pending.
> I'm sure there will be lots of feedback here. :)
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_pjsip_registrar.c 403390 
>   branches/12/res/res_pjsip_endpoint_identifier_ip.exports.in PRE-CREATION 
>   branches/12/res/res_pjsip_endpoint_identifier_ip.c 403390 
>   branches/12/res/res_pjsip/pjsip_configuration.c 403390 
>   branches/12/res/res_pjsip/location.c 403390 
>   branches/12/res/res_pjsip/include/res_pjsip_private.h 403390 
>   branches/12/main/config.c 403390 
>   branches/12/include/asterisk/res_pjsip_endpoint_identifier_ip.h PRE-CREATION 
>   branches/12/include/asterisk/res_pjsip.h 403390 
>   branches/12/include/asterisk/config.h 403390 
> 
> Diff: https://reviewboard.asterisk.org/r/3043/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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