ekanalyzer incorrectly decodes FRU data

AlexanderAmelkin commented 10 years ago

Reported by: Alexander Amelkin Original Ticket: ipmitool/bugs/314

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

One extra byte of data is displayed for Internal Use Area
Internal Use Area is decoded regardless of its actual presence as per FRU header
If Chassis Info Area is not present, the size of Internal Use Area is calculated incorrectly
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:

Fixes incorrect behavior on FRUs without Internal Use Area (which is allowed by the specification)
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)
Reduces the amount of copy/pasted code and magic numbers
Corrects error messages to say the truth (that it couldn’t read something) instead of spitting out false claims (that something was “invalid”).
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

AlexanderAmelkin commented 10 years ago

status: open --> pending

Original comment by: Zdenek Styblik

AlexanderAmelkin commented 10 years ago

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

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

fclose() ?
```
:::C
close(input_file);
```

NULL pointer after free

:::C
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:

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

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

Original comment by: Zdenek Styblik

AlexanderAmelkin commented 10 years ago

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.

Original comment by: Alexander Amelkin

AlexanderAmelkin commented 10 years ago

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

:::C
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.

Original comment by: Zdenek Styblik

AlexanderAmelkin commented 10 years ago

What a crap code!!!

Original comment by: Zdenek Styblik

AlexanderAmelkin commented 10 years ago

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.

Original comment by: Alexander Amelkin

AlexanderAmelkin commented 10 years ago

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). :)

Original comment by: Zdenek Styblik

AlexanderAmelkin commented 10 years ago

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

Original comment by: Zdenek Styblik