libsmbios (ready to deliver?)

Michael_E_Brown@Dell.com Michael_E_Brown at Dell.com
Tue Jul 26 11:55:45 CDT 2005


Tim,

Docs:  All documentation is generated using Doxygen. In the Linux build,
you can do a "make docs" to produce them, they are output under
doc/full/html. The docs are generated from comments embedded in the
code. The majority of the documentation is in the header files under
include/smbios/. The full set of docs are posted at
http://linux.dell.com/libsmbios/main/. Everything on the website is
generated by the doxygen docs. 
	You can add the docs for the fill() operator in the operator>>()
function in the header file. I believe it would be in ISmbiosTable.h

	There is also a Windows build of Doxygen, so theoretically you
could generate docs on Windows. I have not taken the trouble to do a
script to do this, but if you wish to try and submit it, that would be
welcome.

Streamify(): You are not restricted from making any internal code
changes internal to the implementation, as long as they are sane and do
not affect the public API. _ANY_ changes to the include/smbios/Ixxx.h
files change the public API and require extra review. Adding
streamify2() changes the public API and means that programs linked with
older versions will not be binary compatible with the newer version.
Since we can easily do this without changing the public API, lets do it
that way.

fill(): This is a good way of changing output modes without changing the
public API. I think, however, that it may be worth it to add a new API
to change output modes. I have no preconceived ideas on the best way to
do this at the moment. Keeping the fill() code for the time being is
acceptable.

Project files: If you have suggestions for project file changes, please
submit them separately. I want the existing project files to be as
useful as possible to as many people as possible. I believe that you may
have hijacked your project files so that they do not reflect the changes
that I added recently. I added the SmbiosTable_Windows.cpp file to the
project and turned off precompiled headers. This may be why you were
having problems earlier.

SDK: All code in libsmbios must be cross platform. All platform-specific
code must be appropriately isolated. Currently, the XML processing is
done with Xerces because it was the most popular cross platform DOM
processing lib I could find. There is currently no facility of any kind
for adding platform-specific code to the XML processing, and I have no
intention of doing this unless somebody presents an extremely compelling
argument.
	Another goal is to make the dependency graph as short as
possible. Each additional SDK that must be downloaded to compile the
code increases the burden on future developers. This is why I have
created the "miniddk.h" file. All outside definitions that are used are
pasted here. Then, to minimize deps, we LoadLibrary() and
GetProcAddress() rather than statically link. Then we deal with any
failures by falling back to alternate methods. 

As for your last paragraph, I'll send another email to address this, as
this is already getting long and I need to go to lunch. :-)
--
Michael

