add a function to validate topic names by wjwwood · Pull Request #92 · ros2/rmw

wjwwood · 2017-03-16T07:45:52Z

This is the first step in implementing namespaces above the rmw layer. I ended up placing this function in rmw (or at least that's what I'm proposing here) because I thought it might be a useful check (if only in debug situations) that could be used in the rmw implementations as well as in rcl where I will use it to support other namespace functionality.

I'm interested in feedback on placing it here rather than in rcl. Also, I've put it in a rmw/impl namespace, because I feel that it should stand apart from the rmw interface that middleware implementations need to fulfill. In both cases, it is easily moved around or re-namespaced, so please let me know if you think it would be better elsewhere.

I feel that this function's purpose and rules (set out from the design doc) are self contained enough and well defined enough to merge ahead of more namespace work, so I'm going to go ahead and put it in review.

wjwwood · 2017-03-16T07:49:21Z

rmw/include/rmw/impl/validate_topic_name.h

+#define RMW_IMPL_INVALID_TOPIC_NAME_TOKEN_STARTS_WITH_NUMBER 6
+#define RMW_IMPL_INVALID_TOPIC_TOO_LONG 7
+
+#define RMW_IMPL_MAX_TOPIC_NAME_LEGNTH 255 /* impl constraint */ - 8 /* reserved for prefixes */


@Karsten1987 I ended going conservative with 255 - 8 (for the prefix) based on our conversation, when in reality we could go for (255 * 2) - 8 or something like that. I'd like to hear from others what they think about this more conservative limit.

I designed the function so that it reports the length as an issue only after all other checks pass. That way we could make the length limit a warning rather than a hard error.

The design doc could, I think, be clarified on this point after we settle on something.

Given that I was not active in the namespace design review I have a few (maybe obvious) questions in order to understand this:

In the case of using 255*2-MAX_PREFIX_LENGTH as the max length, we assume that the the namespace part is less than 255-MAX_PREFIX_LENGTH and that the base name is less than 255 right ? Will this be enforced by the rmw_implementation itself or by rmw?

if the former we should specify that everything before the last forward slash is the namespace and everything after it is the base name, otherwise according to the implementation the topics won't match and we will lose interoperability

if the latter (what I think makes more sense): We should check that both base names and namespaces fulfill the requirement (and not the sum of their length) in the shared code and pass them individually to the rmw_implementations.

Pros:

That'll cover non mapable scenario /ns/my_topic_name_that_is_way_too_long_for_some_obscure_reason_and_hypothetically_longer_than_255_characters_for_the_sake_of_this_example

DRY

Interroperability

Cons:

prevents rmw specific optimization that could provide more flexibility by splitting the ROS name to fit in 255 for Partition + 255 for topic name even if the namespace or base name doesn't fulfill respectively the 255-character long requirement

Conclusion: I think we should stick with the max length implemented in this PR for now. It leaves us some leg room, will likely be easier if we ever want to plus another protocol under rmw and doesn't restrict users that much. We can always revisit it if someone raises the issue of the allowed length being too restrictive for their use case

The way I see this is that everything above RMW should not necessarily deal with specific namespace constraints and for sure not with partitions as for whatever reason we may change DDS with something else.

@mikaelarguedas That's a good point that I had thought about but failed to put in my comment, that allowing more than 255-ish would require the more complicated constraint of 255-ish for the namespace and 255-ish for the basename which is both harder to check and harder to communicate to users. @Karsten1987 I take the meaning of your comment to agree with that idea.

So, I'll leave it as-is and propose a clarification to the design doc with this rationale.

Separately we can still make topic names > 247 characters a warning at the rcl level and let the middleware implementation throw up the error if any real boundary is reached lengthwise. What do you guys think of that?

👍 sounds good to me

Karsten1987 · 2017-03-16T15:09:48Z

rmw/src/impl/validate_topic_name.c

