Just as a suggestion: please express such changes in the form of a Pull Request instead of a direct commit to avoid getting such mistakes into the code base.
I’m not advocating it for truly trivial stuff - but changing the thread_unlock to an OB1 call probably should be given a chance for comment > On Apr 7, 2016, at 10:45 AM, Nathan Hjelm <hje...@lanl.gov> wrote: > > > Hah, just caught that as well. Commented on the commit on > github. Definitely looks wrong. > > -Nathan > > On Thu, Apr 07, 2016 at 05:43:17PM +0000, Dave Goodell (dgoodell) wrote: >> [inline] >> >> On Apr 7, 2016, at 12:53 PM, git...@crest.iu.edu wrote: >>> >>> This is an automated email from the git hooks/post-receive script. It was >>> generated because a ref change was pushed to the repository containing >>> the project "open-mpi/ompi". >>> >>> The branch, master has been updated >>> via 92290b94e0584271d6459a6ab5923a04125e23be (commit) >>> from 7cdf50533cf940258072f70231a4a456fa73d2f8 (commit) >>> >>> Those revisions listed above that are new to this repository have >>> not appeared on any other notification email; so we list those >>> revisions in full, below. >>> >>> - Log ----------------------------------------------------------------- >>> https://github.com/open-mpi/ompi/commit/92290b94e0584271d6459a6ab5923a04125e23be >>> >>> commit 92290b94e0584271d6459a6ab5923a04125e23be >>> Author: Thananon Patinyasakdikul <tpati...@utk.edu> >>> Date: Wed Apr 6 14:26:04 2016 -0400 >>> >>> Fixed Coverity reports 1358014-1358018 (DEADCODE and CHECK_RETURN) >>> >>> diff --git a/ompi/mca/pml/ob1/pml_ob1_recvreq.c >>> b/ompi/mca/pml/ob1/pml_ob1_recvreq.c >>> index 9d1d402..a912bc3 100644 >>> --- a/ompi/mca/pml/ob1/pml_ob1_recvreq.c >>> +++ b/ompi/mca/pml/ob1/pml_ob1_recvreq.c >>> @@ -106,7 +106,7 @@ static int mca_pml_ob1_recv_request_cancel(struct >>> ompi_request_t* ompi_request, >>> /* The rest should be protected behind the match logic lock */ >>> OB1_MATCHING_LOCK(&ob1_comm->matching_lock); >>> if( true == request->req_match_received ) { /* way to late to cancel >>> this one */ >>> - OPAL_THREAD_UNLOCK(&ob1_comm->matching_lock); >>> + OB1_MATCHING_LOCK(&ob1_comm->matching_lock); >> >> I've only taken a cursory look, but this looks very wrong to me. Shouldn't >> you be using the "OB1_MATCHING_UNLOCK" macro instead? Doubly locking the >> lock will almost certainly lead to sadness. >> >>> assert( OMPI_ANY_TAG != ompi_request->req_status.MPI_TAG ); /* not >>> matched isn't it */ >>> return OMPI_SUCCESS; >>> } >>> diff --git a/opal/mca/btl/tcp/btl_tcp.h b/opal/mca/btl/tcp/btl_tcp.h >>> index f2c8917..7e9d726 100644 >>> --- a/opal/mca/btl/tcp/btl_tcp.h >>> +++ b/opal/mca/btl/tcp/btl_tcp.h >>> @@ -96,7 +96,7 @@ extern int mca_btl_tcp_progress_thread_trigger; >>> do { \ >>> if(0 < mca_btl_tcp_progress_thread_trigger) { \ >>> opal_event_t* _event = (opal_event_t*)(event); \ >>> - opal_fd_write( mca_btl_tcp_pipe_to_progress[1], >>> sizeof(opal_event_t*), \ >>> + (void) opal_fd_write( mca_btl_tcp_pipe_to_progress[1], >>> sizeof(opal_event_t*), \ >> >> Seems better to capture the return value and at least put an assert() on it >> if it fails, though admittedly things are very screwed up if you overrun the >> pipe. >> >> -Dave >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2016/04/18739.php > _______________________________________________ > devel mailing list > de...@open-mpi.org > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel > Link to this post: > http://www.open-mpi.org/community/lists/devel/2016/04/18740.php
