Fix warnings (C99; local snprintf() and math functions)

[jimklimov]

For kicks I configured and built with "-Wall -Werror"... and the build died on me, on several OSes. So this is a proposed fix, as far as syntax is concerned. Testing desired (that strings are still printed as they should be), and running on the buildbot farm should be useful too ;)

[clepple]

I am still a little confused as to how snprintf.c is getting compiled on a modern system (which should have snprintf() in the standard library)

[jimklimov]

Me neither (comments in configure.ac are not helpful in this regard) but it gets added to LTLIBOBJS on both Debian8 and OpenIndiana Hipster, both pretty modern. Maybe an oversight in configure.ac logic, that bit I did not deal with so far - just glanced where the object-file conflicts grow from...

[clepple]

Do you have the following in the ./configure output?

checking for vsnprintf... yes
checking for snprintf... yes

I don't think I have ltdl installed on the Buildbot debian8 slave system, so that might be one difference.

[jimklimov]

Looking at configure.ac, here is the snippet :

dnl the following may add stuff to LIBOBJS (is this still needed?)
AC_CHECK_FUNCS(vsnprintf snprintf, [], [
        AC_LIBOBJ(snprintf)
        AC_TYPE_LONG_DOUBLE
        AC_TYPE_LONG_LONG_INT
])

AC_REPLACE_FUNCS(setenv strerror atexit)

... and the relevant logs:

[debian8] checking for unsigned long long int... yes
[debian8] checking for vsnprintf... yes
[debian8] checking for snprintf... yes
[debian8] checking for setenv... yes
[debian8] checking for strerror... yes
[debian8] checking for atexit... yes

and here is some, I guess, from OI hipster:

checking for unsigned long long int... yes
checking for vsnprintf... no
checking for long double... yes
checking for long long int... yes
checking for snprintf... no
checking for long double... (cached) yes
checking for long long int... (cached) yes
checking for setenv... yes
checking for strerror... yes
checking for atexit... yes

Looking at it more, the only builds which failed for me (on both platforms) were the -Wall -Werror pedantic distcheck jobs. It may be that these keys caused some compilation done by autoconf to fail and think these routines are not available:

root@jimoi:/var/lib/jenkins/jobs/nut/branches/test%2FJenkinsfile/lastSuccessful# grep dmf-warn log | ggrep -B1 -A8 vsnpr
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for unsigned long long int... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for vsnprintf... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for snprintf... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for setenv... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for strerror... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for atexit... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for pkg-config... /usr/bin/pkg-config
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking pkg-config is at least version 0.9.0... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for CPPUNIT... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking whether optind is declared... yes
--
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for unsigned long long int... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for vsnprintf... no
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for long double... yes
[TEST-distcheck-dmf-warnings@hipster_jimoi_openindiana_solaris] checking for long long int... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking whether byte ordering is bigendian... no
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for inline... inline
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for flexible array members... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for variable-length arrays... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for flock... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for lockf... yes
--
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for unsigned long long int... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for vsnprintf... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for snprintf... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for setenv... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for strerror... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for atexit... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for pkg-config... /usr/bin/pkg-config
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking pkg-config is at least version 0.9.0... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for CPPUNIT... no
[TEST-distcheck-dmf-warnings@debian_debian8_linux] configure: WARNING: libcppunit not found.
--
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for unsigned long long int... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for vsnprintf... no
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for long double... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for long long int... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for snprintf... no
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for long double... (cached) yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for long long int... (cached) yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for setenv... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for strerror... yes
[TEST-distcheck-dmf-warnings@debian_debian8_linux] checking for atexit... yes

But still, for such a use-case of some ancient OS (or something that otherwise builds like one) there was an issue and now it's plumbed ;)

[jimklimov]

Rebased over today's master to take advantage of new Travis CI integration.

[jimklimov]

@clepple : are there any objections to merging this PR?

[clepple]

Still curious as to why the fallback snprintf() is being built on a modern system.

[jimklimov]

Not fully certain. Probably some macros did not get set in the OpenIndiana build case. At least, the routine is optionally defined on Solarish platforms (like many other similar bits in headers), so without explicit declaration that you want new features, it does what legacy software expected:

/usr/include/iso/stdio_c99.h :

#if defined(__EXTENSIONS__) || defined(_STDC_C99) || \
        (!defined(_STRICT_STDC) && !defined(__XOPEN_OR_POSIX)) || \
        defined(_XPG5)
extern int snprintf(char *_RESTRICT_KYWD, size_t, const char *_RESTRICT_KYWD,
        ...);
extern int vsnprintf(char *_RESTRICT_KYWD, size_t, const char *_RESTRICT_KYWD,
        __va_list);

#endif /* defined(__EXTENSIONS__) || defined(_STDC_C99) ... */

[jimklimov]

So? :)

Put another way, does merging this break anything (proven or anticipated)?

[clepple]

The goal of zero warnings is noble, but patching a local version of snprintf() shouldn't mask the underlying problem, which is that we have more work to do to make sure we are using recent C/POSIX features on all platforms.

Would be nice to add some of the notes about enabling C99 to the documentation, I guess, but not strictly required. Either way, I'm going to add to the PR title so we can find this discussion in the future if any issues crop up.

[jimklimov]

Recommendation 3.3 at http://networkupstools.org/docs/developer-guide.chunked/ar01s03.html essentially suggests C89/C90 if not older (ok, there ain't older ;) )

[jimklimov]

Also, the goal of zero warnings is indeed a goal, because NUT uses -Wall (per style doc 3.5) and on stricter systems this coupled with -Werror yields a build failure. And adding portability rather than removing it is also nice.

Anyhow, now that this is renamed for easier tracking in the future, can it be merged to clean up the queue? ;p

[clepple]

Anyhow, now that this is renamed for easier tracking in the future, can it be merged to clean up the queue? ;p

Yes

[jimklimov]

Thanks :) Done! 👍

[clepple]

@jimklimov not to beat a dead horse, but IPMI is one case where the effort needed to suppress all warnings is not worth it. I can't find the exact versions now, but one of the FreeIPMI versions returns values as unsigned, and another returns signed. Since we are looking for positive voltages, it doesn't affect the code. Rather than using -Werror, we should be looking at the warnings to see if they warrant a unit test. What would be great is to compare a commit to its parent to see if new warnings have been added.

Many of the developer recommendations listed as portability issues are no longer applicable as such, but note that the style is based off of K&R C. The original author intermingled style and portability guidelines in that section, and while I feel a twinge of shame for not trying to update that, the things I consider readable can be traced back to K&R as well. If the guidelines need to be updated to take advantage of progress since C89, then that is the discussion we should be having, rather than trying to play a metrics game with the warnings count.