ompi/opal/orte/oshmem/test: max hostname length cleanup #1552

ghost · 2016-04-16T01:17:30Z

Trying to standardize from where max hostname length is derived and how gethostname() called. This is low priority/cleanup, but touches a fair number of files.

I've left MPI_Get_processor_name() to continue using MPI_MAX_PROCESSOR_NAME, as the variable to which it writes the hostname is passed into the function. If for some reason hostname max lengths ever exceed MPI_MAX_PROCESSOR_NAME, we could segfault.

Also, programs in orte/test/ are not configured to build, and further, they have various build failures. I've added the hostname max len fixups to these files as well, knowing they don't build. Perhaps a future TODO :)

Fixes: #1551

hppritcha · 2016-04-18T15:16:19Z

@kmroz would you mind rebasing this down to a smaller set of commits? it helps in tracking when we/if we want to merge in to other branches. Squashing down to one isn't necessary, more like 2 or 3 would be fine.

ghost · 2016-04-18T15:35:05Z

@hppritcha - Will do.

jsquyres · 2016-04-19T13:45:30Z

ompi/runtime/ompi_mpi_abort.c

        host = ompi_process_info.nodename;
    } else {
-        gethostname(hostname, sizeof(hostname));
+        gethostname(hostname, MAXHOSTNAMELEN);


Is there a reason to make changes like this? I somewhat like the defensive coding style of using sizeof(foo) to indicate the length of an array. In this case, it's obviously MAXHOSTNAME, but using sizeof protects you in case the array changes size someday (i.e., there's only 1 place to update instead of 2).

Indeed, I wouldn't mind seeing:

Changing the size of all hostname-like arrays to MAXHOSTNAMELEN (which you do already in this PR)

Changing all references to the length of the array (e.g., passing as the 2nd arg to gethostname()) use sizeof(array_name).

I had a serious moral dilemma here ;) I agree we should go the sizeof() route.

jsquyres · 2016-04-19T14:04:32Z

@kmroz Pro tip: if you put "Fixes #1551" in a commit message, it'll auto-close that issue when this is merged. I don't remember offhand, but you might get the same effect if you put that string in the original description of this PR...?

ghost · 2016-04-19T14:45:20Z

@jsquyres Ah, cool! I didn't know this. We can test the "PR" hypothesis with this PR... provided I can get it into the right shape :)

bosilca · 2016-04-19T19:22:47Z

ompi/info/info.c

    /* local host name */
-    gethostname(val, MPI_MAX_INFO_VAL);
+    gethostname(val, MAXHOSTNAMELEN);
    ompi_info_set(&ompi_mpi_info_env.info, "host", val);


This is limited by MPI_MAX_INFO_KEY.

Thanks! I will look into changing this occurrence.

bosilca · 2016-04-19T19:24:55Z

So the plan is to change [where possible] to use sizeof instead of hardcoding the MAXHOSTNAMELEN ?

jsquyres · 2016-04-19T19:27:38Z

@bosilca I was specifically referring to using char foo[X] to size the arrays (e.g., X = MAXHOSTNAMELEN), but then use sizeof(foo) when passing the length of the array to something (e.g., gethostname()) -- just seems like good defensive programming.

bosilca · 2016-04-19T19:34:20Z

That's exactly what I was referring to. I was wondering if the plan was to change all the instances where sizeof(array[]) can be used (and thus provide a consistent usage) or just the one where it has been specifically mentioned ?

ghost · 2016-04-19T19:59:31Z

I will rework this PR to pass sizeof() to all invocations of gethostname().

jsquyres · 2016-04-19T19:59:47Z

This PR was just about MAXHOSTNAMELEN -- I don't think there's a larger effort to unify the whole code base on sizeof(foo). But I agree, that would be a Good Thing. 😄

ghost · 2016-04-19T20:04:20Z

Yes of course, I meant using sizeof() in the context of this PR 😄

bosilca · 2016-04-19T20:05:01Z

Much appreciated @kmroz that will put some consistency in the code. @jsquyres I wasn't talking about the entire code base, but at everything touched by this patch.

ghost · 2016-04-19T23:31:01Z

Rebased on top of current master, and s/MAXHOSTNAMELEN/sizeof()/ when invoking gethostname().

Some additional things:

In opal_config_bottom.h - if HOST_NAME_MAX is not defined in limits.h, I've increased MAXHOSTNAMELEN to the maximum possible value according to SUSv2 (ie. 255 + 1).
@bosilca - ompi/info/info.c:MPI_MAX_INFO_KEY = OPAL_MAX_INFO_VAL = 256. So, we shouldn't exceed this with gethostname() and MAXHOSTNAMELEN. Agreed?
pmix_config_bottom.h - includes limit.h, so we should get a valid HOST_NAME_MAX on which to base MAXHOSTNAMELEN, otherwise I've bumped it to 255 + 1.
I've mostly left get_processor_name.c:MPI_Get_processor_name() alone and using MPI_MAX_PROCESSOR_NAME for now. It's invoked in a number of places with varying allocations for *name. I think this would also benefit some cleanup but from another PR as it would touch fortran code, etc... and I've no idea yet what I'm doing there 😄
I've left win_compat.h alone. It defines MAXHOSTNAMELEN as _MAX_PATH. Any thoughts?

