Skip to content

orte/nidmap: correctly handle '-' as a valid hostname character #4627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 19, 2017
Merged

orte/nidmap: correctly handle '-' as a valid hostname character #4627

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions orte/util/nidmap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,22 +270,15 @@ int orte_util_nidmap_create(opal_pointer_array_t *pool, char **regex)
}
}
node = nptr->name;
/* determine this node's prefix by looking for first non-alpha char */
/* determine this node's prefix by looking for first digit char */
fullname = false;
len = strlen(node);
startnum = -1;
memset(prefix, 0, ORTE_MAX_NODE_PREFIX);
numdigits = 0;
for (i=0, j=0; i < len; i++) {
if (!isalpha(node[i])) {
/* found a non-alpha char */
if (!isdigit(node[i])) {
/* if it is anything but a digit, we just use
* the entire name
*/
fullname = true;
break;
}
/* valid hostname characters are ascii letters, digits and the '-' character. */
if (isdigit(node[i])) {
/* count the size of the numeric field - but don't
* add the digits to the prefix
*/
Expand All @@ -296,6 +289,13 @@ int orte_util_nidmap_create(opal_pointer_array_t *pool, char **regex)
}
continue;
}
if ('.' == node[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand why a dot character mandates use of the entire name - could you add a comment to explain that option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 node is a valid hostname and error otherwise
  • remove the dead code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/* just use the entire name */
fullname = true;
break;
}
/* this is either an alpha or '-' */
assert(isalpha(node[i]) || '-' == node[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (startnum < 0) {
prefix[j++] = node[i];
}
Expand Down