implement avoid_ros_namespace_conventions QoS option #227
Conversation
rmw_connext_cpp/src/functions.cpp
Outdated
| bool avoid_ros_namespace_conventions, | ||
| char ** request_partition_str, | ||
| char ** response_partition_str, | ||
| char ** service_str) |
There was a problem hiding this comment.
the arguments here are not in the same order as __process_topic_name (which has the partition string last): is there a reason for this? If not, consistency would be better to prevent accidental misuse in the future (e.g. when refactoring)
| allocator, | ||
| "%s/%s", ros_service_response_prefix, name_tokens.data[0]); | ||
| if (!response_concat_str) { | ||
| allocator.deallocate(request_concat_str, allocator.state); |
There was a problem hiding this comment.
should this be removed? (it's not in the equivalent block for request_concat_str or concat_str)
There was a problem hiding this comment.
No, the request_concat_str points to allocated memory at this point. If the deallocate did not happen, then it would be leaked here. There is nothing that needs to be deallocated in either of the other two cases, request_concat_str or concat_str.
There was a problem hiding this comment.
yep, thanks. I read it as deallocating response_concat_str.
There was a problem hiding this comment.
lgtm with running CI
I would interested in to see what's happening with setting the ros_prefix to an empty string. I am almost certain that this will work. If so, that would slightly ease the functions by getting rid of the if-case and the boolean parameter to skip the prefix.
rmw_connext_cpp/src/functions.cpp
Outdated
| @@ -214,6 +213,57 @@ rmw_destroy_node(rmw_node_t * node) | |||
| return destroy_node(rti_connext_identifier, node); | |||
| } | |||
|
|
|||
| bool | |||
| __process_topic_name( | |||
There was a problem hiding this comment.
nitpick, but I think this file uses only one underscore for "non-public" methods.
rmw_connext_cpp/src/functions.cpp
Outdated
| // Connext will call deallocate on this, passing ownership to connext | ||
| *topic_str = DDS_String_dup(name_tokens.data[1]); | ||
| } else { | ||
| RMW_SET_ERROR_MSG("Illformated topic name") |
There was a problem hiding this comment.
That might be my incorrect english :)
rmw_connext_cpp/src/functions.cpp
Outdated
| char ** partition_str) | ||
| { | ||
| bool success = true; | ||
| rcutils_string_array_t name_tokens; |
There was a problem hiding this comment.
I should have default initialized that data structure as done in the service partitions.
wjwwood
added
enhancement
| if (!_process_topic_name( | ||
| topic_name, | ||
| qos_profile->avoid_ros_namespace_conventions, | ||
| &topic_str, |
There was a problem hiding this comment.
@wjwwood When is the allocated memory of this variable being freed? Same for partition_str next line.
There was a problem hiding this comment.
Connext takes ownership of it according to this comment: 69bc3e6#diff-becdf01440f1757df19f4775b534e066R251
I believe if you free it then connext later does and you get a double free.
There was a problem hiding this comment.
Currently it shows up as being leaked in valgrind with the demo_nodes_cpp talker / listener.
There was a problem hiding this comment.
You can try freeing it, but I'm pretty sure we were getting a double free before.
There was a problem hiding this comment.
Also, this is the wrong pull request to ask on I think because it was only moved into a function in this pr. The removed code already had this ownership comment in place.
There was a problem hiding this comment.
for reference, seems related to https://github.com/ros2/rcl/pull/161/files, maybe doing one will impact the other
There was a problem hiding this comment.
for reference, seems related to https://github.com/ros2/rcl/pull/161/files, maybe doing one will impact the other
Those are not related. topic_str is a local variable within rmw_connext_cpp.
This implements the
avoid_ros_namespace_conventionsQoS option. @Karsten1987 I also refactored this to share code between the pub-sub and again between service-client. It could be deduplicated more, but I think this is incrementally better. I also switched to usingrcutils_format_stringto address one of your TODO's. Please have a look at those changes carefully when reviewing (lots of copy-paste going on, could be a mistake).Connects to ros2/rmw#106