update to support node namespaces in rmw API #220
Conversation
wjwwood
added
enhancement
| node_handle->name_space = | ||
| reinterpret_cast<const char *>(rmw_allocate(sizeof(char) * strlen(name_space) + 1)); | ||
| if (!node_handle->name_space) { | ||
| RMW_SET_ERROR_MSG("failed to allocate memory for node name"); |
There was a problem hiding this comment.
Thanks fixed in 930df8f.
wjwwood
added
in review
| if (!participant_qos.participant_name.name) { | ||
| RMW_SET_ERROR_MSG("failed to allocate memory"); | ||
| return NULL; | ||
| } | ||
| participant_qos.participant_name.name = strdup(name); |
There was a problem hiding this comment.
isn't that a potential memory leak here? We call rmw_allocate beforehand and then in line 330 strdup allocates again memory for the string internally.
Sorry for comment on this line, it's technically not part of the PR.
There was a problem hiding this comment.
Yes... I was just adding the check that I thought was missing. But the malloc looks completely unnecessary and leaked because we use strdup there.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'll fix it in this branch.
There was a problem hiding this comment.
See 27ffa13 for my guess at a fix, I'm still waiting for it to rebuild so I can check for double free's.
There was a problem hiding this comment.
man, git blame, that beast 😆
There was a problem hiding this comment.
I had to remove the extra free. DDS appears to free this for us, because I get this on exit if I leave the free in:
[D0087|DELETE Sub]RTIOsapiHeap_freeBufferAligned:inconsistent free/alloc: block id 0X1 being freed with "RTIOsapiHeap_freeBufferAligned" and was allocated with "RTIOsapiHeap_unknownFunction"
talker(63898,0x7fff72c12000) malloc: *** error for object 0x7fc288c17cc0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
See: 44fb031
| RMW_SET_ERROR_MSG("failed to allocate memory for node namespace"); | ||
| goto fail; | ||
| } | ||
| memcpy(const_cast<char *>(node_handle->namespace_), namespace_, strlen(namespace_) + 1); |
There was a problem hiding this comment.
this is link to the previous comment, where I believe that memcpy is the right approach here and strdup isn't.
| @@ -555,6 +574,8 @@ destroy_node(const char * implementation_identifier, rmw_node_t * node) | |||
| node->data = nullptr; | |||
| rmw_free(const_cast<char *>(node->name)); | |||
| node->name = nullptr; | |||
| rmw_free(const_cast<char *>(node->namespace_)); | |||
There was a problem hiding this comment.
didn't look up the complete node struct here, but what's the difference between the node and the node_handle is this context?
There was a problem hiding this comment.
node_handle is just the variable name in the create_node function. In this function, destroy_node that same object is passed in as node rather than node_handle. At least that's how I read the code.
Connects to ros2/rmw#95