Hi Jim, (I'm going to cc the ipmitool-devel list so that other people can see your review and we don't have duplicate your work)
On Mon, Apr 1, 2013 at 2:24 PM, Jim Mankovich <jm...@hp.com> wrote: > Dan, > > Here are a couple of comments/questions on your code changes in this patch > set. I did > not seen anything broken , but I thought I would give you my thoughts on > your > changes. > > You split some longer lines and changed indentation without changing > anything > else. I don't see that your changes are really any better than what was > there before. > Why did you bother making these changes? Basically because I cannot change or understand code that I cannot read. Code which assumes that I have a 240 column screen, which puts error cases for every 'if' statement at the bottom, forcing 20 levels of indentation, variable levels of tab indentation, etc.. I just have to clean that stuff up before I can even start looking at something. To me it's kind of programming 101 to have a half way reasonable coding style so that people can read your code. The linux kernel style is not my favorite, but at least it's reasonable and consistent. > The code creating the pointer to the struct fru_picmgext_link_desc * from an > an unsigned > int data in ipmi_fru_picmg_ext_print() is making an assumption that the > sizeof(struct fru_picmgext_link_desc) is equal to the sizeof(int). Well, no, it's making the assumption that sizeof(struct fru_picmgext_link_desc) is less than or equal to an unsigned int... A PICMG channel descriptor is always three bytes long.. I don't see that changing ever really. PICMG would have to change the specification and break all existing code. > You could verify this is always the case via a compile time #if construct. I don't really see how you would even do that. > Another option would be to > make the bit fields in fru_picmgext_link_desc part of a union with an > unsigned int, but > that would require quite a bit of code change. I don't really see how that would help. The problem was with the way that unaligned bit fields are packed in gcc 4.1-4.3. With gcc in those versions this struct would be 5 bytes, not 3. That would not change by using a union. > I only noticed this because you changed data from unsigned long to unsigned > int. yes because the structure is only 3 bytes long. Recall that a long is 8 bytes on 64 bit machines. An unsigned int is always 4 bytes (unless we're talking about 16 or 8 bit processors here, which we're not...). Longs (unsigned or not) should really be avoided unless you're really prepared to deal with them being different sizes or 32 or 64 bit machines. > A similar assumption is being made for > struct fru_picmgext_amc_link_desc_record. Right.. These structures are defined in the specification. There is no reason to believe that they are ever going to change. If they do, then we'll change the code, but what I wrote doesn't assume any more about the size of the structure than was assumed before. It just works around the bug in gcc where it doesn't know how to pack unaligned bitfields. > The code as written is correct, so it does not really need to be changed. > I just thought I would point this out to you for future > reference. thanks for taking the time to look at these.. I appreciate it! thanks dan ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
