#314 ekanalyzer incorrectly decodes FRU data

[Alexander Amelkin]

As partly mentioned in my comment to ticket #53 (https://sourceforge.net/p/ipmitool/bugs/53/#c3f8),
ekanalyzer incorrectly decodes FRU data as follows:

1. One extra byte of data is displayed for Internal Use Area
2. Internal Use Area is decoded regardless of its actual presence as per FRU header
3. If Chassis Info Area is not present, the size of Internal Use Area is calculated incorrectly
4. The data in Custom Mfg. fields is not decoded according to the specification and is always output in hex.

The attached patch fixes all these issues and also cleans up the code a bit. Specifically:

1. Fixes incorrect behavior on FRUs without Internal Use Area (which is allowed by the specification)
2. Corrects size calculation for Internal Use Area (finds the start of the actual next section instead of blindly assuming that the next section is Chasiss Info)
3. Reduces the amount of copy/pasted code and magic numbers
4. Corrects error messages to say the truth (that it couldn’t read something) instead of spitting out false claims (that something was “invalid”).
5. Reduces nesting level by getting rid of “while(0)/break” in favor of simple “goto”.

There are two patches attached:

• eka-1.8.14.diff is the full patch
• eka-1.8.14-nospace.diff is the patch without whitespace changes, for code review only

[Zdenek Styblik]

Hello,

I did give your patch a look. Here are the comments:

• Please, give a look to http://sourceforge.net/p/ipmitool/wiki/coding_standards/

• unused variables

  uint8_t next_offset;
  int i; /* multiple times through the diff */
  

• fclose() ?

  close(input_file);
  

• NULL pointer after free

  free(data);
  data = NULL; /* missing multiple times */
  

Also, I must say I'd be willing to give it a shot to rewrite this code. I mean:

if (feof()) {
   goto done;
}
printf(...);
if (feof()) {
   goto done;
}
printf(...);
[...]

I don't mean you patch is bad, code in ipmi_ekanalyzer.c is.

[Alexander Amelkin]

Yes, code in ipmi_ekanalyzer.c is awful. I will look closer at your comments in context of my patch and will reply soon. Thank you.

[Zdenek Styblik]

One more thing I've recalled today. This didn't make sense to me, although I see it's in original code as well:

return (size_t)(-1);

Size can't be less than zero. Ok, this is wrong assumption according to http://stackoverflow.com/questions/1119370/where-do-i-find-the-definition-of-size-t However, let's assume size being 0 means error given the fact it can be either signed or unsigned int.

[Zdenek Styblik]

What a crap code!!!

[Alexander Amelkin]

Well, according to C99 size_t is unsigned, so (size_t)(-1) is a kind of oxymoron. On the other hand, if the caller performs a reverse typecast to (int), he will most probably get the actual (-1) back. Why complicate it like that is another interesting question.

[Zdenek Styblik]

according to C99 size_t is unsigned, so (size_t)(-1)

Therefore, size shoudln't be less than 0. (Yes, C99 compliance is/should be desirable). Also, it just doesn't make sense to me to have size less than zero(but hey, that's me). :)

[Zdenek Styblik]

Listen, don't worry about this one, ok? Let's get your patch in and then start with slight rewrite of the code.

[Hinko Kocevar]

How far before this gets accepted? I agree with the reported on the issues and we are seeing same problems when trying to show contents of the FMC FRU.
Sadly I can not get his patches to try them out..

[Zdenek Styblik]

Hello,

why can't you download the files? What's the error you're getting? I've re-attached them just to be sure.
I gave the patch quick look. If it still merges and doesn't produce more warnings, then I shouldn't be problem to get it merged. However, it would be really, really great to get rid of macros this patch brings in.

Z.

[Zdenek Styblik]

I see I had some comments there. May be author uploaded new version and didn't say he did so? This isn't clear to me :(

Z.

[Hinko Kocevar]

I'm getting :
Error 403
Moderate access required

for patches in the original post. Your I was able to download your attachments.. THANKS.

[Hinko Kocevar]

After applying the patch I was unable to dump info for FMC FRU which had type/length of 0xc1 = field of one byte in length. This is because 0xc1 is also 'end-of-fields' as per storage spec and patched code was not able to cope with that.

Unpatched can properly handle this type of field.

[Zdenek Styblik]

I see. Do you plan to re-work the patch?

[Hinko Kocevar]

Here is what I can say ATM about the fixes in initial post:

1. Solved by https://sourceforge.net/p/ipmitool/bugs/423/
2. Also solved by https://sourceforge.net/p/ipmitool/bugs/423/ ?

For case 3. and 5. I can provide separate patches that deal with those issues separately.

[Zdenek Styblik]

Are you sure? Because it looks to me the patch did more than that.

[Hinko Kocevar]

Well the patch touched some other stuff too. Mainly formatting and some log messages IMHO.
The things I can not test in that patch are Internal use area and custom mfg area parsing since I so not have suitable FRUs at hand.