[WIP] Add support for reading from a raw fru dump #9
Conversation
frugen.c
Outdated
| struct tm tm_1996 = { | ||
| .tm_year = 96, | ||
| .tm_mon = 0, | ||
| .tm_mday = 1 | ||
| }; | ||
| // The argument to mktime is zoneless | ||
| board.tv.tv_sec = mktime(&tm_1996) + 60 * min_since_1996; |
There was a problem hiding this comment.
There is already a very similar piece of code in fru.c, please think about some function that would allow to get rid of repetitions.
|
Thank you for your effort. |
fru.c
Outdated
| */ | ||
| static unsigned char *fru_decode_6bit(const fru_field_t *field) | ||
| static bool fru_decode_6bit(uint8_t typelen, //<[in] typelen of encoded buffer | ||
| const uint8_t *data, //< [in] buffer containing encoded 6bit ascii |
There was a problem hiding this comment.
What was the reason for ditching the fru_field_t in favor of separate typelen and data ?
There was a problem hiding this comment.
To avoid an extra memcpy. It does make things a bit messier, so I guess I'm not entirely convinced it's worth it.
There was a problem hiding this comment.
I don't exactly get how ditching fru_field_t helps in avoiding any extra memcpy.
I can see how changing from internal to external allocation helps in that, but not the fru_field_t.
fru.c
Outdated
| if (out_len < (FRU_FIELDDATALEN(typelen) + 1)) | ||
| return false; |
There was a problem hiding this comment.
Indentation looks very strange here
fru.c
Outdated
| if (custom_field == NULL) | ||
| return false; | ||
|
|
||
| // Create a NUL terminated verion of the data for encoding |
There was a problem hiding this comment.
verSion. a typo.
fru.c
Outdated
| const uint8_t *curr_data = area->data; | ||
| typelen = curr_data[0]; | ||
| curr_data++; |
There was a problem hiding this comment.
Why don't you use fru_field_t here? It was specifically created for this.
|
|
||
| free(product_raw); | ||
| has_product = true; | ||
| } |
There was a problem hiding this comment.
Looks like you're losing the "internal use" and "multi-record" areas here. The output file won't have them even if the original raw file did. That may defeat the purpose of your change as I understand it.
There was a problem hiding this comment.
we don't need those right now. If it's alright, I'd like to omit support for multirec and internal in the output for now. The internal area is pretty straightforward, but the multirec area seems to require some parsing, which I'd prefer to not have to implement for this PR.
There was a problem hiding this comment.
That's ok because libfru initially didn't support creating those areas, but please specify in the help that users may lose their pre-existing areas when using --raw, and also please update the README.md with the information about the new feature and its limitations.
|
|
||
| free(raw_fru); | ||
| close(fd); | ||
| } |
There was a problem hiding this comment.
Also, that code block is large enough for a separate function.
In the next patch this will allow avoiding some memcpys.
fru.c
Outdated
| */ | ||
| static unsigned char *fru_decode_6bit(const fru_field_t *field) | ||
| static bool fru_decode_6bit(uint8_t typelen, //<[in] typelen of encoded buffer | ||
| const uint8_t *data, //< [in] buffer containing encoded 6bit ascii |
There was a problem hiding this comment.
I don't exactly get how ditching fru_field_t helps in avoiding any extra memcpy.
I can see how changing from internal to external allocation helps in that, but not the fru_field_t.
fru.c
Outdated
| uint8_t typelen; | ||
|
|
||
| chassis_out->type = area->langtype; | ||
| const uint8_t *curr_data = area->data; |
There was a problem hiding this comment.
The declaration still needs to be moved to the declarations block, before the code starts.
|
|
||
| free(product_raw); | ||
| has_product = true; | ||
| } |
There was a problem hiding this comment.
That's ok because libfru initially didn't support creating those areas, but please specify in the help that users may lose their pre-existing areas when using --raw, and also please update the README.md with the information about the new feature and its limitations.
aneeshdurg
changed the title
The motivation for this is to use
ipmitoolto dump the current fru contents and then modify/add fields before writing it back.