>  -----Original Message-----
> From: 	Fettig, Tim - Authorized Dell Representative  
> Sent:	Tuesday, July 26, 2005 10:05 AM
> To:	Brown, Michael E; Borgstrand, Matt
> Cc:	'libsmbios-devel at lists.us.dell.com'
> Subject:	RE: libsmbios (ready to deliver?)
> 
> Michael,
> 	I have no problems with lengthy comments and will submit changes
> to the code as per your restrictions and the those of the standards
> cited within.
> To address specific answers within I will use your text as reference
> and place my replies in Green.
> 
> _____________________________________________ 
> From: 	Brown, Michael E  
> Sent:	Monday, July 25, 2005 11:04 AM
> To:	Borgstrand, Matt
> Cc:	Fettig, Tim - Authorized Dell Representative;
> 'libsmbios-devel at lists.us.dell.com'
> Subject:	RE: libsmbios (ready to deliver?)
> 
> Matt, Tim,
> 	Thanks for your code submission for libsmbios. I have started
> reviewing it and am copying this msg to the development lists because
> I want to ensure that all new code submissions are public, even
> internal Dell contributions.
> 
> 	I don't want to discourage, but my comments are lengthy. The
> reason for this is that any new code that I accept into libsmbios has
> to become "mine". That is, I cannot expect that people who contribute
> code will be around in 6 months for bugfixes or features additions, so
> I have to ensure that I understand the code and it integrates cleanly
> with the existing style. If I do not understand the code, it doesn't
> match the existing style, or is overly complicated, I make my life
> much more difficult in the future when I have to do additional
> maintenance and feature work. No problems here.
> 
> 	First, there are a few style-guide issues with the code.
> http://linux.dell.com/libsmbios/main/coding.html and, in particular
> http://linux.dell.com/libsmbios/main/coding_style.html. The main
> problem is the indentation. It must be 4 spaces, no tab chars. Your
> braces and if() style look good, no changes there. I will review these
> links to be sure that this submission meets these style-guides.
> 
> 	Next, as a design issue, you are using the iostream fill() char
> as a switch to change between XML and regular output (per Tim's notes
> below and looking at the code), correct? While I believe that having
> the ability to switch output styles is a good thing, I need to see
> more discussion on if this is the best way to do this. In any case, I
> would expect to see much more documentation on this feature. In the
> dumpSmbios.c file where it is used, there should be a big, fat
> comment. In the file where it is implemented, there should be a
> comment. I agree with the big fat comment, mostly waiting to see if
> you might offer insight to your code and an alternative to using the
> fill() char as a flag. If this method were to be accepted, then
> certainly documentation of it would be in the code and be submitted
> for your official library docs. I did not see a document like this in
> the set of code/text I received. Do you have a link to it?
> 
> 	In addition, naming of the streamify() and streamify2() needs to
> be rethought. Neither name clearly indicates what it does or is
> supposed to do. one should be streamify_xml or something similar. You
> should not have to add streamify2() to the public definitions in
> ISmbiosTable, nor add a duplicate function definition in
> SmbiosTable.cpp. I believe that a better way to do this would be to
> simply add a parameter to streamify(), like this: ostream &
> SmbiosItem::streamify (ostream & cout, int output_format) const; Then,
> the SmbiosTable type just silently ignores the output format. The
> streamify() for SmbiosTableXml is an if() statement that either calls
> the superclass streamify() if type == TEXT (an enum), or calls
> streamify_toxml() if type == XML. (add the enum to the private
> SmbiosTableXml class) 
> 
>    I worked under the perception that you needed the streamify() to
> exist for the processing of the table info via standard out. The use a
> second function was to NOT interfere with or complicate the standard
> function path. Since you already had code inplace to control the use
> of streamify(), it seemed like a logical branching point to simply
> redirect. The name of the function could be whatever makes the most
> sense to you because, it is not exposed to the application interface
> directly and would have no impact on dumpSmbios.exe. As to
> duplication, the VC++ compiler would not allow the wrapper class of
> libsmbiosxml to contain some thing that was not in libsmbios...more
> argument to use the parameter or another class member to indicate XML
> tagging versus standard text. Now that I  know that streamify() is not
> restricted to change I agree with your approach of adding a parameter.
> 
> 	It appears that you "hand generate" your XML output. I want to
> understand why you do this instead of using the existing xerces
> library for XML generation.
> The initial requirement for my coding effort was to accomplish XML
> tagged output WITHOUT the Xerces library. DSET has a lot of
> restrictions to package size with the Xerces DLL adding a 2.x MB
> penalty. Most of the XML generated within DSET's utilities is done
> sans any DOM objects. Another reason for "hand generation" was to use
> the existing table element definitions for the tags. These strings
> typically were human oriented labels containing embedded spaces which
> are not allowed as tags in XML.
> 
>   Once it was established that the method employed for libsmbios to
> assign textual labels/decoding of table values was via Xerces and your
> pre-compiled header file, there  was no effort made to reorient the
> XML tagging to leverage a DOM handler. This could be modified going
> forward since libsmbios is so tightly coupled to DOM processing and
> there is no expectation that this would change.
> 
> 	Next, it does not compile, and, additionally, adds new warnings.
> Warnings are not acceptable, the code should compile without warnings.
> (and it should compile... :-)  I'll have to fire up VC++ to see what
> is going on, it probably compiles on VC, but my main dev platform is
> Linux.
> 
> COMPILING C++ (shared)   : libraries/smbiosxml/SmbiosXml.lo
> libraries/smbiosxml/SmbiosXml.cpp: In member function `virtual
> std::ostream&
>    smbios::SmbiosItemXml::streamify2(std::ostream&) const':
> libraries/smbiosxml/SmbiosXml.cpp:670: `ZeroMemory' undeclared (first
> use this
>    function)
> libraries/smbiosxml/SmbiosXml.cpp:670: (Each undeclared identifier is
> reported
>    only once for each function it appears in.)
> libraries/smbiosxml/SmbiosXml.cpp:957: warning: unused variable
> `unsigned int
>    tmpval'
> libraries/smbiosxml/SmbiosXml.cpp:957: warning: unused variable
> `unsigned int
>    tmpmsb'
> make: *** [libraries/smbiosxml/SmbiosXml.lo] Error 1
> 	Since the make files I received would not compile for me using
> VC++  or make a library, I assumed it was because we had differing
> development environments and that you would NOT take my project files.
> (especially since you were creating header files that I had no ability
> to generate and the Xerces components you were using were not in the
> set). The changes to the project files I made, included the addition
> of a Xerces_lib to contain Debug and Release versions of the Xerces
> stub lib and of course the includes needed to compile. The other major
> difference is my use of the Windows Platform SDK which I'm sure you do
> not use since you work under Linux. 
> 
> I can remove all SDK functions to ensure that Linux can resolve
> function calls.
> 
> 	Last, and this is the largest issue I have with the code, the
> main addition to the code is streamify2() in SmbiosXml.cpp, which is
> one single function consisting of over 440 lines of code. There are a
> very small handful of existing functions in libsmbios that are around
> 60 lines. This is about the maximum function size that I allow, unless
> there are extremely compelling reasons to the contrary. Even then, at
> 60 lines, the function must be very well commented.  A reasonable
> maximum size for a function is 20 to 30 lines of code. Since this your
> biggest issue, I want to understand this clearly:
> 
>    Keeping functions to 60 lines or less would mean to me, breaking
> out each element type (i.e. BITSTREAM would have one for
> WORD,DWORD,BYTE, etc) to be processed by it's own function?
> 
> . From my review of the XML structure used to decode the table data,
> it seemed that each type was not only unique in it's size but also in
> what I could determine were it's rules of use (I did not find
> documentation that spoke to these rules). This made for a very
> unnecessarily complicated chunk of code since the data being populated
> in the table did not always require or have a matching entry in the
> XML table. I assumed that this was because the XML used in the header
> was still a "work in progress" and  that once it had a good XSD or DTD
> assigned to it that this code could be very easily made to resolve
> many types via a single function (in specific I expected to see a
> place holder for bits not used (ala BYTE type) instead of using "lsb"
> and "msb" attributes to indicate that bit positions contained more
> than a single bit's worth of data)  . (Do you have an XSD that
> validates your XML structure header file? This would be very helpful
> in getting this code size down.)
> 
>     Also in this area, if Matt does not object to relying on the
> Xerces DOM to handle XML creation (although I did not see any method
> of creating a "good" XML tag from a string containing spaces and other
> invalid tag characters), this whole section of code could be collapsed
> considerably. So re-coding this to use Xerces would not only allow
> smaller functions but, (with the addition of your XSD) allow us to
> create the entire XML document and validate it before writing a single
> character out. Currently we toss it out as we make it and validate it
> outside of libsmbios.
> 
> 	Can you please work on the issues above and then please post
> this as a patch to the libsmbios-devel mailing list? (you can CC me as
> well) You can do this by doing a "make distclean" in both your dev
> stream and integration stream (if compiling on Linux), or chosing the
> cleanup option in VC. Then, run "diff -ruP integration/libsmbios
> dev/libsmbios > patch_filename" where integration and dev are your
> integration stream and dev stream checkouts, respectively. Then attach
> this to your list posting along with a short summary. I would like to
> see it posted to the mailing list as a history of who has contributed
> as well as so that others can comment on the code. When you post it to
> the list, please make the subject of the email look like this:
> "[PATCH] Implement XML ouput for smbios dumps." or something similar.
> --
> Michael
> 
> 	 -----Original Message-----
> 	From: 	Borgstrand, Matt  
> 	Sent:	Tuesday, July 19, 2005 11:18 PM
> 	To:	Brown, Michael E
> 	Cc:	Fettig, Tim - Authorized Dell Representative
> 	Subject:	RE: libsmbios (ready to deliver?)
> 
> 	Michael:
> 
> 	I think we're a "go" over here. I rebased and merged Tim's
> changes into v0.9 and everything still seems to compile and function.
> 
> 	Now, I have NEVER delivered anything in CC before so figured I'd
> wait until you gave the thumbs up. I think I'm already in a little bit
> of trouble because the only stream it is showing I can deliver is an
> integration stream snapshot I can no longer find. I know we have the
> files and the activities are checked in, just don't know how to get
> them to you to review.
> 
> 	How do we proceed?
> 
> 
> 	- Matt
> 
> 			_____________________________________________
> 			From: Fettig, Tim - Authorized Dell
> Representative 
> 			Sent: Tuesday, July 19, 2005 4:26 PM
> 			To: Borgstrand, Matt
> 			Subject: RE: libsmbios
> 
> 			Matt,
> 				Even more simple than adding files...
> simply add the file (which was actually in the set) to the Project
> file and everything compiles and executes as we would expect it to.
> So, I don't think there are any changes required for you to go ahead
> and submit. He may want to remove some of my commented code (I noticed
> as I was searching) but that would have zero effect.
> 			-Tim
> 
> 			_____________________________________________ 
> 			From: 	Borgstrand, Matt  
> 			Sent:	Tuesday, July 19, 2005 4:01 PM
> 			To:	Fettig, Tim - Authorized Dell
> Representative
> 			Subject:	RE: libsmbios
> 
> 			V0.9 baseline is copied to your \For Tim. Crack
> that DSW open.
> 
> 			_____________________________________________
> 			From: Fettig, Tim - Authorized Dell
> Representative 
> 			Sent: Tuesday, July 19, 2005 1:04 PM
> 			To: Borgstrand, Matt
> 			Subject: RE: libsmbios
> 
> 			Matt,
> 				Well it mostly works.
> 			He added a section of code that seems dependent
> on a processor define "LIBSMBIOS_HAS_ARCH_TABLE_CLASS" that causes a
> function that is not in this code to be called. I commented this
> define out and recompiled to make a working library for
> dumpsmbios.exe.
> 
> 			 According to his notes, this should only be
> defined "for platforms that have a special arch-specific  table class
> defined"; this is "new" code from what we had in version 8. Is there a
> chance that this new "arch-specific" feature was checked-in but not
> completely working yet? Since I did not find any source that contained
> the method referenced smbios::SmbiosTableArchSpecific(), maybe we are
> missing a new file added to the set?
> 
> 			-Tim
> 
> 
> 			_____________________________________________ 
> 			From: 	Borgstrand, Matt  
> 			Sent:	Monday, July 18, 2005 8:57 PM
> 			To:	Fettig, Tim - Authorized Dell
> Representative
> 			Subject:	RE: libsmbios
> 
> 			Tim:
> 
> 			Michael's CC project forced me to re-base (aka,
> merge) our changes with his latest approved 0.9 baseline. How we know
> those changes - well, I'm not sure except what I saw from the various
> diffs I had to ack. So go into the Source\matt . . . libsmbios_core\
> to recompile the VCC project(s) to verify things are still working. I
> _did_ notice a mention of change to using a Win32 API call to get to
> SMBIOS vs. a direct call, but I don't recall the file(s) that was
> changed in.
> 
> 			Anyway, try a compile, let me know if it chokes
> bad, and I'll see if I can drag Michael over here to help us get this
> merged and checked back in for him to review in ClearCase.
> 
> 
> 			- Matt
> 
> 			_____________________________________________
> 			From: Fettig, Tim - Authorized Dell
> Representative 
> 			Sent: Friday, July 15, 2005 11:36 AM
> 			To: Borgstrand, Matt
> 			Subject: libsmbios
> 
> 			Matt,
> 				The libsmbios code for check-in is in
> under Source\libsmbios_checkin\libsmbios on ips-borgserv.
> 
> 			A quick summary of changes are:
> *	Addition of ::streamify2() to smbiositem contained in
> libsmbios.lib just a copy of streamify().
> *	Additon of  ::streamify2() to smbiositemxml contained in
> libsmbiosxml.lib, the actual processing for XML output
> *	Addition of BeginTag() and EndTag() local functions contained in
> libsmbiosxml.lib used with streamify2() to tag output.
> *	Selection of streamify() versus streamify2() controled via Fill
> Character property in "cout" set to '<' to cause branch to
> streamify2()
> *	Modifications to dumpsmbios.exe to allow for overload of CLI
> param 1 to be "-x" or <memory file> used to set "cout" fill character
> 
> 			There are also some changes to the WorkSpace
> file to account for my local path to the Platform SDK and a Xerces_lib
> subdirectory structure to
> 			allow for easy changes between Xerces versions.
> 
> 			I  have to be out this afternoon, so I'll put
> together a "master" output XML file to ensure that our XSD will handle
> all variations that result from dynamic content.
> 			Have a great weekend and I'll see you on Monday.
> 
> 			Tim Fettig
> 			Authorized Dell Representative
> 			IT Guide Limited
> 			Technology Architect
> 			ph. (512) 576-7005
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.us.dell.com/pipermail/libsmbios-devel/attachments/20050726/d9979a4d/attachment-0001.htm


More information about the libsmbios-devel mailing list