+  if (topic_name_length == 0) {
+    *validation_result = RMW_IMPL_INVALID_TOPIC_IS_EMPTY_STRING;
+    *invalid_index = 0;
+    return RMW_RET_OK;


Is this not a potential conflict here? The validation results in an invalid topic, but the return code is ok? Why not returning the validation result directly?

Even if the topic is invalid, the function succeeds. Please read the documentation for the function.

+1 for clarifying it inside the documentation.

@Karsten1987 I think it is pretty clear in the documentation, can you suggest something different if you think it isn't clear enough?

Karsten1987 · 2017-03-16T15:12:12Z

rmw/src/impl/validate_topic_name.c

+    return RMW_RET_OK;
+  }
+  // check for unallowed characters
+  for (size_t i = 0; i < topic_name_length; ++i) {


Why not converting to ascii and check if the value is a valid range?

The string is ascii? That's what isalnum does I think (compare to a range). Also there is no single range that covers isalnum and underscore and forward slash. If you mean something else please clarify. Are you talking about the todo I left?

yes, my comment was with respect to the todo, if isalnum is giving us a hard time with the local, we could manually specify the allowed ascii ranges ([a-z][A-Z][0-9]).

Hmm, yeah I was trying to avoid custom code, but I might do just as you said to avoid locale issues.

Karsten1987 · 2017-03-16T15:12:47Z

rmw/src/impl/validate_topic_name.c

+      continue;
+    } else {
+      // if it is none of these, then it is an unallowed character in a FQN topic name
+      *validation_result = RMW_IMPL_INVALID_TOPIC_CONTAINS_UNALLOWED_CHARACTERS;


same comment as above

Same comment as above.

Karsten1987 · 2017-03-16T15:14:34Z

rmw/src/impl/validate_topic_name.c

+      return RMW_RET_OK;
+    }
+  }
+  // check for double '/' and tokens that start with a number


Is there any problem in giving a topic such as /foo//bar//baz/ ?

yes... what do mean by problem? It has a double slash twice and a trailing slash. It has several problems which make the topic invalid. This function just stops on the first problem it finds. I'm not sure what you're asking.

I was thinking about the filesystem analogy, where you can specify file paths with any number of forward slashes and these are being ignored or merged into one. I wonder if we should do the same.

Collapsing the double slash is an implementation specific assist. https://en.wikipedia.org/wiki/Path_(computing)

I'd suggest we stay strict and if really necessary we can open it up later. It's powerful but also dangerous, if you're using a substitution variable and it's key is typoed you could end up promoting everything by accident.

I agree, we should not allow repeated slashes. The design doc is pretty clear about this:

must not be empty [in reference to name tokens], e.g. the name //bar is not allowed

rationale: it removes the chance for accidental // from concatenation and thereore the need to collapse // to /

-- http://design.ros2.org/articles/topic_and_service_names.html#name-tokens

(btw fixed the spelling in ros2/design@efc9bf9)

If we decide later that this is not flexible enough, then we can change it.

mikaelarguedas · 2017-03-16T15:32:31Z

rmw/src/impl/validate_topic_name.c

