opal/util: Change opal/util/if.h macro IF_NAMESIZE to OPAL_IF_NAMESIZE #6852
Conversation
|
Replaced #6849 with this PR. |
| #ifndef IF_NAMESIZE | ||
| #define IF_NAMESIZE 32 | ||
| #endif | ||
| #define OPAL_IF_NAMESIZE 32 |
There was a problem hiding this comment.
It would be probably worth putting a comment here explaining a little history, and where the magic value of 32 came from, etc. (I know that such an explanation didn't already exist, but given that we just have to have a conversation digging up the past, it means that there really should be a comment here explaining what/why/etc.).
There was a problem hiding this comment.
I'm not actually sure where the magic value of 32 comes from myself. I read in the other thread:
I could swear we recently had a debate about namesize and the value we should use to ensure that our array was at least as large as whatever the system used. I thought we resolved it be picking a size that was bigger than anything we might encounter (in Linux or whatever OS flavor we encounter). Is there some reason that fix wasn't adequate (I can't immediately find the PR where all that was discussed)?
I do not know the actual history behind this value though.
There was a problem hiding this comment.
I was referring to your commit message on #6849
In cases where opal/util/if.h is included but net/if.h is not, IF_NAMESIZE will be 32. If net/if.h is included on Linux systems, IF_NAMESIZE will be 16. This can cause a mismatch when using the same macro macro on a system. To avoid this error case, we include this file in util/if.h.
It would be good to include a little history here that we previously just #define'd IF_NAMESIZE to 32 blah blah blah... that was a bad idea because blah blah blah... so now we #define OPAL_IF_NAMESIZE to avoid the issue blah blah blah...
That kind of text. Something to help our Future Selves when someone looks at this code 6-12 months from now.
|
FWIW: Github pro tip: you didn't need to close #6849 and make a new PR -- you could have just doing a force-push on your old branch on the old PR and the PR would have updated itself with the new git commit(s). |
Due to IF_NAMESIZE being a reused and conditionally defined macro, issues could arise from macro mismatches. In particular, in cases where opal/util/if.h is included, but net/if.h is not, IF_NAMESIZE will be 32. If net/if.h is included on Linux systems, IF_NAMESIZE will be 16. This can cause a mismatch when using the same macro on a system. Thus different parts of the code can have differring ideas on the size of a structure containing a char name[IF_NAMESIZE]. To avoid this error case, we avoid reusing the IF_NAMESIZE macro and instead define our own as OPAL_IF_NAMESIZE. Signed-off-by: William Zhang <wilzhang@amazon.com>
Due to IF_NAMESIZE being a reused and conditionally defined macro,
issues could arise from macro mismatches. In particular, in cases where
opal/util/if.h is included, but net/if.h is not, IF_NAMESIZE will be 32.
If net/if.h is included on Linux systems, IF_NAMESIZE will be 16. This
can cause a mismatch when using the same macro on a system. Thus
different parts of the code can have differring ideas on the size of a
structure containing a char name[IF_NAMESIZE]. To avoid this error case,
we avoid reusing the IF_NAMESIZE macro and instead define our own as
OPAL_IF_NAMESIZE.
Signed-off-by: William Zhang wilzhang@amazon.com