-
Notifications
You must be signed in to change notification settings - Fork 364
Improving the code readability for Dell OEM commands #226
base: master
Are you sure you want to change the base?
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 <[email protected]>
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 <[email protected]>
Removed incorrectly placed ipmi_delloem.h file from include folder. added in include/ipmitool folder. Resolves ipmitool#225 Signed-off-by: mohan_pujari <[email protected]>
@AlexanderAmelkin |
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These represent just the sequence numbers. I will repost the code after correcting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will repost the after correction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LICENSE_NOT_SUPPORTED == rsp->ccode
, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this 0x2d
? This needs a descriptive macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 3
or 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please use spaces around 0
in { 0 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not IDX_10
. Maybe something like IDRAC_VALIDATOR_DEVICE_TYPE_OFFSET
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
@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 [email protected]