Improving the code readability for Dell OEM commands #226
Conversation
…ility.
1. Removing the Deprecated Dell OEM command 'setled'.
This command is deprecated from 13th Generation Dell Servers onward.
2. Improving the code readability by
i. Removing the nested else-if's in main function and using switch-case.
ii. Removing the unnecessary function declaration,
by moving the main function to the bottom of file.
Resolves ipmitool#225
Signed-off-by: mohan_pujari <mohan.hoy@gmail.com>
|
I understand that |
Added Dell Servers generation check function. Added an API that frames and send/receive raw IPMI commands which is used accross the file. Resolves ipmitool#225 Signed-off-by: mohan_pujari <mohan.hoy@gmail.com>
Removed incorrectly placed ipmi_delloem.h file from include folder. added in include/ipmitool folder. Resolves ipmitool#225 Signed-off-by: mohan_pujari <mohan.hoy@gmail.com>
|
@AlexanderAmelkin |
mohanpujari
changed the title
|
@AlexanderAmelkin @vmauery @jaingaurav @chu11 |
|
@AlexanderAmelkin |
include/ipmitool/ipmi_delloem.h
Outdated
| #define MSG_LEN_0 0 | ||
| #define MSG_LEN_1 1 | ||
| #define MSG_LEN_2 2 | ||
| #define MSG_LEN_3 3 | ||
| #define MSG_LEN_4 4 | ||
| #define MSG_LEN_5 5 | ||
| #define MSG_LEN_6 6 | ||
| #define MSG_LEN_11 11 | ||
| #define MSG_LEN_13 13 | ||
| #define MSG_LEN_18 18 | ||
|
|
||
| //index Macro's | ||
| #define IDX_0 0 | ||
| #define IDX_1 1 | ||
| #define IDX_2 2 | ||
| #define IDX_3 3 | ||
| #define IDX_4 4 | ||
| #define IDX_5 5 | ||
| #define IDX_6 6 | ||
| #define IDX_7 7 | ||
| #define IDX_8 8 | ||
| #define IDX_9 9 | ||
| #define IDX_10 10 | ||
| #define IDX_11 11 | ||
| #define IDX_12 12 | ||
| #define IDX_13 13 | ||
| #define IDX_14 14 |
There was a problem hiding this comment.
These macros look:
- Unused (some)
- Truistic/useless
If they represent just a sequence number, then please remove them.
If the sequence number bears any meaning, then please disclose it in the macro's names, e.g. #define MSG_LEN_AUTHENTICATION 13 or #define MSG_LEN_SENSOR 6.
Anyway, if you don't use those macros in this PR, then don't add them at all.
There was a problem hiding this comment.
These represent just the sequence numbers. I will repost the code after correcting.
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| if((msg_len > 0 && msg_data == NULL) || cmd_rsp == NULL | ||
| || netfun == 0 || cmd == 0 ) { |
There was a problem hiding this comment.
Better have rvalue on the left of the comparison (e.g. SOME_MACRO == variable, not variable == SOME_MACRO). In case with NULL and 0 I prefer using boolean context for brevity:
if ((msg_len > 0 && !msg_data) || !cmd_rsp || !netfun || !cmd) {
...
}
There was a problem hiding this comment.
Will repost the after correction.
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| { | ||
| lprintf(LOG_ERR, "FM001 : A required license is missing or expired"); | ||
| return -1; | ||
| } else if ((rsp->ccode == 0xc1)||(rsp->ccode == 0xcb)) { |
There was a problem hiding this comment.
Those are IPMI_CC_INV_CMD and IPMI_CC_REQ_DATA_NOT_PRESENT from ipmi_cc.h, I suppose?
Then use them please, and mind the rvalue on the left as I said earlier.
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| lprintf(LOG_ERR, "Error getting in response "); | ||
| return -1; | ||
| } else if ((iDRAC_FLAG > IDRAC_11G) | ||
| && (rsp->ccode == LICENSE_NOT_SUPPORTED)) |
There was a problem hiding this comment.
LICENSE_NOT_SUPPORTED == rsp->ccode, please
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| lprintf(LOG_ERR, "FM001 : A required license is missing or expired"); | ||
| return -1; | ||
| } else if ((rsp->ccode == 0xc1)||(rsp->ccode == 0xcb)) { | ||
| if(cmd == 0x2d) |
There was a problem hiding this comment.
What's this 0x2d ? This needs a descriptive macro.
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| msg_data[IDX_0] = 0; // get cmd | ||
| msg_data[IDX_1] = IDRAC_VALIDATOR_PARAM; // parameter | ||
| msg_data[IDX_2] = 2; // block selector | ||
| msg_data[IDX_3] = 0; // set selector |
There was a problem hiding this comment.
Those IDX_* here could be:
#define IDRAC_VALIDATOR_CMD_IDX 0
#define IDRAC_VALIDATOR_PARAM_IDX 1
#define IDRAC_VALIDATOR_BLKSEL_IDX 2
#define IDRAC_VALIDATOR_SETSEL_IDX 3or better yet they could be an enum:
enum {
IDRAC_VALIDATOR_CMD_IDX,
IDRAC_VALIDATOR_PARAM_IDX,
IDRAC_VALIDATOR_BLKSEL_IDX,
IDRAC_VALIDATOR_SETSEL_IDX,
IDRAC_VALIDATOR_REQ_LEN
};
...
uint8_t msg_data[IDRAC_VALIDATOR_REQ_LEN] = { 0 };There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| int | ||
| oem_dell_idracvalidator_command(struct ipmi_intf *intf) | ||
| { | ||
| uint8_t msg_data[MSG_LEN_4] = {0}; |
There was a problem hiding this comment.
Also please use spaces around 0 in { 0 }
There was a problem hiding this comment.
Addressed the comment and reposted for review.
lib/ipmi_delloem.c
Outdated
| IPMI_GET_SYS_INFO, msg_data, MSG_LEN_4, &rsp) != 0) { | ||
| return -1; | ||
| } | ||
| switch (rsp->data[IDX_10]) { |
There was a problem hiding this comment.
Definitely not IDX_10. Maybe something like IDRAC_VALIDATOR_DEVICE_TYPE_OFFSET ?
There was a problem hiding this comment.
Addressed the comment and reposted for review.
| @@ -121,7 +121,15 @@ const struct vFlashstr vFlash_completion_code_vals[] = { | |||
| {0x63, "UNKNOWN_ERROR"}, | |||
| {0x00, NULL} | |||
| }; | |||
|
|
|||
| const struct valstr oem_dell_commands[] = { | |||
| {0x00, "lcd"}, | |||
There was a problem hiding this comment.
Definitely need macros or constants to those numbers. Especially taking in account that you (presumably) re-use exactly those numbers in the switch at ipmi_delloem.c:4279, but now it's not obvious and I can only guess if those numbers are really the same or it's just a coincidence.
There was a problem hiding this comment.
It is the same numbers used at switch statement in ipmi_delloem.c:4279 and not a coincidence.
Also, we plan to follow similar approach for sub commands, for future patches.
| return 0; | ||
| } | ||
| oem_dell_idracvalidator_command(intf); | ||
| switch (str2val(argv[0], oem_dell_commands)) { |
There was a problem hiding this comment.
What exactly was the benefit of replacing string comparisons in the original code with this call to str2val and introduction of oem_dell_commands array that isn't used anywhere except this switch?
There was a problem hiding this comment.
Planning to refactor the complete delloem commands code, for better readability, modularity, organized and structured code.
This is the first patch to achieve that goal. Hence introduced oem_dell_commands.
Addressed all the review comments Resolves ipmitool#225 Signed-off-by: mohan_pujari <mohan.hoy@gmail.com>
|
@AlexanderAmelkin Code changes requested are addressed. Please review. |
Improving the code readability for Dell OEM commands
i. Removing the nested else-if's in main function and using switch-case.
ii. Removing the unnecessary function declaration,
by moving the main function to the bottom of file.
Resolves #225
Signed-off-by: mohan_pujari mohan.hoy@gmail.com