@jsquyres - If you feel it would be better to drop libevent/pmix modifications from this PR, I'm happy to rebase things without those changes. I could try to go straight upstream, but would need a pointer where upstream is.

Thanks again for all the reviews!

rhc54 · 2016-04-20T00:13:25Z

@kmroz Please do a "git diff opal/mca/pmix" and send me the results - I'll upstream them to the pmix master. You can leave them here as well for now and I'll update them with the next PMIx release.

ghost · 2016-04-20T07:07:28Z

@rhc54 - The diff you asked for is here: https://gist.github.com/kmroz/e786d5ea9e5dfd90ccc342d97792b997 I'd appreciate your feedback on pmix_config_bottom.h. The MAXHOSTNAMELEN section appears to only be included as part of #ifdef MCS_VXWORKS - which means all the pmix code using MAXHOSTNMELEN in my Linux build is actually getting it from elsewhere 😬 I assumed from opal_config_bottom.h, but introducing compiler errors there didn't break pmix. Any ideas where it might be picking up the macro?! I'll dig a little deeper tomorrow.

ghost · 2016-04-20T08:33:15Z

@rhc54 - I think I narrowed it down. In pmix code, MAXHOSTNAMELEN is taken from /usr/include/asm-generic/param.h. I've moved the #ifndef MAXHOSTNAMELEN higher in pmix_config_bottom.h to cover all environments.

Above gist updated with most recent pmix diffset.

rhc54 · 2016-04-22T17:48:10Z

Thanks - that indeed looks correct. I'll update this upstream, but let you include it in your commit here to avoid conflicts.

ghost · 2016-04-22T17:52:56Z

@rhc54 - Great, thanks!

jsquyres · 2016-04-23T10:20:02Z

@kmroz Sorry for the delay. Some (very minor) nits:

I wonder if we should rename MAXHOSTNAMELEN to be OPAL_MAXHOSTNAMELEN, given that we are adding one to it, and therefore it is not the same as the system MAXHOSTNAMELEN (if there is one). This would reinforce the idea that this value is guaranteed to have enough space for the \0 terminator.
In a small number of places, you multiply the length of the string by sizeof(char). That probably isn't necessary.

ghost · 2016-04-23T14:49:08Z

@jsquyres - No problem.

I've removed the sizeof(char) instances and updated the PR.
I will investigate s/MAXHOSTNAMELEN/OPAL_MAXHOSTNAMELEN/.

ghost · 2016-04-23T15:12:45Z

@jsquyres - Hmmm, did you have something like this in mind for both opal_config_bottom.h and pmix_config_bottom.h? Then, globally s/hostname[MAXHOSTNAMELEN]/hostname[OPAL_MAXHOSTNAMELEN]/.

    #ifndef MAXHOSTNAMELEN
    /* From limits.h. Since we're using gethostname(), we add 1 to the max to allow
     * for null termination.
     */
    #ifdef HOST_NAME_MAX
    #define OPAL_MAXHOSTNAMELEN HOST_NAME_MAX + 1
    #else
    /* SUSv2 guarantees that "Host names are limited to  255  bytes". */
    #define OPAL_MAXHOSTNAMELEN 255 + 1
    #endif
    #else
    #define OPAL_MAXHOSTNAMELEN MAXHOSTNAMELEN + 1
    #endif

rhc54 · 2016-04-23T15:37:21Z

Yes, that is what he is proposing. I would second it as it will avoid future confusion, if you wouldn't mind.

ghost · 2016-04-23T16:06:12Z

@jsquyres / @rhc54 - Hi gents!

OK - I think this should do it. Please have a look and let me know if you think I've missed something.

I've also modified win_compat.h to include a #define OPAL_MAXHOSTNAMELEN MAXHOSTNAMELEN + 1. Lastly, I've moved the #define OPAL_MAXHOSTNAMELEN block in opal_config_bottom.h out from the #ifdef MCS_VXWORKS block.

@rhc54 - I've updated the pmix diff (https://gist.github.com/kmroz/e786d5ea9e5dfd90ccc342d97792b997) with the latest.

jsquyres · 2016-04-23T16:54:13Z

@kmroz Perhaps a little simpler, without the negative logic (pro tip: put a "c" after the triple-single-tick markdown for verbatim, and it syntax highlights for C):

