[Home]

Summary:ASTERISK-12520: Did a code review and found a few issues
Reporter:Jon Hornstein (jon hornstein)Labels:
Date Opened:2008-08-04 22:39:38Date Closed:2008-08-05 21:54:34
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:Could someone verify the following "soft" issues
Comments:By: Russell Bryant (russell) 2008-08-05 07:12:57

If you have code to include, please include it as an attachment.  Also, please verify that this is code that you wrote.  Please include a description of what you think is "wrong" here, as well.  Finally, is there a reason this issue is marked private?
By: Jon Hornstein (jon hornstein) 2008-08-05 21:00:41

I'm new to logging in code into Digium and I don't have anyone I can code review with, hence not to appear like an idiot I wanted to do this privately.

I have 4 issues

* looks like a bug (in threadstorage.h)
   changing:
__ast_threadstorage_object_add(buf, init_size, file, function, line);
  to:
__ast_threadstorage_object_add(ts->key, init_size, file, function, line);


* Suggestion for coding changes that would make compiling with less warnings (utils.h)

   Allow for vargs

   change:
#define ast_asprintf(a,b,c) asprintf(a,b,c)
#define ast_vasprintf(a,b,c) vasprintf(a,b,c)
   to:
#define ast_asprintf(a,b,...) asprintf(a,b,__VA_ARGS__)
#define ast_vasprintf(a,b,...) vasprintf(a,b,__VA_ARGS__)

* Cisco default payload number for ILBC is 116 which although is configurable ona cisco box never the less causes the stream not to pass out the cisco box. This payload number is configurable but 97 is not the default setup. (rtp.c)

       change and reorder:
[97] = {1, AST_FORMAT_ILBC},
       to:
[116] = {1, AST_FORMAT_ILBC},

* In function ast_variables_destroy (config.c) the free function was called when the symmetric ast_free (current effectively the same function) should be called. The variable was created with ast_calloc.
By: Tilghman Lesher (tilghman) 2008-08-05 21:50:52

I'll give you the first two, but the last two are incorrect.

If Cisco is hardcoding iLBC to RTP assignment 116, they are in violation of IANA RTP payload assignment guidelines.  The standard is that RTP payloads in the range of 96 through 127 are dynamic and are thus governed by the rules in RFC 3551.  Further, RFC 3551 states in section 11 that all codecs not explicitly defined in the standard will not have static assignments.  Therefore, any assignment of iLBC to a static payload number is in direct violation of the standard.

And, as you correctly pointed out, ast_free and free are exactly the same function, and no change is warranted.  If you have further questions about this matter, please familiarize yourself with the coding guidelines document located in the doc/ subdirectory of the source tree, and redirect further questions on internal code to the asterisk-dev list.
By: Digium Subversion (svnbot) 2008-08-05 21:54:33

Repository: asterisk
Revision: 135899

U   branches/1.4/include/asterisk/threadstorage.h
U   branches/1.4/include/asterisk/utils.h

------------------------------------------------------------------------
r135899 | tilghman | 2008-08-05 21:54:32 -0500 (Tue, 05 Aug 2008) | 4 lines

1) Bugfix for debugging code
2) Reduce compiler warnings for another section of debugging code
(Closes issue ASTERISK-12520)

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

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