fixes #172, Add support for editing FRU 'board mfg date' #328
Conversation
include/ipmitool/ipmi_fru.h
Outdated
| @@ -598,21 +598,34 @@ struct fru_picmgext_amc_link_desc_record { | |||
| /* FRU Board manufacturing date */ | |||
| #define FRU_BOARD_DATE_UNSPEC 0 /* IPMI FRU Information Storage Definition | |||
| v1.0 rev 1.3, Table 11-1 */ | |||
|
|
|||
| static inline uint32_t SECS_FROM_1970_1996=820454400; | |||
There was a problem hiding this comment.
Why did you demote it to uint32_t while moving it to global namespace?
Actually, I think it would be the best to use time_t. And it has to be const, on the contrary, inline doesn't seem to do anything here.
lib/ipmi_fru.c
Outdated
| ipmi_time2fru(total_secs, board_mfg_date); | ||
|
|
||
| checksum = 0; | ||
| /* Calculate Header Checksum */ |
There was a problem hiding this comment.
Indent the comment to the same level with the code, please.
| uint32_t *board_mfg_date=&fru_data[3]; | ||
|
|
||
| total_secs = ipmi_strptime("%Y-%m-%d %H:%M:%S", f_string); | ||
| ipmi_time2fru(total_secs, board_mfg_date); |
There was a problem hiding this comment.
looks like a range check is required here. we don't want values that exceed 24 bits, but ipmi_strptime is capable of returning greater values.
There was a problem hiding this comment.
How should we check the range of the total_secs?
The total_secs will be subtracted by SECS_FROM_1970_1996 and divided by 60 to get the minutes and mask down to three bytes. I think this is safe here.
| checksum += fru_data[i]; | ||
| } | ||
| checksum = (~checksum) + 1; | ||
| fru_data[fru_section_len - 1] = checksum; |
There was a problem hiding this comment.
use ipmi_csum() please
There was a problem hiding this comment.
Also, it looks to me like all this FRU update code forgets to update the overall FRU checksum after modifying an area.
There was a problem hiding this comment.
since we update the mfg date only and we do not change any length of the records.
I don't think the common header field checksum will be changed.
|
Please let me know if there are any changes required. |
based on discussion #172 (comment)