+    }
+  }
+  // check if the topic name is too long last, since it might be a soft invalidation
+  if (topic_name_length > RMW_IMPL_MAX_TOPIC_NAME_LEGNTH) {


Typo: Commenting here rather than above to dissociate from discussion: RMW_IMPL_MAX_TOPIC_NAME_LEGNTH => RMW_IMPL_MAX_TOPIC_NAME_LENGTH

Thanks, updated in d746662

wjwwood · 2017-03-16T22:18:40Z

@dirk-thomas I had to add 2a66f7a in order to address this Linux-only build failure:

../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::TestPartResultReporterInterface*>::~ThreadLocal()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEED2Ev[_ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEED5Ev]+0x25): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEED2Ev[_ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEED5Ev]+0x3a): undefined reference to `pthread_key_delete'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<std::vector<testing::internal::TraceInfo, std::allocator<testing::internal::TraceInfo> > >::~ThreadLocal()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEED2Ev[_ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEED5Ev]+0x25): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEED2Ev[_ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEED5Ev]+0x3a): undefined reference to `pthread_key_delete'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<std::vector<testing::internal::TraceInfo, std::allocator<testing::internal::TraceInfo> > >::GetOrCreateValue() const':
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE16GetOrCreateValueEv]+0x27): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE16GetOrCreateValueEv]+0x8b): undefined reference to `pthread_setspecific'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::TestPartResultReporterInterface*>::CreateKey()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE9CreateKeyEv[_ZN7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE9CreateKeyEv]+0x25): undefined reference to `pthread_key_create'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<std::vector<testing::internal::TraceInfo, std::allocator<testing::internal::TraceInfo> > >::CreateKey()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE9CreateKeyEv[_ZN7testing8internal11ThreadLocalISt6vectorINS0_9TraceInfoESaIS3_EEE9CreateKeyEv]+0x25): undefined reference to `pthread_key_create'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::Sequence*>::CreateKey()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_8SequenceEE9CreateKeyEv[_ZN7testing8internal11ThreadLocalIPNS_8SequenceEE9CreateKeyEv]+0x25): undefined reference to `pthread_key_create'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::TestPartResultReporterInterface*>::GetOrCreateValue() const':
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE16GetOrCreateValueEv]+0x25): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalIPNS_31TestPartResultReporterInterfaceEE16GetOrCreateValueEv]+0x89): undefined reference to `pthread_setspecific'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::Sequence*>::GetOrCreateValue() const':
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalIPNS_8SequenceEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalIPNS_8SequenceEE16GetOrCreateValueEv]+0x25): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZNK7testing8internal11ThreadLocalIPNS_8SequenceEE16GetOrCreateValueEv[_ZNK7testing8internal11ThreadLocalIPNS_8SequenceEE16GetOrCreateValueEv]+0x89): undefined reference to `pthread_setspecific'
../gmock/libgmock.a(gmock-gtest-all.cc.o): In function `testing::internal::ThreadLocal<testing::Sequence*>::~ThreadLocal()':
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_8SequenceEED2Ev[_ZN7testing8internal11ThreadLocalIPNS_8SequenceEED5Ev]+0x25): undefined reference to `pthread_getspecific'
gmock-gtest-all.cc:(.text._ZN7testing8internal11ThreadLocalIPNS_8SequenceEED2Ev[_ZN7testing8internal11ThreadLocalIPNS_8SequenceEED5Ev]+0x3a): undefined reference to `pthread_key_delete'
collect2: error: ld returned 1 exit status

You can see that a similar thing has been done for the other test here in rmw. Should the ament_add_gmock_test macro be doing this linking against pthread? I do not believe these tests are introducing the need for pthread based on the linked error lines above. I think it's more likely that our other tests get pthread linked in for some other reason, hiding this problem in most cases.

wjwwood · 2017-03-16T23:20:18Z

CI:

Linux:
macOS:
Windows: (I had to add --skip-packages fastrtps to my test args...)

wjwwood · 2017-03-17T01:03:24Z

Ok, CI looks good. Anyone else have comments before merging?

@Karsten1987 I tried to reply to each of your questions/comments. Can you re-review real quick.

wjwwood · 2017-03-21T18:21:25Z

I'm going to address #92 (comment), in the meantime does anyone have any comments on the placement of this function? Should it be in the impl part of rmw? Should it be in rmw rather than somewhere else like c_utilities or just rcl?

mikaelarguedas · 2017-03-21T18:27:11Z

I don't think we should put this in c_utilities if we want to keep c_utilities ros agnostic. not sure if it needs to be as low as rmw or can be in rcl though. It may actually be better in rcl given that different rmw_implementations and transports could decide to deal with the full topic name in different ways. I don't have a strong opinion about it though.

dirk-thomas · 2017-03-21T18:30:43Z

Imo c_utilities should only contains generic C stuff which is not specific to any ROS concepts.

If there is no strong reason why these functions are being used in rmw I would favor placing them in rcl.

wjwwood · 2017-03-21T18:32:04Z

If there is no strong reason why these functions are being used in rmw I would favor placing them in rcl.

The only reason would be so that rmw implementations could "double check" the incoming topics are valid FQN. This should be asserted by rcl before passing them down, but if someone were to use rmw directly it might not always be the case.

dirk-thomas · 2017-03-21T18:35:47Z

If there is no strong reason why these functions are being used in rmw I would favor placing them in rcl.

The only reason would be so that rmw implementations could "double check" the incoming topics are valid FQN. This should be asserted by rcl before passing them down, but if someone were to use rmw directly it might not always be the case.

