On Mon, Jun 17, 2013 at 7:05 AM, Dmitry Bazhenov <dim...@pigeonpoint.com> wrote: > Hello, IPMITool maintainers team, > > Please, review the attached patch which does the following with the code in > lib/ipmi_fru.c: > > 1. Removes hard-coded 16-byte length FRU inventory device transactions and > introduces a command-line argument to setup such restriction for appropriate > devices. > 2. Fixes a bug where the algorithm incorrectly gathers FRU info blocks which > led to an impossibility to write data into a FRU device. > 3. Adds several code optimizations by organizing FRU blocks into a linked > list which is simpler to handle in a higher layer code. > 4. Replace absolute offset with the relative one which is used to access the > data buffer in the read_fru_area() API. Updated the dumping functions > correspondingly. > > Regards, > Dmitry >
Hello Dmitry, I've read through your diffs and my comments follow. I can only review code itself as I'm not aware of any FRU standards(nor I have looked hard enough for it). I'm mostly nit-picking - it looks ok in general, although I intend to apply the diff and see "the bigger picture". 1] ``const char *section_id[4]'' - is there no chance at all this is going to be extended at some point in future? If it is, I'd be for better solution. My point is ``for (i = 0; i < 4; i++)'' and if it gets extended so must code which follows/is using/iterate over it. But may be I'm wrong. 2] lprintf() doesn't need '\n' unlike printf() 3] some code formatting issues I've seen: ``sizeof (...)'', ``if ( condition)'', ``(char *) var'' ;; these are just examples and they're even inconsistent with your changes. I can fix these before commit eventually. 4] it seems to me some lines are way over 80 chars in length My $0.02 USD. Regards, Z. > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by Windows: > > Build for Windows Store. > > http://p.sf.net/sfu/windows-dev2dev > _______________________________________________ > Ipmitool-devel mailing list > Ipmitool-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/ipmitool-devel > ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
