Skip to content

Fixed memory leak and some -Werror=unused-result warnings #2509

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 27, 2016
Merged

Fixed memory leak and some -Werror=unused-result warnings #2509

Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ Josh Hursey, Indiana University, Oak Ridge National Laboratory, Los Alamos Natio
jjhursey@open-mpi.org
Joshua Gerrard
enquiries@joshuagerrard.com
joshuagerrard+ompi-commit@protonmail.com
Copy link
Member

@jsquyres jsquyres Dec 17, 2016

Choose a reason for hiding this comment

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

@OMGtechy Are these the emails that you committed with? We typically only show the emails that show up in git log.

Copy link
Contributor Author

@OMGtechy OMGtechy Dec 17, 2016

Choose a reason for hiding this comment

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

One of them is - I've updated accordingly.

Joshua Ladd, Mellanox
jladd.mlnx@gmail.com
joshual@mellanox.com
Expand Down
77 changes: 59 additions & 18 deletions ompi/errhandler/errhandler_predefined.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,43 +177,84 @@ static void backend_fatal_aggregate(char *type,
char *name, int *error_code,
va_list arglist)
{
char *arg, *prefix, *err_msg = "Unknown error";
bool err_msg_need_free = false;
char *arg = NULL, *prefix = NULL, *err_msg = NULL;
const char* const unknown_error_code = "Error code: %d (no associated error message)";
const char* const unknown_error = "Unknown error";
const char* const unknown_prefix = "[?:?]";

// these do not own what they point to; they're
// here to avoid repeating expressions such as
// (NULL == foo) ? unknown_foo : foo
const char* usable_prefix = unknown_prefix;
const char* usable_err_msg = unknown_error;

arg = va_arg(arglist, char*);
va_end(arglist);

asprintf(&prefix, "[%s:%d]", ompi_process_info.nodename,
(int) ompi_process_info.pid);
if (asprintf(&prefix, "[%s:%d]",
ompi_process_info.nodename,
(int) ompi_process_info.pid) == -1) {
prefix = NULL;
// non-fatal, we could still go on to give useful information here...
opal_output(0, "%s", "Could not write node and PID to prefix");
Copy link
Member

@jsquyres jsquyres Dec 17, 2016

Choose a reason for hiding this comment

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

Just curious -- why do the double-string form? You shouldn't need to use the copy through %s -- you could just put the 3rd argument directly as the 2nd arg.

Copy link
Contributor Author

@OMGtechy OMGtechy Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's a habit to stop situations like this:

opal_output(0, user_input, ...)

Some compilers will also omit warnings if you don't do it this way, IIRC

Copy link
Member

@jsquyres jsquyres Dec 27, 2016

Choose a reason for hiding this comment

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

FWIW, I think compilers only warn on that for libc functions (e.g., printf). They don't for opal_output. But it doesn't matter that much...

opal_output(0, "Node: %s", ompi_process_info.nodename);
opal_output(0, "PID: %d", (int) ompi_process_info.pid);
}

if (NULL != error_code) {
err_msg = ompi_mpi_errnum_get_string(*error_code);
if (NULL == err_msg) {
err_msg_need_free = true;
asprintf(&err_msg, "Error code: %d (no associated error message)",
*error_code);
if (asprintf(&err_msg, unknown_error_code,
*error_code) == -1) {
Copy link
Member

@jsquyres jsquyres Dec 17, 2016

Choose a reason for hiding this comment

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

Will a failed asprintf() leave NULL in err_msg? Or could it be some random value (that free(err_msg), below, will choke on)?

Copy link
Contributor Author

@OMGtechy OMGtechy Dec 17, 2016

Choose a reason for hiding this comment

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

Good spot! I also made the same fix for prefix

err_msg = NULL;
opal_output(0, "%s", "Could not write to err_msg");
Copy link
Member

@jsquyres jsquyres Dec 17, 2016

Choose a reason for hiding this comment

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

Same comment as above -- %s doesn't seem to be needed...?

Copy link
Contributor Author

@OMGtechy OMGtechy Dec 17, 2016

Choose a reason for hiding this comment

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

See other comment :)

opal_output(0, unknown_error_code, *error_code);
}
}
}

usable_prefix = (NULL == prefix) ? unknown_prefix : prefix;
usable_err_msg = (NULL == err_msg) ? unknown_error : err_msg;

if (NULL != name) {
opal_show_help("help-mpi-errors.txt",
"mpi_errors_are_fatal", false,
prefix, (NULL == arg) ? "" : "in",
"mpi_errors_are_fatal",
false,
usable_prefix,
(NULL == arg) ? "" : "in",
(NULL == arg) ? "" : arg,
prefix, OMPI_PROC_MY_NAME->jobid, OMPI_PROC_MY_NAME->vpid,
prefix, type, name, prefix, err_msg, prefix, type, prefix);
usable_prefix,
OMPI_PROC_MY_NAME->jobid,
OMPI_PROC_MY_NAME->vpid,
usable_prefix,
type,
name,
usable_prefix,
usable_err_msg,
usable_prefix,
type,
usable_prefix);
} else {
opal_show_help("help-mpi-errors.txt",
"mpi_errors_are_fatal unknown handle", false,
prefix, (NULL == arg) ? "" : "in",
"mpi_errors_are_fatal unknown handle",
false,
usable_prefix,
(NULL == arg) ? "" : "in",
(NULL == arg) ? "" : arg,
prefix, OMPI_PROC_MY_NAME->jobid, OMPI_PROC_MY_NAME->vpid,
prefix, type, prefix, err_msg, prefix, type, prefix);
usable_prefix,
OMPI_PROC_MY_NAME->jobid,
OMPI_PROC_MY_NAME->vpid,
usable_prefix,
type,
usable_prefix,
usable_err_msg,
usable_prefix,
type,
usable_prefix);
}

if (err_msg_need_free) {
free(err_msg);
}
free(prefix);
free(err_msg);
}

/*
Expand Down