[asterisk-dev] [Code Review]: chan_sip code cleanup

[Leif Madsen]

> On June 22, 2012, 5:08 p.m., Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 11098-11100
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line11098>
> >
> >     Would I be correct in saying this is a bug fix rather than a piece of code cleanup? If so, this should probably be committed to other branches of Asterisk besides just trunk.
> 
> gareth wrote:
>     The code was modified because Cisco phones require an Expires header to be present.
>     
>     Looking at RFC3903 section 6.6 says "The response MUST contain an Expires header field indicating the expiration interval chosen by the ESC". So I would say that is a bug.

Could you break out the bug fixes into separate reviews? Code cleanup I don't think should be making any behaviour changes like bug fixes. Those should be reviewed and committed independently.


- Leif


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


On June 24, 2012, 10:27 p.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1993/
> -----------------------------------------------------------
> 
> (Updated June 24, 2012, 10:27 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is the first part split out from the much larger patch at https://reviewboard.asterisk.org/r/1976/ - it contains just the various chan_sip code cleanups.
> 
> - struct sip_refer converted to use the stringfields API.
> 
> - sip_{refer|notify}_allocate -> sip_{notify|refer}_alloc to match other *alloc functions.
> 
> - Replace get_msg_text, get_msg_text2 and get_pidf_body -> No, not get_pidf_msg_text_body3 but get_content, to match add_content.
> 
> - get_body doesn't get the request body, renamed to get_content_line.
> 
> - get_body_by_line doesn't get the body line, and is just a simple if test. Moved code inline and removed function.
> 
> - Remove camelCase in struct sip_peer peer state variables, onHold -> onhold, inUse -> inuse, inRinging -> ringing.
> 
> - Remove camelCase in struct sip_request rlPart1 -> rlpart1, rlPart2 -> rlpart2.
> 
> - Rename instances of pvt->randdata to pvt->nonce because that is what it is, no need to update struct sip_pvt because _it already has a nonce field_.
> 
> - Removed struct sip_pvt randdata stringfield.
> 
> - Remove useless (and inconsistent) 'header' suffix on variables in handle_request_subscribe.
> 
> - Use ast_strdupa on Event header in handle_request_subscribe to avoid overly complicated strncmp calls to find the event package.
> 
> - Move get_destination check in handle_request_subscribe to avoid duplicate checking for packages that don't need it.
> 
> - Move extension state callback management in handle_request_subscribe to avoid duplicate checking for packages that don't need it.
> 
> - Remove duplicate append_date prototype.
> 
> - Rename append_date -> add_date to match other add_xxx functions.
> 
> - Added add_expires helper function, removed code that manually added expires header.
> 
> - Remove _header suffix on add_diversion_header (no other header adding functions have this).
> 
> - Don't pass req->debug to request handle_request_XXXXX handlers if req is also being passed.
> 
> - Don't pass req->ignore to check_auth as req is already being passed.
> 
> - Don't create a subscription in handle_request_subscribe if p->expiry == 0.
> 
> - Don't walk of the back of referred_by_name when splitting string in get_refer_info
> 
> - Remove duplicate check for no dialog in handle_incoming when sipmethod == SIP_REFER, handle_request_refer checks for that.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 369065 
>   /trunk/channels/sip/include/sip.h 369065 
>   /trunk/channels/sip/security_events.c 369065 
> 
> Diff: https://reviewboard.asterisk.org/r/1993/diff
> 
> 
> Testing
> -------
> 
> It compiles.
> 
> 
> Thanks,
> 
> gareth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120625/8452fb39/attachment.htm>