Hello, Zdenek, Please, see my comments below
24.06.2013 13:41, Zdenek Styblik пишет: > 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. Is 'for ( i = 0; i < sizeof(section_id) / siceof(section_id[0]); i++)' solves the issue? > > 2] lprintf() doesn't need '\n' unlike printf() Ok. > > 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. Ok. > > 4] it seems to me some lines are way over 80 chars in length Ok. Will come up with the addressed patch shortly. Regards, Dmitry > > 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
