[Open-scap] [PATCHes] OpenSCAP symbol visibility + cleanups

[Steve Grubb]

----------  Forwarded Message  ----------

Subject: [PATCHes] OpenSCAP symbol visibility + cleanups
Date: Tuesday 06 October 2009
From: Miloslav Trmac <mitr at redhat.com>
To: Peter Vrabec <pvrabec at redhat.com>

Hello,
Hide-most-symbols-missing-from-public-header-files.patch modifies visibility of 
symbols in libopenscap.so.

The rules for keeping symbol visibility correct with this patch are:
- Local functions should be static (This is important, and has to be 
consistently enforced!)
- Each non-static function should be defined in a header file, either public or 
private
  (-Wmissing-prototypes can help with checking this)
  NOTE: If a function is declared both in a private and public header file, it 
will be hidden.
- Each private header file must:
  - #include "common/util.h"
    (The mechanism currently requires GCC, but it is centralized in util.h, so 
other compilers can be accommodated by only modifying this file.)
  - Before all declarations (but after all #includes), add
    OSCAP_HIDDEN_START;
  - After all declarations, add
- If a function is declared both in a private and public header file, it will 
be hidden.
    OSCAP_HIDDEN_END;

This approach leads to better generated code than using linker scripts, and it 
"fails open" - if the rules are not followed, the symbol will be visible.  If 
you want a system that "fails closed" (a symbol is not visible unless an 
explicit action is taken), that would be possible too, at the cost of being 
more gcc-specific.

The patch considers the public header files authoritative.  Some consequences:
- Several files contained global variables that could be used to change debug 
state at run-time by a debugger, e.g. "int _DEBUG = 0;".  Because this 
mechanism was not used consistently, I guess that it was not intended for this 
use, and the patch makes them static, so they might be optimized away by the 
compiler.  If you truly want run-time modifiable debug flags, use the "volatile" 
qualifier.
- Some functions are not declared in the public header files, neither used 
anywhere else in the code.  They were marked static as well, search for 
"defined but not used" warnings and add them to a public header file if desired.
- Some exported functions have irregular names, e.g. behavior_set_keyval().  I 
don't know whether these functions should be kept as-is (that's what I did), 
made private, or renamed.

Two sets of symbols were not hidden in entirety:
- Many xccdf_{benchmark,check,fixtext,group,item,profile,rule,value}_get_ 
functions are automatically generated, but they are missing from header files.  
I think the prototypes should be added in this case.
- Some recent cpe_dict_* changes add many functions, but do not add them to 
public header files.  I left most of them alone, assuming the area is currently 
being worked on and the prototypes will be added later.


I have also attached a diff between the symbols exported by the unpatched and 
patched version, and a full list of symbols exported by the patched version.  
Please verify them to make sure the changes are reasonable. 

The second attached patch fixes quite a few warnings; many avoidable warnings 
are still left.


Finally, some suspicious things I noticed:
- There is a case of sprintf("..."), writing into a constant string.  That 
will crash; I'm not sure what the code should do.
- There are two instances of "operation ... may be undefined" that clearly are 
undefined (basically i = i++).  Again, I don't know what the code is supposed 
to do.

Thank you,
    Mirek
-------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Hide-most-symbols-missing-from-public-header-files.patch
Type: application/mbox
Size: 159322 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/open-scap-list/attachments/20091007/1d7aac61/attachment.mbox>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-quite-a-few-warnings-many-others-left.patch
Type: application/mbox
Size: 86956 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/open-scap-list/attachments/20091007/1d7aac61/attachment-0001.mbox>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: symbol-changes
Type: text/x-patch
Size: 24337 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/open-scap-list/attachments/20091007/1d7aac61/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: all-symbols-new
Type: application/octet-stream
Size: 27984 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/open-scap-list/attachments/20091007/1d7aac61/attachment.obj>