orte/nidmap: correctly handle '-' as a valid hostname character #4627
Conversation
'-' is not an alpha character nor a digit, but it is a valid hostname character and should be handled as an alpha character, otherwise, nodes such as node-001 do not get "compressed" in the regex. Refs open-mpi#4621 Signed-off-by: Gilles Gouaillardet <[email protected]>
| break; | ||
| } | ||
| /* this is either an alpha or '-' */ | ||
| assert(isalpha(node[i]) || '-' == node[i]); |
There was a problem hiding this comment.
Something doesn't look right here - why would you want to assert if the character is illegal? I should think we wouldn't want the daemon to simply crash. Let's instead do a show_help reporting the unsupported name and return an error code.
| @@ -296,6 +289,13 @@ int orte_util_nidmap_create(opal_pointer_array_t *pool, char **regex) | |||
| } | |||
| continue; | |||
| } | |||
| if ('.' == node[i]) { | |||
There was a problem hiding this comment.
Not sure I understand why a dot character mandates use of the entire name - could you add a comment to explain that option?
There was a problem hiding this comment.
well, i did not fully understand the initial logic, so i just preserved it.
the initial test was
if (!isalpha(node[i]) && !isdigit(node[i])) {
fullname=true;
break;
}because node is a valid hostname, node[i] is either
- an
alpha - a
digit - the
-character - the
.character
i honestly do not see any reason why . should not be treated as a digit, so that means fullname is always false and hence i can remove some dead code. do you remember the rationale for the fullname test ?
at that stage, node should always be a valid hostname, so i did not feel the need to validate it (and hence the assert()). that being said, this is not a strong opinion, and i am fine
- checking
nodeis a valid hostname and error otherwise - remove the dead code
There was a problem hiding this comment.
IIRC, the rationale was that I didn't want to assume validity. The user can input a hostname in either a hostfile or a -host option, and we never check it for validity. Thus, the characters could be anything.
I suppose a better option would be to add a validity check in the hostfile and -host parsers, and then have a show_help and error out if a name fails. We could cite https://en.wikipedia.org/wiki/Hostname to explain the error.
Then I'd be comfortable just taking out the assert and assuming validity here, and the need for fullname goes away. Make sense?
rhc54
dismissed
their stale review
Let's go ahead and take this for now - I can look at the hostfile parser and upgrade it
'-' is not an alpha character nor a digit, but it is a valid hostname
character and should be handled as an alpha character, otherwise, nodes
such as node-001 do not get "compressed" in the regex.
Refs #4621
Signed-off-by: Gilles Gouaillardet [email protected]