[Home]

Summary:ASTERISK-04251: [patch] various patches for a cleaner compile with -Werror
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-05-21 04:38:14Date Closed:2011-06-07 14:10:46
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 4353.stable.patch.txt
( 1) werror.patch
Description:Trying to compile with -Werror identifies a number of minor problems in the
code that this patch tries to fix.

app.c
  use of  *ptr++ instead of ptr++

config.c
  wrong argument type (possibly os-dependent) in snprintf

apps/app_controlplayback.c
  multiple uses of  *var++  instead of  var++

apps/app_dictate.c
  missing prototype for mkdir;

apps/app_flash.c
  wrong include order causing ELAST redefinition in system's header

channels/chan_zap.c
  PRI debugging variables declared outside the PRI conditional block

channels/iax2-parser.c
  some versions of gcc e.g. 2.95.x on FreeBSD do not recognise %F as a
  valid strftime format. Maybe a bug in the compiler. However it costs
  nothing to replace it with the recognised and equivalent form.

codecs/gsm/src/preprocess.c
codecs/gsm/src/short_term.c
  ifdef out some unused variables

include/asterisk/utils.h
  ifdef out the inet_ntoa redefinition, causes clashes with system headers
  (maybe a better solution is to change the include order).

res/Makefile
  remove -Werror when compiling res_crypto.c, too many warnings otherwise.
  (probably due to some missing headers, but no time to investigate it now)

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

disclaimer on file.
Comments:By: Kevin P. Fleming (kpfleming) 2005-05-21 13:56:58

I'm OK with most of this, except for these issues:

1) This is not a failing of _all_ GCC, only those that do not implement C99, I believe. A more specific comment may be in order.

- strftime(output, maxlen, "%F  %T", &tm);
+ /* XXX gcc does not like %F so use %Y-%m-%d */
+ strftime(output, maxlen, "%Y-%m-%d  %T", &tm);

2) Why are you suppressing this?

+#if 0
#ifdef inet_ntoa
#undef inet_ntoa
#endif
#define inet_ntoa __dont__use__inet_ntoa__use__ast_inet_ntoa__instead__
+#endif

3) What is the need for this?

+res_crypto.o: res_crypto.c
+ echo "--- compiling crypto.c "
+ $(CC) -c ${CFLAGS:-Werror=} $<
By: Luigi Rizzo (rizzo) 2005-05-21 15:58:00

The changes are not dependent on each other so you can just
take those you like. There are actually a few more that I omitted,
related to format strings which are not correctly recognised, and
I am not sure if they are due to my compiler or a real problem in the code.

Response to your comments:

1) maybe you can use the comment in the description above,
/*  some versions of gcc e.g. 2.95.x on FreeBSD do not recognise %F as a
*  valid strftime format. Maybe a bug in the compiler. However it costs
*  nothing to replace it with the recognised and equivalent form. %Y-%m-%d
*/

2) the inet_ntoa redefinition occurs too early, probably because
  include/asterisk/util.h is included before system headers,
  and this causes the compiler to complain about the symbol being
  redefined by the system's headers (on FreeBSD 4.11 at least).
  Not sure on how many files this happens, but it is on many of them.
  If it were only one or a few files i would simply make the compiler
  less pedantic on those files, but it is on way too many of them.
  If you leave this, you cannot run with -Werror.

3) there is a huge number of warnings in compiling res_crypto.c - have
  not checked in detail the reason, but without fixing those you cannot
  use -Werror on that file. See the note above.
By: Michael Jerris (mikej) 2005-05-22 11:21:06

in what system headers are the inet_ntoa definitions for you?  Why not include the propper headers directly in util.h so we can get rid of the order dependecy.  There are several of these order dependency issues in asterisk that need to be fixed.
By: Mark Spencer (markster) 2005-05-22 17:34:20

The gsm ones are not unused, it just depends on your compile options.
By: Russell Bryant (russell) 2005-05-23 18:51:22

Do you see a place where utils.h is included before system headers?  There was recently a patch to clean up the header include order so that this was never the case.  Maybe a spot was missed.
By: Luigi Rizzo (rizzo) 2005-05-24 17:27:19

i would suggest to simply apply the non-controversial pieces,
drop the rest and close the bug report.
If the need arises again, I or others will resubmit the dropped parts
in a better way.

It's pointless to keep discussing the various bits for so long, anyways - it just makes the entire report to be forgotten and lost.
By: Russell Bryant (russell) 2005-05-25 06:55:15

Added to CVS HEAD

I excluded the following things ...

1) The codec_gsm changes

2) The if 0 on inet_ntoa

3) The res_crypto Makefile change

Thanks!
By: Michael Jerris (mikej) 2005-06-01 23:55:23

chan_zap.c, iax2-parser.c, app_dictate.c, config.c: changes not necessary in stable due to the problematic lines or vars not being in stable

Changes to app_controlplayback.c, app.c attached as 4353.stable.patch.txt
By: Russell Bryant (russell) 2005-06-14 17:07:27

fixed in 1.0
By: Digium Subversion (svnbot) 2008-01-15 15:36:01.000-0600

Repository: asterisk
Revision: 5764

U   trunk/app.c
U   trunk/apps/app_controlplayback.c
U   trunk/apps/app_dictate.c
U   trunk/apps/app_flash.c
U   trunk/channels/chan_zap.c
U   trunk/channels/iax2-parser.c
U   trunk/config.c

------------------------------------------------------------------------
r5764 | russell | 2008-01-15 15:36:01 -0600 (Tue, 15 Jan 2008) | 2 lines

various code cleanups (bug ASTERISK-4251)

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

http://svn.digium.com/view/asterisk?view=rev&revision=5764
By: Digium Subversion (svnbot) 2008-01-15 15:38:15.000-0600

Repository: asterisk
Revision: 5914

U   branches/v1-0/app.c
U   branches/v1-0/apps/app_controlplayback.c

------------------------------------------------------------------------
r5914 | russell | 2008-01-15 15:38:14 -0600 (Tue, 15 Jan 2008) | 2 lines

remove pointless dereference (bug ASTERISK-4251)

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

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