Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rmw_validate_node_name() #93

Merged
merged 4 commits into from
Apr 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rmw/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ include_directories(include)
add_library(${PROJECT_NAME} SHARED
"src/allocators.c"
"src/error_handling.c"
"src/validate_node_name.c"
"src/validate_topic_name.c"
"src/sanity_checks.c")
configure_rmw_library(${PROJECT_NAME})
Expand Down
95 changes: 95 additions & 0 deletions rmw/include/rmw/validate_node_name.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright 2017 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RMW__VALIDATE_NODE_NAME_H_
#define RMW__VALIDATE_NODE_NAME_H_

#if __cplusplus
extern "C"
{
#endif

#include "rmw/macros.h"
#include "rmw/types.h"

#define RMW_NODE_NAME_VALID 0
#define RMW_NODE_NAME_INVALID_IS_EMPTY_STRING 1
#define RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS 2
#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 */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it then, thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Determine if a node name is valid.
/**
* Node names must follow these rules:
*
* - 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also allow dashes?

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

Copy link
Member Author

@wjwwood wjwwood Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* If either the node name C string or validation_result pointer are null, then
* `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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant #92.

* The invalid_index size_t pointer should point to valid memory so in the
* event of a validation error, the location in the input string can be stored
* therein.
*
* The invalid_index will not be assigned a value if the node name is valid.
*
* The int which validation_result points to will have a one of a few possible
* results values (defined with macros) stored into it:
*
* - RMW_NODE_NAME_VALID
* - RMW_NODE_NAME_INVALID_IS_EMPTY_STRING
* - RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS
* - RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER
* - RMW_NODE_NAME_INVALID_TOO_LONG
*
* The result value can be converted to a description with the
* rmw_node_name_validation_result_string() function.
*
* The `RMW_NODE_NAME_INVALID_TOO_LONG` is guaranteed to be checked last, such
* that if you get that result, then you can assume all other checks succeeded.
* This is done so that the length limit can be treated as a warning rather
* than an error if desired.
*
* \param[in] node_name node name to be validated
* \param[out] validation_result int in which the result of the check is stored
* \param[out] invalid_index size_t index of the input string where an error occurred
* \returns `RMW_RET_OK` on successfully running the check, or
* \returns `RMW_RET_INVALID_ARGUMENT` on invalid parameters, or
* \returns `RMW_RET_ERROR` when an unspecified error occurs.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_ret_t
rmw_validate_node_name(
const char * node_name,
int * validation_result,
size_t * invalid_index);

/// Return a validation result description, or NULL if unknown or RMW_NODE_NAME_VALID.
RMW_PUBLIC
RMW_WARN_UNUSED
const char *
rmw_node_name_validation_result_string(int validation_result);

#if __cplusplus
}
#endif

#endif // RMW__VALIDATE_NODE_NAME_H_
6 changes: 3 additions & 3 deletions rmw/include/rmw/validate_topic_name.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ extern "C"
* - RMW_TOPIC_INVALID_TOO_LONG
*
* The result value can be converted to a description with the
* rmw_validation_result_string() function.
* rmw_topic_validation_result_string() function.
*
* The `RMW_TOPIC_INVALID_TOO_LONG` is guaranteed to be checked last, such
* that if you get that result, then you can assume all other checks succeeded.
Expand All @@ -88,11 +88,11 @@ rmw_validate_topic_name(
int * validation_result,
size_t * invalid_index);

/// Return a string to describe the validation result, or NULL if unknown.
/// Return a validation result description, or NULL if unknown or RMW_TOPIC_VALID.
RMW_PUBLIC
RMW_WARN_UNUSED
const char *
rmw_validation_result_string(int validation_result);
rmw_topic_validation_result_string(int validation_result);

#if __cplusplus
}
Expand Down
93 changes: 93 additions & 0 deletions rmw/src/validate_node_name.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright 2017 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <rmw/validate_node_name.h>

#include <ctype.h>
#include <string.h>

#include "./isalnum_no_locale.h"

rmw_ret_t
rmw_validate_node_name(
const char * node_name,
int * validation_result,
size_t * invalid_index)
{
if (!node_name) {
return RMW_RET_INVALID_ARGUMENT;
}
if (!validation_result) {
return RMW_RET_INVALID_ARGUMENT;
}
if (!invalid_index) {
return RMW_RET_INVALID_ARGUMENT;
}
size_t node_name_length = strlen(node_name);
if (node_name_length == 0) {
*validation_result = RMW_NODE_NAME_INVALID_IS_EMPTY_STRING;
*invalid_index = 0;
return RMW_RET_OK;
}
// check for unallowed characters
for (size_t i = 0; i < node_name_length; ++i) {
if (isalnum_no_locale(node_name[i])) {
// if it is an alpha numeric character, i.e. [0-9|A-Z|a-z], continue
continue;
} else if (node_name[i] == '_') {
// if it is an underscore, continue
continue;
} else {
// if it is none of these, then it is an unallowed character in a node name
*validation_result = RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS;
*invalid_index = i;
return RMW_RET_OK;
}
}
if (isdigit(node_name[0]) != 0) {
// this is the case where the name starts with a number, i.e. [0-9]
*validation_result = RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER;
*invalid_index = 0;
return RMW_RET_OK;
}
// check if the node name is too long last, since it might be a soft invalidation
if (node_name_length > RMW_NODE_NAME_MAX_NAME_LENGTH) {
*validation_result = RMW_NODE_NAME_INVALID_TOO_LONG;
*invalid_index = RMW_NODE_NAME_MAX_NAME_LENGTH - 1;
return RMW_RET_OK;
}
// everything was ok, set result to valid node name, avoid setting invalid_index, and return
*validation_result = RMW_NODE_NAME_VALID;
return RMW_RET_OK;
}

const char *
rmw_node_name_validation_result_string(int validation_result)
{
switch (validation_result) {
case RMW_NODE_NAME_VALID:
return NULL;
case RMW_NODE_NAME_INVALID_IS_EMPTY_STRING:
return "node name must not be empty";
case RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS:
return "node name must not contain characters other than alphanumerics or '_'";
case RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER:
return "node name must not start with a number";
case RMW_NODE_NAME_INVALID_TOO_LONG:
return
"node name length should not exceed '" RMW_STRINGIFY(RMW_NODE_NAME_MAX_NAME_LENGTH) "'";
default:
return NULL;
}
}
4 changes: 2 additions & 2 deletions rmw/src/validate_topic_name.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ rmw_validate_topic_name(
}

const char *
rmw_validation_result_string(int validation_result)
rmw_topic_validation_result_string(int validation_result)
{
switch (validation_result) {
case RMW_TOPIC_VALID:
return "topic name is valid";
return NULL;
case RMW_TOPIC_INVALID_IS_EMPTY_STRING:
return "topic name must not be empty";
case RMW_TOPIC_INVALID_NOT_ABSOLUTE:
Expand Down
15 changes: 15 additions & 0 deletions rmw/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,18 @@ if(TARGET test_validate_topic_name)
set_target_properties(test_validate_topic_name PROPERTIES COMPILE_FLAGS "-std=c++14")
endif()
endif()

ament_add_gmock(test_validate_node_name
test_validate_node_name.cpp
# Append the directory of librmw so it is found at test time.
APPEND_LIBRARY_DIRS "$<TARGET_FILE_DIR:${PROJECT_NAME}>"
)
if(TARGET test_validate_node_name)
target_link_libraries(test_validate_node_name ${PROJECT_NAME})
if(UNIX AND NOT APPLE)
target_link_libraries(test_validate_node_name pthread)
endif()
if(NOT WIN32)
set_target_properties(test_validate_node_name PROPERTIES COMPILE_FLAGS "-std=c++14")
endif()
endif()
130 changes: 130 additions & 0 deletions rmw/test/test_validate_node_name.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// Copyright 2017 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <string>

#include "gmock/gmock.h"

#include "rmw/validate_node_name.h"

TEST(test_validate_node_name, invalid_parameters) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret = rmw_validate_node_name(nullptr, &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_INVALID_ARGUMENT, ret);
ret = rmw_validate_node_name("test", nullptr, &invalid_index);
ASSERT_EQ(RMW_RET_INVALID_ARGUMENT, ret);
ret = rmw_validate_node_name("test", &validation_result, nullptr);
ASSERT_EQ(RMW_RET_INVALID_ARGUMENT, ret);
}

TEST(test_validate_node_name, valid_node_name) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret;

ret = rmw_validate_node_name("nodename", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_VALID, validation_result);

validation_result = -1;
ret = rmw_validate_node_name("node_name", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_VALID, validation_result);

ASSERT_EQ((char *)NULL, rmw_node_name_validation_result_string(validation_result));
}

TEST(test_validate_node_name, empty_node_name) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret;

ret = rmw_validate_node_name("", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_IS_EMPTY_STRING, validation_result);
ASSERT_EQ(0ul, invalid_index);

ASSERT_NE((char *)NULL, rmw_node_name_validation_result_string(validation_result));
}

TEST(test_validate_node_name, unallowed_characters) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret;

ret = rmw_validate_node_name("node/name", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS, validation_result);
ASSERT_EQ(4ul, invalid_index);

ret = rmw_validate_node_name("node_{name}", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS, validation_result);
ASSERT_EQ(5ul, invalid_index);

ret = rmw_validate_node_name("~node_name", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS, validation_result);
ASSERT_EQ(0ul, invalid_index);

ret = rmw_validate_node_name("with spaces", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS, validation_result);
ASSERT_EQ(4ul, invalid_index);

ret = rmw_validate_node_name("with.periods", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS, validation_result);
ASSERT_EQ(4ul, invalid_index);

ASSERT_NE((char *)NULL, rmw_node_name_validation_result_string(validation_result));
}

TEST(test_validate_node_name, starts_with_number) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret;

ret = rmw_validate_node_name("42node", &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER, validation_result);
ASSERT_EQ(0ul, invalid_index);

ASSERT_NE((char *)NULL, rmw_node_name_validation_result_string(validation_result));
}

TEST(test_validate_node_name, node_name_too_long) {
int validation_result;
size_t invalid_index;
rmw_ret_t ret;

// Ensure the length is not the first error
std::string invalid_and_long_node_name(RMW_NODE_NAME_MAX_NAME_LENGTH + 1, '0');
ret = rmw_validate_node_name(
invalid_and_long_node_name.c_str(), &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER, validation_result);
ASSERT_EQ(0ul, invalid_index);

// Ensure length check works when there are no other issues
std::string valid_but_long_node_name(RMW_NODE_NAME_MAX_NAME_LENGTH + 1, 'a');
ret = rmw_validate_node_name(
valid_but_long_node_name.c_str(), &validation_result, &invalid_index);
ASSERT_EQ(RMW_RET_OK, ret);
EXPECT_EQ(RMW_NODE_NAME_INVALID_TOO_LONG, validation_result);
EXPECT_EQ(RMW_NODE_NAME_MAX_NAME_LENGTH - 1, invalid_index);

ASSERT_NE((char *)NULL, rmw_node_name_validation_result_string(validation_result));
}
Loading