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

gareth reviewboard at asterisk.org
Sun Jun 24 22:25:05 CDT 2012 


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, lines 15220-15221
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line15220>
> >
> >     Just another bit of inappropriate white space that you got near.

Fixed


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, lines 16227-16229
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line16227>
> >
> >     Since you are changing this, you might as well make it fit the style guidelines.
> >     
> >     Remove the trailing white space, add braces to the condition.

Fixed


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, lines 16531-16542
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line16531>
> >
> >     You have changes that are pretty close to these blobgs... might as well clean up the white space.

Fixed


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, line 26180
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line26180>
> >
> >     Remove the extra white space.

Fixed


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, lines 16508-16514
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line16508>
> >
> >     I don't really see why this kind of minor optimization is being grouped with some of the other stuff in this patch. I'm also not to fond of variables with less than useful names like 'ptr' for a character pointer... though if it's going to be recycled like this, there probably isn't a real problem with that. Maybe something like delimiter_ptr or something would be more appropriate than ptr.
> 
> Mark Michelson wrote:
>     I agree, saving 4-8 bytes of stack space is not "cleaning up" the code. Giving a variable like 'ptr' yet another purpose in the function just makes things harder to follow. Not immensely harder, mind you, but still it makes it harder to pin down the purpose of specific variables.
> 
> Mark Michelson wrote:
>     However, the added " > refer->referred_by_name" is useful since it prevents potentially writing a null byte in an invalid place.

This changed wasn't made to save stack space. The rest of get_refer_info uses ptr (some 8+ instances) as a temporary variable when splitting up the string, so the code was changed to match the rest of the function.


> On June 21, 2012, 3:48 p.m., jrose wrote:
> > /trunk/channels/chan_sip.c, lines 7453-7459
> > <https://reviewboard.asterisk.org/r/1993/diff/1/?file=28966#file28966line7453>
> >
> >     This seems to be a minor optimization (to avoid redeclaring the character pointer) that doesn't really have anything to do with other stuff in the patch.  I'd recommend splitting optimizations like this into a separate patch.
> 
> Mark Michelson wrote:
>     I'm going to step in here and say that I find the idea of taking a function and moving it to be in-line is a step backwards when it comes to cleaning up code. Even if get_body_by_line is a simple if check, as you say in the description, it helps to have it in a function instead of having a somewhat cryptic if statement in-line.

I don't think the test is that cryptic, I can put a commment above the test to explain what is doing if that would help.

get_sdp_line, located inbetween get_sdp_iterate and get_content_line has a similar loop body.


- gareth


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


On June 19, 2012, 10:08 p.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1993/
> -----------------------------------------------------------
> 
> (Updated June 19, 2012, 10:08 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/cea9748d/attachment-0001.htm>

More information about the asterisk-dev mailing list