[Ipmitool-devel] Series 3 - Tracker 3608759

[Dan Gora]

Here are the patches for series 3.

[Dan Gora]

This was hiding an important bug in ipmi_fru.h and is not a valid
flag for older versions of gcc.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/configure.in | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ipmitool/configure.in b/ipmitool/configure.in
index 0708795..a35cd24 100644
--- a/ipmitool/configure.in
+++ b/ipmitool/configure.in
@@ -33,7 +33,7 @@ AC_CHECK_FUNCS([alarm gethostbyname socket select])
AC_CHECK_FUNCS([memmove memset strchr strdup strerror])
AC_CHECK_FUNCS([getpassphrase])

-CFLAGS="$CFLAGS -fno-strict-aliasing -Wreturn-type -Wno-unused-result -Wno-packed-bitfield-compat"
+CFLAGS="$CFLAGS -fno-strict-aliasing -Wreturn-type -Wno-unused-result"

AM_PROG_LIBTOOL
LIBTOOL="$LIBTOOL --silent"

[Dan Gora]

The previous test to see if we need pragma pack(1) was failing if we
cross compile because there was no way to run the test.

We now use AC_TRY_COMPILE and add the pragma pack(1) to the test code.
If it compiles, it must be good, so add the define.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/configure.in | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ipmitool/configure.in b/ipmitool/configure.in
index a35cd24..e319ef2 100644
--- a/ipmitool/configure.in
+++ b/ipmitool/configure.in
@@ -513,7 +513,7 @@ if test "x$xenable_file_security" != "xno"; then
fi


