-
Notifications
You must be signed in to change notification settings - Fork 882
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
sessions: add missing errhandler funcs #10442
Conversation
These missing functions are a blocker for v5.0; we can't claim sessions support without them. |
bot:ompi:retest |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
somehow managed to not get into the original sessions pr. also do some cleanup of the use mpi_f08 profiling functions. Related to open-mpi#10388 Related to open-mpi#9097 Signed-off-by: Howard Pritchard <howardp@lanl.gov>
b030970
to
e5c3795
Compare
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.
A couple of comments, but nothing to stop approval. I think the branch needs rebasing, but otherwise good to go.
@@ -513,7 +513,6 @@ typedef int (MPI_Type_delete_attr_function)(MPI_Datatype, int, | |||
typedef int (MPI_Win_copy_attr_function)(MPI_Win, int, void *, | |||
void *, void *, int *); | |||
typedef int (MPI_Win_delete_attr_function)(MPI_Win, int, void *, void *); | |||
typedef int (MPI_Session_delete_attr_function)(MPI_Session, int, void *, void *); |
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.
This change already came in #10499 - maybe the branch needs to be rebased?
/* Ditch the old errhandler, and decrement its refcount. */ | ||
tmp = session->error_handler; | ||
session->error_handler = errhandler; | ||
OBJ_RELEASE(tmp); |
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.
Do you need to check if this is a predefined error handler before releasing it? The logic in this function follows MPI_Comm_set_errhandler
so it should be ok. But worth asking the question.
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.
good point. I think we may want protection in the mPI_Errhandler object destructor to catch this. But this can be for another PR.
thanks @jjhursey . doesn't look like there's a conflict so merging. |
somehow managed to not get into the original sessions pr.
also do some cleanup of the use mpi_f08 profiling functions.
Related to #10388
Related to #9097
Signed-off-by: Howard Pritchard howardp@lanl.gov