libsmbios (ready to deliver?)

Michael_E_Brown@Dell.com Michael_E_Brown at Dell.com
Mon Jul 25 11:04:16 CDT 2005


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.

	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. 

	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.

	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)

	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.

	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

	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. 

	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/20050725/fb39c58b/attachment.htm


More information about the libsmbios-devel mailing list