-AC_TRY_RUN([
+AC_TRY_COMPILE([],[
#include <stdio.h>

struct packstruct {
@@ -532,7 +532,9 @@ AC_TRY_RUN([
else
return(0);
}
- ],[],[AC_DEFINE(HAVE_PRAGMA_PACK,[1],
+ ],
+ [],
+ [AC_DEFINE(HAVE_PRAGMA_PACK,[1],
[Define to 1 if you need to use #pragma pack instead of __attribute__ ((packed))])]
)

[Zdenek Styblik]

Post by Dan Gora
The previous test to see if we need pragma pack(1) was failing if we
cross compile because there was no way to run the test.
We now use AC_TRY_COMPILE and add the pragma pack(1) to the test code.
If it compiles, it must be good, so add the define.

I'm ok with this one.

Z.

Post by Dan Gora
---
ipmitool/configure.in | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/ipmitool/configure.in b/ipmitool/configure.in
index a35cd24..e319ef2 100644
--- a/ipmitool/configure.in
+++ b/ipmitool/configure.in
@@ -513,7 +513,7 @@ if test "x$xenable_file_security" != "xno"; then
fi
-AC_TRY_RUN([
+AC_TRY_COMPILE([],[
#include <stdio.h>
struct packstruct {
@@ -532,7 +532,9 @@ AC_TRY_RUN([
else
return(0);
}
- ],[],[AC_DEFINE(HAVE_PRAGMA_PACK,[1],
+ ],
+ [],
+ [AC_DEFINE(HAVE_PRAGMA_PACK,[1],
[Define to 1 if you need to use #pragma pack instead of __attribute__ ((packed))])]
)
--
1.7.7
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

[Dan Gora]

This is not a valid compiler flag with older gcc versions.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/configure.in | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ipmitool/configure.in b/ipmitool/configure.in
index e319ef2..641f266 100644
--- a/ipmitool/configure.in
+++ b/ipmitool/configure.in
@@ -33,7 +33,7 @@ AC_CHECK_FUNCS([alarm gethostbyname socket select])
AC_CHECK_FUNCS([memmove memset strchr strdup strerror])
AC_CHECK_FUNCS([getpassphrase])

-CFLAGS="$CFLAGS -fno-strict-aliasing -Wreturn-type -Wno-unused-result"
+CFLAGS="$CFLAGS -fno-strict-aliasing -Wreturn-type"

AM_PROG_LIBTOOL
LIBTOOL="$LIBTOOL --silent"

[Dan Gora]

Dan,
Without this compile flag, there are many warning messages with the new gcc
versions.

Are there? I just compiled it without -Wno-unused-result with gcc
4.6.2 and I only see three warnings:

../../lib -I.. -I../../include -g -O2 -fno-strict-aliasing
-Wreturn-type -MT ipmi_fwum.lo -MD -MP -MF .deps/ipmi_fwum.Tpo -c -o
ipmi_fwum.lo ../../lib/ipmi_fwum.c
../../lib/ipmi_fwum.c: In function 'KfwumGetFileSize':
../../lib/ipmi_fwum.c:482:47: warning: cast from pointer to integer of
different size [-Wpointer-to-int-cast]

/bin/sh ../libtool --silent --tag=CC --mode=compile gcc
-DHAVE_CONFIG_H -I. -I../../lib -I.. -I../../include -g -O2
-fno-strict-aliasing -Wreturn-type -MT ipmi_dcmi.lo -MD -MP -MF
.deps/ipmi_dcmi.Tpo -c -o ipmi_dcmi.lo ../../lib/ipmi_dcmi.c
../../lib/ipmi_dcmi.c: In function 'ipmi_dcmi_prnt_oobDiscover':
../../lib/ipmi_dcmi.c:457:20: warning: cast from pointer to integer of
different size [-Wpointer-to-int-cast]

/bin/sh ../../../libtool --silent --tag=CC --mode=compile gcc
-DHAVE_CONFIG_H -I. -I../../../../src/plugins/imb -I../../..
-I../../../../include -g -O2 -fno-strict-aliasing -Wreturn-type -MT
imbapi.lo -MD -MP -MF .deps/imbapi.Tpo -c -o imbapi.lo
../../../../src/plugins/imb/imbapi.c
../../../../src/plugins/imb/imbapi.c: In function 'MapPhysicalMemory':
../../../../src/plugins/imb/imbapi.c:1971:23: warning: cast from
pointer to integer of different size [-Wpointer-to-int-cast]

None of these would go away with -Wno-unused-result.

Would it be possible to instead change configure.in to determine if the
installed gcc version
supports -Wno-unused-result?

I think that it would be much better to fix the warnings rather than
to hide them. Especially since the -Wno-packed-bitfield-compat flag
was hiding serious bugs..

I have found that people don't notice new compile time warnings that crop up
due to their
changes when files are already emitting warnings.

I agree, but I think that it's better to fix the warnings if at all
possible. Of course adding -Werror would force people to deal with
their warnings.

I'd really like to see
ipmitool compile
cleanly with recent versions of gcc, and of course with older versions if
possible as well.

Agreed, but it seems to compile pretty cleanly for me on x64 with gcc
4.6.2 and on Cavium MIPS64:

dg:speedy:build(master) => gcc --version
gcc (SUSE Linux) 4.6.2
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

dg:speedy:debian(master) => mips64-octeon-linux-gnu-gcc --version
mips64-octeon-linux-gnu-gcc (Cavium Inc. Version: 2_3_0 build 128) 4.3.3
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

thanks
dan

[Zdenek Styblik]

On Mon, Apr 1, 2013 at 8:03 PM, Dan Gora <***@adax.com> wrote:
[...]

Post by Dan Gora
I agree, but I think that it's better to fix the warnings if at all
possible. Of course adding -Werror would force people to deal with
their warnings.

Dan,

does it mean to deal with warnings/"errors" left by "others" as well?
If so ... yeah, I'd be realistic about this one.
In other words, I despite I agree, I don't think it's going to fly.
:-/ Same for clean compilation - somewhat wishful thinking. Just check
the tracker and never ending queue of tickets.

Regards,
Z.

[Dan Gora]

Post by Zdenek Styblik
[...]

Post by Dan Gora
I agree, but I think that it's better to fix the warnings if at all
possible. Of course adding -Werror would force people to deal with
their warnings.

Dan,
does it mean to deal with warnings/"errors" left by "others" as well?

Well if you want it to compile cleanly without warnings, that's pretty
much the only way to do it, regardless of who's responsible.

Post by Zdenek Styblik
If so ... yeah, I'd be realistic about this one.

Yeah I agree.. I don't have any problem with the odd warning here and
there, especially since gcc changes what it warns about in some
versions (comparing signed and unsigned chars for example).

Post by Zdenek Styblik
In other words, I despite I agree, I don't think it's going to fly.
:-/ Same for clean compilation - somewhat wishful thinking. Just check
the tracker and never ending queue of tickets.

I agree, but removing -Wno-unused-return doesn't add any new warnings
that I can see. If it does, then I'll fix it, but I didn't see any.

thanks
d

[Zdenek Styblik]

Post by Dan Gora

Post by Zdenek Styblik
[...]

Post by Dan Gora
I agree, but I think that it's better to fix the warnings if at all
possible. Of course adding -Werror would force people to deal with
their warnings.

Dan,
does it mean to deal with warnings/"errors" left by "others" as well?

Well if you want it to compile cleanly without warnings, that's pretty
much the only way to do it, regardless of who's responsible.

Post by Zdenek Styblik
If so ... yeah, I'd be realistic about this one.

Yeah I agree.. I don't have any problem with the odd warning here and
there, especially since gcc changes what it warns about in some
versions (comparing signed and unsigned chars for example).

Post by Zdenek Styblik
In other words, I despite I agree, I don't think it's going to fly.
:-/ Same for clean compilation - somewhat wishful thinking. Just check
the tracker and never ending queue of tickets.

I agree, but removing -Wno-unused-return doesn't add any new warnings
that I can see. If it does, then I'll fix it, but I didn't see any.
thanks
d

Oh! I didn't mean I'm against its removal. I'm sorry if there was
misunderstanding.

Z.

[Jim Mankovich]

Dan, Z,

I use gcc 4.6.3 with a fairly recent linux stable kernel release.

Here is an example of why I would like the -Wno-unused-result flag to remain. Notice that if you take the
TOB kernel and compile with a pretty recent compiler and kernel you get a bunch of unused result warnings
that are not of much interest. These could be cleaned up by casting the returns from these system calls to
void and these warnings would go away and then we would not need -Wno-unused-result at all. This would
the best way to get rid of the -Wno-unused-result flag and maintain a clean compilation.

Now take a look at what you see with Dan's first patch applied to TOB and -Wno-unused-result flag. You see
will see that 3 new warnings cropped up in ipmi_fru.c that should be fixed, but were not noticed either because the
warnings were lost in the other warnings or by virtue of a compiler/OS that didn't have the extra warning
messages.

With gcc 4.6.3 on ubuntu here are the warnings I get with TOB and no (-Wno-unused-result) flag.
Note: If I have -Wno-unused-result turned on, I don't get any warning messages when I compile ipmitool.

helper.c:633:7: warning: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Wunused-result]
helper.c:642:5: warning: ignoring return value of 'dup', declared with attribute warn_unused_result [-Wunused-result]
helper.c:643:5: warning: ignoring return value of 'dup', declared with attribute warn_unused_result [-Wunused-result]
ipmi_fru.c:1563:7: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_fru.c:1575:9: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_hpmfwupg.c:2592:15: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_hpmfwupg.c:1107:10: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2335:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2397:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2411:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2426:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2451:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2454:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2465:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2475:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2554:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2557:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2567:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2619:13: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2639:31: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2668:28: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2681:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2742:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2745:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2754:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3892:13: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3908:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3918:25: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]


With gcc 4.6.3 on ubuntu here are the warnings I get with Dan's First patch applied to TOB and no (-Wno-unused-result) flag.

helper.c:633:7: warning: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Wunused-result]
helper.c:642:5: warning: ignoring return value of 'dup', declared with attribute warn_unused_result [-Wunused-result]
helper.c:643:5: warning: ignoring return value of 'dup', declared with attribute warn_unused_result [-Wunused-result]
ipmi_fru.c:2917:11: warning: format '%d' expects argument of type 'int', but argument 2 has type 'long unsigned int' [-Wformat]
ipmi_fru.c:2917:11: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat]
ipmi_fru.c:2917:11: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat]
ipmi_fru.c:1563:7: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_fru.c:1575:9: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_hpmfwupg.c:2592:15: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_hpmfwupg.c:1107:10: warning: ignoring return value of 'scanf', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2335:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2397:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2411:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2426:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2451:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2454:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2465:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2475:22: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2554:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2557:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2567:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2619:13: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2639:31: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2668:28: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2681:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2742:16: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2745:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:2754:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3892:13: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3908:19: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]
ipmi_ekanalyzer.c:3918:25: warning: ignoring return value of 'fread', declared with attribute warn_unused_result [-Wunused-result]

Post by Dan Gora

Post by Zdenek Styblik
[...]

Post by Dan Gora
I agree, but I think that it's better to fix the warnings if at all
possible. Of course adding -Werror would force people to deal with
their warnings.

Dan,
does it mean to deal with warnings/"errors" left by "others" as well?

Well if you want it to compile cleanly without warnings, that's pretty
much the only way to do it, regardless of who's responsible.

Post by Zdenek Styblik
If so ... yeah, I'd be realistic about this one.

Yeah I agree.. I don't have any problem with the odd warning here and
there, especially since gcc changes what it warns about in some
versions (comparing signed and unsigned chars for example).

Post by Zdenek Styblik
In other words, I despite I agree, I don't think it's going to fly.
:-/ Same for clean compilation - somewhat wishful thinking. Just check
the tracker and never ending queue of tickets.

I agree, but removing -Wno-unused-return doesn't add any new warnings
that I can see. If it does, then I'll fix it, but I didn't see any.
thanks
d

[Dan Gora]

Post by Jim Mankovich
Dan, Z,
I use gcc 4.6.3 with a fairly recent linux stable kernel release.
Here is an example of why I would like the -Wno-unused-result flag to
remain. Notice that if you take the
TOB kernel and compile with a pretty recent compiler and kernel you get a
bunch of unused result warnings
that are not of much interest. These could be cleaned up by casting the
returns from these system calls to
void and these warnings would go away and then we would not need
-Wno-unused-result at all. This would
the best way to get rid of the -Wno-unused-result flag and maintain a clean compilation.

Actually we probably should check the return values for failure! I'll
try and see if I can find a system here which gives these errors and
see just how bad it is to clean this up.

Or... would we rather just punt and put -Wno-unused-return back in?

thanks
dan

[Jim Mankovich]

Fom my perspective, cleaning up the errors to remove the need for -Wno-unused-return would be
best. But, I wouldn't spend a lot of time writing code and tests to deal with handling all possible
errors from the system calls.

Post by Dan Gora

Post by Jim Mankovich
Dan, Z,
I use gcc 4.6.3 with a fairly recent linux stable kernel release.
Here is an example of why I would like the -Wno-unused-result flag to
remain. Notice that if you take the
TOB kernel and compile with a pretty recent compiler and kernel you get a
bunch of unused result warnings
that are not of much interest. These could be cleaned up by casting the
returns from these system calls to
void and these warnings would go away and then we would not need
-Wno-unused-result at all. This would
the best way to get rid of the -Wno-unused-result flag and maintain a clean compilation.

Actually we probably should check the return values for failure! I'll
try and see if I can find a system here which gives these errors and
see just how bad it is to clean this up.
Or... would we rather just punt and put -Wno-unused-return back in?
thanks
dan