[asterisk-dev] [Code Review] 3587: chan_pjsip: PJSIPNotify - strip content-length headers, add documentation.

[Mark Michelson]

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



/branches/12/res/res_pjsip_notify.c
<https://reviewboard.asterisk.org/r/3587/#comment22025>

    You've done a good job to plug some of the memory leaks that were previously present, but there are still some here.
    
    At a minimum, you'll need to call ast_variables_destroy(first_header) in the INVALID_ENDPOINT and ALLOC_ERROR cases in the switch statement.
    
    However, I think the addition of the first_header variable list is adding unnecessary allocations to the process. My suggestion is to do the following:
    1) Get rid of the first_header and header variables.
    2) Get rid of the for loop you added. If you want to warn about an unnecessary Content-Length header, you can add the check and warning message to the for loop in build_ami_notify(). IMO, the message shouldn't be a warning, but a notice.
    3) Get rid of the ast_variables_destroy(vars) call you added above the switch statement, as well as the setting of vars to NULL afterwards.
    4) Pass vars as the second argument to push_notify()
    5) Call ast_variables_destroy(vars) in the INVALID_ENDPOINT and ALLOC_ERROR cases in the switch statement.


- Mark Michelson


On June 5, 2014, 6:19 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3587/
> -----------------------------------------------------------
> 
> (Updated June 5, 2014, 6:19 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> There was some confusion on usage of the PJSIPNotify manager command... notably in how headers are added to the notify message. This patch adds documentation explaining how to add headers and also adds some of the argument vetting from the chan_sip variant (SIPNotify).
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_pjsip_notify.c 415187 
> 
> Diff: https://reviewboard.asterisk.org/r/3587/diff/
> 
> 
> Testing
> -------
> 
> Send some notifies with in the documented manner.  Added Content + Content-type + Content-length variables to test how that worked.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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