[Home]

Summary:ASTERISK-04337: [patch] chan_sip.c misc code cleanup
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-06-03 11:29:25Date Closed:2008-01-15 15:38:23.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) blanks.diff
( 1) sip_cleanup.diff
Description:This patch removes several instances of cut&pasted or inefficient string
manipulation code, and fixes some issues found in the code on passing.

In detail:

+ introduce skip_blanks() trim_blanks() skip_nonblanks() functions that
 deal with leading/trailing blank/nonblank substrings. These function
 are then used to replace several hand-rolled versions of the same code.
 XXX There are actually more replicas of this code in other files,
 perhaps these function would deserve a place in utils.c

+ introduce function get_in_brackets() which, when given a string
 of the form "bla bla bla <some text> foo" returns a pointer to
 the text in brackets trimming out the rest. The function is then used
 to replace 4 hand-rolled copies of the same code.
 XXX same as above about possible wider use of the function.

+ rewrite url_decode() in what i think is a more readable format.

+ remove sip_unescape_uri() and dependent functions, as it does exactly
 the same of url_decode() but with O(N^2) instead O(N) complexity.

+ introduce find_alias() to lookup aliases of sip headers. While only
 used once, it makes the body of __get_header() a lot more readable.

+ rewrite __get_header() to remove extra replicas of the code and a
 recursive call when only an extra iteration was needed.

+ mark with XXX some code issues.


****** ADDITIONAL INFORMATION ******

the patch might not apply cleanly as it is part of much larger changes,
so i had to hand-edit it, and the line offsets might be large.
Please bear with it.
Comments:By: Olle Johansson (oej) 2005-06-03 11:51:49

Please spend some time to make it work with a clean copy of chan_sip.c from cvs head. It's easier and better for you to do it, since you know your own code. Thank you!
By: Olle Johansson (oej) 2005-06-03 11:53:08

For all functions, add comments like we have in the rest of chan_sip.
By: Olle Johansson (oej) 2005-06-03 11:55:19

This improves readability of chan_sip a lot. Great stuff when you've fixed it!
By: Olle Johansson (oej) 2005-06-04 07:08:14

Waiting for update...
By: Luigi Rizzo (rizzo) 2005-06-07 09:09:55

update dowloaded. Compiles fine with 20050607 sources.

Please note that the functions ast_*_blanks() have been
added to utils.c (and utils.h) because they are of very general use,
although so far I have only patched chan_sip.c

When this is in, it will be possible to update the other files as well.
I am not sure about get_in_brackets() or url_decode() -- how general
is this kind of parsing in other protocols ?
By: Olle Johansson (oej) 2005-06-07 09:29:07

I am coming up with url_encode as well. Might need your help optimizing it. url_encode/decode might be useful for app_curl... get_in_brackets is propably very chan_sip specific.
By: Luigi Rizzo (rizzo) 2005-06-12 15:25:13

any progress with ASTERISK-4337 ? I have updated it (file blank.diff)
almost a week ago to apply cleanly to HEAD but it seems
to have stalled...
By: Mark Spencer (markster) 2005-06-17 08:58:35

Some of these routines (e.g. ast_skip_blanks, ast_trim_blanks, ast_skip_nonblanks) should be implemented as static inlines.  Also, ast_trim_blanks should likely be modified to not operate before the beginning of a string (e.g. think about what happens if you call ast_skip_blanks("")).
By: Kevin P. Fleming (kpfleming) 2005-06-17 09:24:52

Committed to CVS HEAD with some documentation and naming fixes, thanks!
By: Kevin P. Fleming (kpfleming) 2005-06-17 10:27:26

I've modified ast_trim_blanks to not run off the beginning of the string, nor to try operate on zero-length strings.
By: Luigi Rizzo (rizzo) 2005-06-17 11:19:00

kevin, thanks for the fixes to ast_trim_blanks,
but why do you remove the comments from the C source ?
They are always useful even if they may look trivial, because they document
the intended behaviour of the functions, and may help in spotting bugs in
the actual implementation.

In fact, in this case the code that I submitted for ast_skip_blanks() _is_ buggy, because it does not accept a NULL argument, despite what the comments
in the header and the source file said. With the comment in the C source,
a casual reader could easily spot the issue. Without it, it becomes less
likely because you have to look up the header file.

The fix is just one line at the beginning of ast_skip_blanks (plus indentation)
   if (str != NULL)
By: Mark Spencer (markster) 2005-06-18 06:56:53

I still believe this should be implemented as a static inline function (unless -DLOW_MEMORY is enabled in the Makefile).
By: Mark Spencer (markster) 2005-06-18 07:05:53

The "trim_blanks" code still seems horribly wrong...  We trim off the trailing spaces and then we point just past the trailing 0?

Also, the check for && *work is not necessary, it's the work >= str that is necessary.
By: Mark Spencer (markster) 2005-06-18 07:53:51

Since there are crashes related to this, I've gone ahead and put in some fixes.  Hopefully this will take care of it.
By: Digium Subversion (svnbot) 2008-01-15 15:38:23.000-0600

Repository: asterisk
Revision: 5924

U   trunk/channels/chan_sip.c
U   trunk/include/asterisk/utils.h
U   trunk/utils.c

------------------------------------------------------------------------
r5924 | kpfleming | 2008-01-15 15:38:23 -0600 (Tue, 15 Jan 2008) | 2 lines

string/whitespace handling cleanups (bug ASTERISK-4337, with mods)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5924