#if defined(MAXHOSTNAMELEN)
#define OPAL_MAXHOSTNAMELEN (MAXHOSTNAMELEN + 1)
#elif defined(HOST_NAME_MAX)
#define OPAL_MAXHOSTNAMELEN (HOST_NAME_MAX + 1)
#else
/* SUSv2 guarantees that "Host names are limited to 255 bytes". */
#define OPAL_MAXHOSTNAMELEN (255 + 1)
#endif

Thanks for your patience! Otherwise, I think it looks great.

ghost · 2016-04-23T17:01:46Z

@jsquyres - On it... will fix it up shortly.

ghost · 2016-04-23T17:22:59Z

@jsquyres - PR updated per your suggestion. Way cleaner, btw 😄 And thanks for the syntax highlighting tip!

@rhc54 - Above gist updated with latest pmix changes.

rhc54 · 2016-04-23T21:48:39Z

Ummm...not quite right. The changes in the pmix code base should be PMIX_MAXHOSTNAMELEN, not OPAL_...

Sorry to be a pain.

Define OPAL_MAXHOSTNAMELEN to be either: (MAXHOSTNAMELEN + 1) or (limits.h:HOST_NAME_MAX + 1) or (255 + 1) For pmix code, define above using PMIX_MAXHOSTNAMELEN. Fixup opal layer to use the new max. Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

ghost · 2016-04-24T06:22:16Z

@rhc54 - No pain at all! PR (specfically commit e1c64e6) and gist pmix.diff updated.

rhc54 · 2016-04-24T14:05:20Z

👍

Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

Also removes orte specific max hostname value. Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

ghost · 2016-05-02T13:01:56Z

@jsquyres - Is there anything else you see that's needed to improve upon this one?

jsquyres · 2016-05-02T13:44:16Z

Works for me. 👍

jsquyres · 2016-05-02T13:45:02Z

This is probably work PR'ing to the v2.x branch -- probably for v2.0.1. It's not critical functionality, by any means, but it will help reduce drift between master and the v2.x branch.

ghost · 2016-05-02T13:47:07Z

@jsquyres - Will do, thanks.

jsquyres reviewed Apr 19, 2016
View reviewed changes

ghost changed the title ~~ompi/opal/orte: max hostname length cleanup~~ DNM ompi/opal/orte: max hostname length cleanup Apr 19, 2016

bosilca reviewed Apr 19, 2016
View reviewed changes

ghost changed the title ~~DNM ompi/opal/orte: max hostname length cleanup~~ ompi/opal/orte: max hostname length cleanup Apr 19, 2016

ghost changed the title ~~ompi/opal/orte: max hostname length cleanup~~ ompi/opal/orte/oshmem/test: max hostname length cleanup Apr 23, 2016

kmroz added 4 commits April 25, 2016 07:08

ompi: fixup hostname max length usage

3322347

Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

orte: fixup hostname max length usage

5c11bdb

Also removes orte specific max hostname value. Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

test: fixup hostname max length usage

bb6ee60

Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

oshmem: fixup hostname max length usage

941f2c1

Signed-off-by: Karol Mroz <mroz.karol@gmail.com>

jsquyres merged commit 265e5b9 into open-mpi:master May 2, 2016

ompi/opal/orte/oshmem/test: max hostname length cleanup #1552

ompi/opal/orte/oshmem/test: max hostname length cleanup #1552

Uh oh!

Conversation

ghost commented Apr 16, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hppritcha commented Apr 18, 2016

Uh oh!

ghost commented Apr 18, 2016

Uh oh!

jsquyres Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

ghost commented Apr 19, 2016

Uh oh!

bosilca Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

bosilca commented Apr 19, 2016

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

bosilca commented Apr 19, 2016

Uh oh!

ghost commented Apr 19, 2016

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

ghost commented Apr 19, 2016

Uh oh!

bosilca commented Apr 19, 2016

Uh oh!

ghost commented Apr 19, 2016

Uh oh!

rhc54 commented Apr 20, 2016

Uh oh!

ghost commented Apr 20, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 20, 2016

Uh oh!

rhc54 commented Apr 22, 2016

Uh oh!

ghost commented Apr 22, 2016

Uh oh!

jsquyres commented Apr 23, 2016

Uh oh!

ghost commented Apr 23, 2016

Uh oh!

ghost commented Apr 23, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhc54 commented Apr 23, 2016

Uh oh!

ghost commented Apr 23, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsquyres commented Apr 23, 2016

Uh oh!

ghost commented Apr 23, 2016

Uh oh!

ghost commented Apr 23, 2016

Uh oh!

rhc54 commented Apr 23, 2016

Uh oh!

ghost commented Apr 24, 2016

Uh oh!

rhc54 commented Apr 24, 2016

Uh oh!

ghost commented May 2, 2016

Uh oh!

ghost commented Apr 16, 2016 •

edited by ghost

Loading

ghost commented Apr 20, 2016 •

edited by ghost

Loading

ghost commented Apr 23, 2016 •

edited by ghost

Loading

ghost commented Apr 23, 2016 •

edited by ghost

Loading