If it will be used to validate parameters passed through the rmw interface https://github.com/ros2/rmw/blob/master/rmw/include/rmw/sanity_checks.h sounds like a reasonable place.

wjwwood · 2017-03-21T19:02:00Z

Ok, so not in the impl? I'd rather not stuff it in that catch-all style header. I'd like to move slowly away from the the rmw header layout which puts everything in groups. So if I move it out of the impl, then I'd just put it in its own header as it is now. Does that sound ok?

dirk-thomas · 2017-03-21T19:09:11Z

Ok, so not in the impl? I'd rather not stuff it in that catch-all style header. I'd like to move slowly away from the the rmw header layout which puts everything in groups. So if I move it out of the impl, then I'd just put it in its own header as it is now. Does that sound ok?

Sounds good to me.

wjwwood · 2017-03-21T19:14:43Z

I implemented a custom isalnum in aaa1323, I'll reorganize the code as discussed after lunch.

wjwwood · 2017-03-21T20:10:39Z

Ok, I reorganized the files in a4963d7 and fixed them in 3ddf742. I also refactored the macros to all start with a common prefix, RMW_TOPIC_ (e.g. RMW_TOPIC_VALID, RMW_TOPIC_INVALID_..., RMW_TOPIC_MAX_NAME_LENGTH), in f3fbb02.

wjwwood · 2017-03-22T04:03:02Z

CI:

Linux:
- Warning is unrelated to this pr, see: Rclcpp time rclcpp#311 (comment)
macOS:
Windows:

Karsten1987 · 2017-04-19T17:21:36Z

rmw/src/validate_topic_name.c

+      }
+    }
+  }
+  // check if the topic name is too long last, since it might be a soft invalidation


maybe a nit, but I believe the check for maximum length should be further up in this function for returning quickly before iterating over all characters.

Please read the documentation or this exact comment for why it is last.

add a function to valid topic names

65d2500

wjwwood added the in review Waiting for review (Kanban column) label Mar 16, 2017

wjwwood commented Mar 16, 2017

View reviewed changes

Karsten1987 reviewed Mar 16, 2017

View reviewed changes

mikaelarguedas reviewed Mar 16, 2017

View reviewed changes

wjwwood self-assigned this Mar 16, 2017

wjwwood added 2 commits March 16, 2017 14:54

fix typo in macro name

d746662

link gtest tests against pthread on Linux

2a66f7a

wjwwood changed the title ~~add a function to valid topic names~~ add a function to validate topic names Mar 21, 2017

use custom isalnum that isn't affected by locale

aaa1323

wjwwood added 3 commits March 21, 2017 12:52

relocate files out of impl folders

a4963d7

fix includes and remove _impl from names

3ddf742

refactor macros to all start with RMW_TOPIC_

f3fbb02

wjwwood added 2 commits March 21, 2017 18:31

make linkage internal for isalnum_no_locale

14ce9cb

cpplint fixup

8da56ea

wjwwood merged commit 9dc1f65 into master Mar 22, 2017

wjwwood deleted the validate_topic_name branch March 22, 2017 04:38

wjwwood removed the in review Waiting for review (Kanban column) label Mar 22, 2017

wjwwood mentioned this pull request Mar 30, 2017

add rmw_validate_node_name() #93

Merged

Karsten1987 reviewed Apr 19, 2017

View reviewed changes

Conversation

wjwwood commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Mar 16, 2017

Uh oh!

wjwwood commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Mar 17, 2017

Uh oh!

wjwwood commented Mar 21, 2017

Uh oh!

mikaelarguedas commented Mar 21, 2017

Uh oh!

dirk-thomas commented Mar 21, 2017

Uh oh!

wjwwood commented Mar 21, 2017

Uh oh!

dirk-thomas commented Mar 21, 2017

Uh oh!

wjwwood commented Mar 21, 2017

Uh oh!

dirk-thomas commented Mar 21, 2017

Uh oh!

wjwwood commented Mar 21, 2017

Uh oh!

wjwwood commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Mar 16, 2017 •

edited

Loading

Karsten1987 Mar 16, 2017 •

edited

Loading

wjwwood commented Mar 16, 2017 •

edited

Loading

wjwwood commented Mar 21, 2017 •

edited

Loading