Fix warnings in OSAL "unit-test" code

[skliper]

The code in the "unit-tests" directory in OSAL has a number of warnings when it compiles and this is one thing preventing turning on the strictest compiler settings (-Wall -Werror, etc) during the build.

Many of the warnings are related to passing a non-const string directly to a printf function when logging test results.

[skliper]

Imported from trac issue 7. Created by jphickey on 2014-12-30T11:27:04, last modified: 2015-11-20T16:22:16

[skliper]

Trac comment by glimes on 2015-01-29 15:11:51:

Click [changeset:530734e here] to see the changeset.

[skliper]

Trac comment by glimes on 2015-01-29 16:35:51:

I have reviewed [changeset:530734e changeset 530734e] and recommend ACCEPT after one potential problem is resolved.

Potential problem:

• Line 99 of ut_os_stubs.h references the old aVAR parameter. This will become a compilation error when using OS_LOG_TO_STDOUT_TOO (with OS_USE_EMBEDDED_PRINTF off). This just need to change to __VA_ARGS__ and all is good again.

Important side benefits of the included changes:

• Fewer calls to sprintf(), a known buffer overflow hazard.
• Fewer calls sending output text as 1st arg to printf()

New things required of target platorms:

• The <limits.h> header must be present for all _LINUX_OS_ targets.
• The preprocessor must support #define FOO(...) style varargs macros.
  The hoop-jumping to get around lack of support for the above should only be added together with documentation of what supported build-and-unit-test environment forced us to jump; it is unlikely that any such platform exists.

[skliper]

Trac comment by mlogan on 2015-01-30 18:49:38:

So my diff of this branch to the master branch shows many more changes.

Compile-time select-able code was introduced, but it is unclear as to how "warnings" were addressed by this. Would like a little more detail.

namely..

+#ifdef OSAL_UNIT_TEST

• OS_ApplicationExit(g_logInfo.nFailed > 0);
  +#else
• return (g_logInfo.nFailed > 0);
  +#endif

Additionally, ut_osloader_stubs.c had a block of code removed as well.
Would like to have more explanation regarding the relationship to the ticket.

-#ifdef OS_INCLUDE_MODULE_LOADER
-/* As defined in osloader.c */
-OS_module_record_t OS_module_table[OS_MAX_MODULES];

-#ifdef LINUX_OS
-pthread_mutex_t OS_module_table_mut;
-#endif
-#endif /* OS_INCLUDE_MODULE_LOADER */

[skliper]

Trac comment by jphickey on 2015-01-30 21:38:10:

Regarding the {{{#ifdef OSAL_UNIT_TEST}}} bits:

At the entry point in all of these tests there was already a conditional compile such as this:

{{{
#ifdef OSAL_UNIT_TEST
void OS_Application_Startup(void)
#else
int main(int argc, char* argv[])
#endif
}}}

However at the end of the function there was an unconditional:
{{{
return (0);
}}}

This produced a warning about returning with a value from a void function in the case that the OSAL_UNIT_TEST was defined and the OS_Application_Startup() function declaration was used. In this case it is changed to calling OS_Application_Exit() (the new function) and the return statement is removed which clears up the warning.

In both cases the return/exit code now indicates the test pass/fail status where it was previously just 0. This bit of the change is not strictly a warning fix. One could argue that this return value modification better fits in trac #29, but this would end up as a merge conflict when trying to put them back together again, so I just included it in this changeset that was already touching that line.

[skliper]

Trac comment by jphickey on 2015-01-30 22:12:48:

As for the removal of the {{{OS_module_table}}} variables from ut_osloader_stubs.c, these were simply unused variables. It is not a warning per se, just some clean up. Not sure what the history of these were and why they needed to be provided by the osloader stub at one time.

If anyone has concerns about the removal of that block in ut_osloader_stubs.c then that one file can be reverted out of this changeset. However given that this is a unit test (not runtime code) and the fact that the compiler/linker tends to be really good at finding uninstantiated variables, one can be fairly sure that these were unreferenced variables.

[skliper]

Trac comment by jphickey on 2015-01-30 22:28:54:

Regarding the original note by Greg about aVAR on Line 99 -- good catch. The current build did not define OS_LOG_TO_STDOUT_TOO but this was indeed a problem when this is defined. I have fixed this and confirmed the build now works when the OS_LOG_TO_STDOUT_TOO macro is defined.

This brings up another topic of conversation on whether to use "git commit --amend" or simply create a new commit on this branch to fix this up.

I generally advocate for cleaner version history and use "git commit --amend" and "git push -f" when this type of after-the-fact patch comes up. But the side effect is that the commitid will change and the link posted earlier will break.

[skliper]

Trac comment by jphickey on 2015-01-30 22:34:31:

Amended commit has been pushed. The new commit is [changeset:6d3b3aa]. I also modified the log message to mention the return value change when OSAL_UNIT_TEST is defined.

[skliper]

Trac comment by glimes on 2015-02-02 14:26:47:

Updated changeset looks good; I recommend ACCEPT.

( On the "to amend or not to amend" question: amend made it harder to find how the proposed changesets differed, but you really need an amend if you are editing the log message to clarify, as a log message in a child commit talking about something in the parent commit is also confusing. I use amend too much, so I recuse myself from making any kind of recommendation here ... )

[skliper]

Trac comment by jwilmot on 2015-02-11 14:25:48:

Seems benign, ACCEPT

[skliper]

Trac comment by acudmore on 2015-02-11 14:53:32:

Looks good, ACCEPT.

[skliper]

Trac comment by glimes on 2015-02-12 14:58:25:

Accepted by #41 for integration candidate.

[skliper]

Trac comment by glimes on 2015-03-04 15:13:15:

Confirmed that the fix from [changeset:6d3b3aa] is present in the
current development branch of OSAL, [changeset:5aae372].

https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSOSAL
produces logs containing the current build. I confirm that
going from build 54 to build 55, many "warning" messages
disappeared from the build log.