ros2 / rmw_cyclonedds

ROS 2 RMW layer for Eclipse Cyclone DDS
Apache License 2.0
108 stars 89 forks source link

Fix get_topic_name and handling long service names #386

Closed eboasson closed 2 years ago

eboasson commented 2 years ago

Determining whether a service is available relies on topic names, but the function to retrieve a topic name mishandled long names by accepting a truncated name when the 128-byte buffer passed in was too small instead of retrying with a larger buffer.

Cyclone DDS' underlying function always returns the actual length of the name and topic names can't change, and so there is no need to guess or loop.

Fixes #384

Signed-off-by: Erik Boasson eb@ilities.com

eboasson commented 2 years ago

CI:

eboasson commented 2 years ago

The latest commit should take care of it @pauldinh — I checked with your test with a range of lengths.

eboasson commented 2 years ago

@clalancette given that I had to fix my fix to fix essentially the same problem that needed fixing in the first place (😖) and the code base is frozen, I feel it is not quite right to merge it based on your original approval. Are you ok with me pressing merge on this one?

clalancette commented 2 years ago

@clalancette given that I had to fix my fix to fix essentially the same problem that needed fixing in the first place (confounded) and the code base is frozen, I feel it is not quite right to merge it based on your original approval. Are you ok with me pressing merge on this one?

This is a fix, so I'm fine with this going in. So I've re-approved it. That said, we should re-run CI on this before merging. Once that comes back successfully, it's fine to merge it.

eboasson commented 2 years ago

CI:

eboasson commented 2 years ago

Windows CI failures are unrelated