-
Notifications
You must be signed in to change notification settings - Fork 329
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.
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
Test the return value of omMMapBinaryFile function and terminate the main program elegantly #3002
Changes from 1 commit
afa4cff
03b00ca
79b9b95
7f8d1e0
c0b0a4d
8156452
9171ead
d0f8bcb
1d93a7d
0ae13e7
1042130
4f88c79
e9f794b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,7 @@ const int i = 1; | |
void checkEndianness(const char constPackIsLE) { | ||
if (XOR(IS_SYSTEM_LE(), constPackIsLE)) { | ||
fprintf(stderr, "Constant pack is stored in a byte order that is not " | ||
"native to this current system."); | ||
"native to this current system: %s", strerror(errno)); | ||
exit(1); | ||
} | ||
} | ||
|
@@ -67,15 +67,15 @@ bool omMMapBinaryFile( | |
// Convert the file name to EBCDIC for the open call. | ||
char *tPath = strdup(fname); | ||
if (!tPath) { | ||
fprintf(stderr, "Error while strdup"); | ||
fprintf(stderr, "Error while strdup: %s", strerror(errno)); | ||
return false; | ||
} | ||
__a2e_s(tPath); | ||
fname = tPath; | ||
#endif | ||
|
||
if (constAddr == NULL) { | ||
perror("Error: null pointer"); | ||
fprintf(stderr, "Error: null pointer: %s", strerror(errno)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
#ifdef __MVS__ | ||
free(tPath); | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a big deal but a bit of an eye sore to see the
and then just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gongsu832 all of these issues come from the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK that makes things simpler. |
||
|
@@ -89,25 +89,22 @@ bool omMMapBinaryFile( | |
size_t baseLen = strlen(basePath); | ||
size_t fnameLen = strlen(fname); | ||
size_t sepLen = strlen(DIR_SEPARATOR); | ||
size_t filePathLen = baseLen + sepLen + fnameLen; | ||
size_t filePathLen = baseLen + sepLen + fnameLen + 1; | ||
filePath = (char *)malloc(filePathLen); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably want to +1 to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I updated this. |
||
if (!filePath) { | ||
fprintf(stderr, "Error while malloc"); | ||
fprintf(stderr, "Error while malloc: %s", strerror(errno)); | ||
#ifdef __MVS__ | ||
free(tPath); | ||
#endif | ||
return false; | ||
} | ||
memcpy(filePath, basePath, baseLen); | ||
memcpy(filePath + baseLen, DIR_SEPARATOR, sepLen); | ||
memcpy(filePath + baseLen + sepLen, fname, fnameLen); | ||
filePath[filePathLen] = '\0'; | ||
snprintf(filePath, filePathLen, "%s%s%s", basePath, DIR_SEPARATOR, fname); | ||
} else { | ||
filePath = (char *)fname; | ||
} | ||
int fd = open(filePath, O_RDONLY); | ||
if (fd < 0) { | ||
fprintf(stderr, "Error while opening %s\n", filePath); | ||
fprintf(stderr, "Error while opening %s: %s\n", filePath, strerror(errno)); | ||
if (basePath) | ||
free(filePath); | ||
#ifdef __MVS__ | ||
|
@@ -123,7 +120,7 @@ bool omMMapBinaryFile( | |
#endif | ||
|
||
if (tempAddr == MAP_FAILED) { | ||
fprintf(stderr, "Error while mmapping %s\n", fname); | ||
fprintf(stderr, "Error while mmapping %s: %s\n", fname, strerror(errno)); | ||
close(fd); | ||
if (basePath) | ||
free(filePath); | ||
|
@@ -158,7 +155,9 @@ bool omMMapBinaryFile( | |
/* Make sure constAddr is the same as the mmap address. | ||
*/ | ||
if (constAddr[0] != tempAddr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is wrong. Only the first thread that successfully performs the compare-and-swap will have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I explicitly set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure why you want to do that. A successful compare-and-swap guarantees that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you are right. In the latest code, I set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to avoid the situation we encountered where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I removed the check. I was overthinking about having a check to debug, but since we found the issue, and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you can use |
||
fprintf(stderr, "Error while updating the buffer address for constants\n"); | ||
fprintf(stderr, | ||
"Error while updating the buffer address for constants: %s\n", | ||
strerror(errno)); | ||
return false; | ||
} | ||
return true; | ||
|
@@ -181,11 +180,11 @@ bool omMMapBinaryFile( | |
void omGetExternalConstantAddr( | ||
void **outputAddr, void **baseAddr, int64_t offset) { | ||
if (outputAddr == NULL) { | ||
perror("Error: null pointer"); | ||
fprintf(stderr, "Error: null pointer: %s", strerror(errno)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return; | ||
} | ||
if (baseAddr == NULL) { | ||
perror("Error: null pointer"); | ||
fprintf(stderr, "Error: null pointer: %s", strerror(errno)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return; | ||
} | ||
// Constant is already loaded. Nothing to do. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I probably didn't make it clear.
errno
is only meaningful after you make a library call such asstrdup
,open
,malloc
, etc. Herestrerror(errno)
is meaningless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thought. Will remove them.