add rmw_validate_node_name() by wjwwood · Pull Request #93 · ros2/rmw

wjwwood · 2017-03-29T00:19:52Z

This is a precursor to the larger set of namespace pull requests. This one was small and reviewable as-is, so I'm submitting it separately.

The documentation for this new function also takes the opportunity to layout some rules for node names, which we've sort of had in mind informally until now.

This new function can be used by rcl* to check if a node name follows the rules for node names, or by rmw_* packages to assert that the node name it is given is valid (should always be the case, but it might be worth checking incase something other than rcl uses rmw directly).

Just like rmw_validate_topic_name(), this function will return a cause if the name is invalid, as well as an index in the string that is the culprit.

I copy-paste-modify'ed this from the other validate function, so please be on the lookout for typos 😄.

wjwwood · 2017-03-29T00:21:51Z

rmw/include/rmw/validate_node_name.h

+ *
+ * - must not be an empty string
+ * - must only contain alphanumeric characters and underscores (a-z|A-Z|0-9|_)
+ * - must not start with an number


@dirk-thomas I modeled these after our discussion a few days ago. Does this look ok?

Maybe also allow dashes?

Is there a reason why not to start with a number?

I avoided both of those for situations where the node name might be used as part of, or the start of, a variable name in some code generation situation. But I don't feel strongly about it.

wjwwood · 2017-03-29T00:27:30Z

rmw/include/rmw/validate_node_name.h

+#define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3
+#define RMW_NODE_NAME_INVALID_TOO_LONG 4
+
+#define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */


Also, I put an arbitrary length check of 255 here, but I excluded it from the rules in the documentation.

My thought process here is that the length limit check is a sanity check. Since there is no technical limit (that I am aware of) for node name at the moment, this is more for "did you really mean to do that?". My expectation is that "node name too long" could be just ignored by the rclcpp/rclpy crowd. I could also just remove it all together. Not sure.

Especially for use cases with preallocated memory the upper bound is a good idea imo.

I'll leave it then, thanks.

+1, we can change the upper bound in the future if needed

dirk-thomas · 2017-03-29T15:40:59Z

rmw/include/rmw/validate_node_name.h

+#define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3
+#define RMW_NODE_NAME_INVALID_TOO_LONG 4
+
+#define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */


Especially for use cases with preallocated memory the upper bound is a good idea imo.

dirk-thomas · 2017-03-29T15:42:09Z

rmw/include/rmw/validate_node_name.h

+ *
+ * - must not be an empty string
+ * - must only contain alphanumeric characters and underscores (a-z|A-Z|0-9|_)
+ * - must not start with an number


Maybe also allow dashes?

Is there a reason why not to start with a number?

dirk-thomas · 2017-03-29T15:43:57Z

rmw/include/rmw/validate_node_name.h

+ * `RMW_RET_INVALID_ARGUMENT` will be returned.
+ * The node_name should be a valid, null-terminated C string.
+ * The validation_result int pointer should point to valid memory so a result
+ * can be stored in it as an output variable.


I wouldn't enforce passing this in. If the caller doesn't care about the reason for a failure it should be fine to pass 0 here. (When this is changed rmw_topic_validation_result_string should probably be updated too.)

Same for the index.

It is required because otherwise you don't know if it is valid or not. The return code is only for exceptions, like invalid arguments.

I could make the index optional.

I would suggest the function doesn't return RMW_RET_OK when the passed node name is invalid (e.g. empty). Maybe RMW_RET_ERROR. If the caller wants to know the reason he can optionally pass the validation_result pointer.

@Karsten1987 and I already discussed this in #93, but I explicitly did it this way to not mix the validation codes for the node name or topic name with the new return codes. I do not want to change how it is currently organized for that reason.

Sorry, I meant #92.

dirk-thomas · 2017-03-29T15:48:11Z

rmw/src/validate_node_name.c

+{
+  switch (validation_result) {
+    case RMW_NODE_NAME_VALID:
+      return "node name is valid";


Do we need a result string for the successful case?

I dunno, I suppose I could return null for valid as well.

+1 for returning null on success

Not my favorite solution, but I'm ok with it. See: c8d25e6

wjwwood · 2017-03-31T18:36:00Z

CI:

Linux:
macOS:
Windows:
- (warning will eventually be addressed by use 64-bit integer for parameter tests system_tests#197)

wjwwood · 2017-03-31T22:46:40Z

If anyone has time to review this I'd appreciate it. This is preventing me from moving forward on the next set of prs (#95 and beyond).

wjwwood · 2017-04-04T00:01:37Z

One more shameless bump for a review.

mikaelarguedas

lgtm

mikaelarguedas · 2017-04-04T02:01:23Z

rmw/include/rmw/validate_node_name.h

+#define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3
+#define RMW_NODE_NAME_INVALID_TOO_LONG 4
+
+#define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */


+1, we can change the upper bound in the future if needed

mikaelarguedas · 2017-04-04T02:02:10Z

rmw/src/validate_node_name.c

+{
+  switch (validation_result) {
+    case RMW_NODE_NAME_VALID:
+      return "node name is valid";


+1 for returning null on success

wjwwood · 2017-04-04T03:40:20Z

CI:

Linux:
macOS:
Windows

wjwwood added 2 commits March 28, 2017 17:13

rename rmw_topic_validation_result_string()

2ac70ac

fix length checking test

537fae8

wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Mar 29, 2017

wjwwood self-assigned this Mar 29, 2017

wjwwood force-pushed the validate_node_name_func branch from 4a815b7 to 468b21b Compare March 29, 2017 00:21

wjwwood commented Mar 29, 2017

View reviewed changes

wjwwood force-pushed the validate_node_name_func branch from 468b21b to 0675ff3 Compare March 29, 2017 00:24

add rmw_validate_node_name()

3982076

wjwwood force-pushed the validate_node_name_func branch from 0675ff3 to 3982076 Compare March 29, 2017 00:25

wjwwood commented Mar 29, 2017

View reviewed changes

This was referenced Mar 29, 2017

Support node namespaces #95

Merged

add concept of node namespace to rcl API ros2/rcl#112

Merged

dirk-thomas reviewed Mar 29, 2017

View reviewed changes

mikaelarguedas approved these changes Apr 4, 2017

View reviewed changes

return NULL when the name is valid (topic and node)

c8d25e6

wjwwood merged commit 6cd3a04 into master Apr 4, 2017

wjwwood deleted the validate_node_name_func branch April 4, 2017 03:40

wjwwood removed the in review Waiting for review (Kanban column) label Apr 4, 2017

Conversation

wjwwood commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood Mar 29, 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!

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!

wjwwood commented Mar 31, 2017

Uh oh!

wjwwood commented Mar 31, 2017

Uh oh!

wjwwood commented Apr 4, 2017

Uh oh!

mikaelarguedas left a comment

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 Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wjwwood Mar 29, 2017 •

edited

Loading