fixes #104, Clean up use of strcmp/strncmp/strncasecmp #185
Conversation
|
Thank you so much for your effort! |
| @@ -647,7 +647,7 @@ uint32_t str2val32(const char *str, const struct valstr *vs) | |||
| int i; | |||
|
|
|||
| for (i = 0; vs[i].str; i++) { | |||
| if (strncasecmp(vs[i].str, str, __maxlen(str, vs[i].str)) == 0) | |||
| if (strcasecmp(vs[i].str, str) == 0) | |||
There was a problem hiding this comment.
This needs a comment above on why we're using a case-insensitive comparison here.
I suspect that's because the strings in valstr structures are usually meant for output and may be capitalized. But I'm not sure that it's the case. What will break if we use plain strcmp() here?
There was a problem hiding this comment.
The current code use val2str major on user input parameter matching to a specific number. Take 'event' code as an example, when previous application use sensor state <lnr|lcr|lnc|unc|ucr|unr>, so before a user can use uppercase LNR or Lnr, it will match to the predefined string and get the index. Using strcmp() will strictly use the predefined string in the code.
lib/ipmi_event.c
Outdated
| @@ -270,9 +270,9 @@ ipmi_event_fromsensor(struct ipmi_intf * intf, char * id, char * state, char * e | |||
|
|
|||
| if (!evdir) | |||
| emsg.event_dir = EVENT_DIR_ASSERT; | |||
| else if (strncasecmp(evdir, "assert", 6) == 0) | |||
| else if (strcasecmp(evdir, "assert") == 0) | |||
There was a problem hiding this comment.
I don't think we need case-insensitive comparison here
There was a problem hiding this comment.
The reason is for backward compatibility. Previous wrote script use case-insensitive parameter will suffer from this change. Anyway, I think it's a tradeoff. So if the decision is to use lower case anyway, I can submit the patch again.
lib/ipmi_event.c
Outdated
| emsg.event_dir = EVENT_DIR_ASSERT; | ||
| else if (strncasecmp(evdir, "deassert", 8) == 0) | ||
| else if (strcasecmp(evdir, "deassert") == 0) |
lib/ipmi_event.c
Outdated
| @@ -318,7 +318,7 @@ ipmi_event_fromsensor(struct ipmi_intf * intf, char * id, char * state, char * e | |||
| int hilo = 0; | |||
| off = 1; | |||
|
|
|||
| if (!state || strncasecmp(state, "list", 4) == 0) { | |||
| if (!state || strcasecmp(state, "list") == 0) { | |||
lib/ipmi_event.c
Outdated
| 0 != strcasecmp(state, "lnc") && | ||
| 0 != strcasecmp(state, "unc") && | ||
| 0 != strcasecmp(state, "ucr") && | ||
| 0 != strcasecmp(state, "unr")) |
There was a problem hiding this comment.
or here as well, or anywhere else except for maybe that str2val function.
src/plugins/ipmi_intf.c
Outdated
| @@ -509,7 +509,7 @@ ipmi_intf_get_max_request_data_size(struct ipmi_intf * intf) | |||
| /* check if request size is not specified */ | |||
| if (!size) { | |||
| /* | |||
| * The IPMB standard overall message length for �non -bridging� | |||
| * The IPMB standard overall message length for �non -bridging� | |||
There was a problem hiding this comment.
Ehm... what are these new characters? Anyway, this change doesn't look relevant to this PR.
There was a problem hiding this comment.
I am sorry for that, they are <91> and <92> no printable character in that, and my editor just saves it to another character.
src/plugins/ipmi_intf.c
Outdated
| @@ -566,7 +566,7 @@ ipmi_intf_get_max_response_data_size(struct ipmi_intf * intf) | |||
| /* check if response size is not specified */ | |||
| if (!size) { | |||
| /* | |||
| * The IPMB standard overall message length for �non -bridging� | |||
| * The IPMB standard overall message length for �non -bridging� | |||
There was a problem hiding this comment.
Same, I will make sure only the intended change in the commit.
|
Should I squash the commits into one? |
Clean up use of strcmp/strncmp/strncasecmp for command line arguments. Never use anything but `strcmp()` unless absolutely neccessary. Partialy resolves ipmitool#104
Reduce code duplication by extracting option names, types, and value ranges into a separate structure, and rewriting the option parsing code without mixing it with the data. Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>
Unify the comparison idioms use. Always use `if(!strcmp())` for "if string equals" and `if(strcmp())` for "if string is not equal". Never use `== 0` and `!= 0` with `strcmp()`. Minor reformatting of the code immediately surrounding the refactored lines. Resolves ipmitool#104 Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>
No description provided.