ompi_comm_enable: always OBJ_RELEASE new_comm if an error occurs

[ggouaillardet]

and simplify the error handling in topo/base upon ompi_comm_enable
invokation

[ggouaillardet]

@bosilca could you please review this ?
this addresses the issue you reported at http://www.open-mpi.org/community/lists/devel/2015/03/17119.php

[mellanox-github]

• Coverity found 928 errors for all: http://bgate.mellanox.com:8888/jenkins/job/gh-ompi-master-pr//ws/cov_build/all_338/output/errors/index.html
• Coverity found 11 errors for oshmem: http://bgate.mellanox.com:8888/jenkins/job/gh-ompi-master-pr//ws/cov_build/oshmem_338/output/errors/index.html
• Coverity found 2 errors for fca: http://bgate.mellanox.com:8888/jenkins/job/gh-ompi-master-pr//ws/cov_build/fca_338/output/errors/index.html
  
  Sent from GH.

[mellanox-github]

Refer to this link for build results (access rights to CI server needed):
http://bgate.mellanox.com/jenkins/job/gh-ompi-master-pr/338/
Test PASSed.

[bosilca]

You cannot rely on the OBJ_RELEASE to correctly destroy the communicator for 2 reasons:

1. The reference count on the communicator is tricky, and in some instances one can have 2 refs (thus one should call ompi_communicator_free at least once).
2. the destructor doesn't free all the MPI level objects (topology and friends). Instead it simply release the groups, call the PML del_comm and remove the communicator from the cid and F2C tables.

Moreover, I don't even see the OBJ_RELEASE in this patch, but the call to ompi_communicator_free went missing. What was the original issue you were trying to fix?

[ggouaillardet]

the original issue, is that

• if ompi_comm_enable fails in ompi_comm_nextcid or ompi_comm_fill_rest,
  then new_comm is left unchanged
• if ompi_comm_enable fails in ompi_comm_activate, then new_comm is OBJ_RELEASE'd
  (OBJ_RELEASE occurs in ompi_comm_activate)

at least in mca_topo_base_cart_create, reference_count of new_comm is always 1
(i checked this with a debugger), so if ompi_comm_activate fails, new_comm is released

back to mca_topo_base_cart_create, if ompi_comm_enable fails, new_comm might have been released or not ... so invoking ompi_comm_free on new_comm might crash.

communicator destructor does OBJ_RELEASE comm->c_topo if it is not NULL
( i am not sure of what you meant by "opology and friends")

[hppritcha]

Where are we with this PR? Should it be closed without merge?

[jsquyres]

@bosilca @ggouaillardet How's it going on this PR? :)

[hppritcha]

Can one of the admins verify this patch?

[bosilca]

I looked at the topo_base_cart_create.c, and it seems that this patch introduced some memory leaks. To be more precise with this patch the newly created communicator (new_comm) and the newly created cart and topo are not released.

[jsquyres]

@ggouaillardet Can you address @bosilca's comments (#459 (comment))?

[rhc54]

@ggouaillardet Can we bring this to conclusion? Been hanging around for a long time.

[ggouaillardet]

let s close this PR
that was just a corner case in the error path anyway