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.

Sign up for GitHub

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

Makes rcl_action_get_*_name() functions check for empty action names. #329

Merged
merged 2 commits into from
Nov 16, 2018
Merged

Makes rcl_action_get_*_name() functions check for empty action names. #329

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d77c9b6
[rcl action] Makes rcl_action_get_*_name() functions check for empty …
hidmic Nov 14, 2018
276aed1
[rcl action] Addresses peer review comments (#329)
hidmic Nov 14, 2018
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
20 changes: 10 additions & 10 deletions rcl_action/include/rcl_action/names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ extern "C"
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] action_name The name of the action whose goal service name is
* being returned.
* \param[in] action_name The non-empty name of the action whose goal
* service name is being returned.
* \param[in] allocator A valid allocator to be used.
* \param[out] goal_service_name Either an allocated string with the action
* goal service name, or `NULL` if the function failed to allocate memory
Expand Down Expand Up @@ -73,8 +73,8 @@ rcl_action_get_goal_service_name(
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] action_name The name of the action whose cancel service name is
* being returned.
* \param[in] action_name The non-empty name of the action whose cancel
* service name is being returned.
* \param[in] allocator A valid allocator to be used.
* \param[out] cancel_service_name Either an allocated string with the action
* cancel service name, or `NULL` if the function failed to allocate memory
Expand Down Expand Up @@ -105,8 +105,8 @@ rcl_action_get_cancel_service_name(
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] action_name The name of the action whose result service name is
* being returned.
* \param[in] action_name The non-empty name of the action whose result service
* name is being returned.
* \param[in] allocator A valid allocator to be used.
* \param[out] result_service_name Either an allocated string with the action
* result service name, or `NULL` if the function failed to allocate memory
Expand Down Expand Up @@ -137,8 +137,8 @@ rcl_action_get_result_service_name(
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] action_name The name of the action whose feedback topic name is
* being returned.
* \param[in] action_name The non-empty name of the action whose feedback
* topic name is being returned.
* \param[in] allocator A valid allocator to be used.
* \param[out] result_service_name Either an allocated string with the action
* feedback topic name, or `NULL` if the function failed to allocate memory
Expand Down Expand Up @@ -169,8 +169,8 @@ rcl_action_get_feedback_topic_name(
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] action_name The name of the action whose status topic name is
* being returned.
* \param[in] action_name The non-empty name of the action whose status topic
* name is being returned.
* \param[in] allocator A valid allocator to be used.
* \param[out] result_service_name Either an allocated string with the action
* status topic name, or `NULL` if the function failed to allocate memory
Expand Down
24 changes: 23 additions & 1 deletion rcl_action/src/rcl_action/names.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ extern "C"

#include "rcl_action/names.h"

#include <string.h>

#include "rcl/error_handling.h"
#include "rcutils/format_string.h"

Expand All @@ -30,6 +32,10 @@ rcl_action_get_goal_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
Copy link
Member

Choose a reason for hiding this comment

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

Suggest returning RCL_RET_ACTION_NAME_INVALID. Then it makes it possible for the action client/server to know if the provided name is empty (invalid) vs a null argument. Same for other functions and also update the docs to include the new return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, makes sense. Done in 276aed1.

}
RCL_CHECK_ARGUMENT_FOR_NULL(goal_service_name, RCL_RET_INVALID_ARGUMENT);
if (NULL != *goal_service_name) {
RCL_SET_ERROR_MSG("writing action goal service name may leak memory");
Expand All @@ -51,6 +57,11 @@ rcl_action_get_cancel_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate of line above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, fixed in 276aed1.

Copy link
Member

Choose a reason for hiding this comment

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

Redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, yeah, fixed in 276aed1.

if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(cancel_service_name, RCL_RET_INVALID_ARGUMENT);
if (NULL != *cancel_service_name) {
RCL_SET_ERROR_MSG("writing action cancel service name may leak memory");
Expand All @@ -72,12 +83,15 @@ rcl_action_get_result_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(result_service_name, RCL_RET_INVALID_ARGUMENT);
if (NULL != *result_service_name) {
RCL_SET_ERROR_MSG("writing action result service name may leak memory");
return RCL_RET_INVALID_ARGUMENT;
}

*result_service_name = rcutils_format_string(allocator, "%s/_action/get_result", action_name);
if (NULL == *result_service_name) {
RCL_SET_ERROR_MSG("failed to allocate memory for action result service name");
Expand All @@ -94,6 +108,10 @@ rcl_action_get_feedback_topic_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(feedback_topic_name, RCL_RET_INVALID_ARGUMENT);
if (NULL != *feedback_topic_name) {
RCL_SET_ERROR_MSG("writing action feedback topic name may leak memory");
Expand All @@ -115,6 +133,10 @@ rcl_action_get_status_topic_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(status_topic_name, RCL_RET_INVALID_ARGUMENT);
if (NULL != *status_topic_name) {
RCL_SET_ERROR_MSG("writing action status topic name may leak memory");
Expand Down
16 changes: 10 additions & 6 deletions rcl_action/test/rcl_action/test_names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,17 @@ class TestActionDerivedName

TEST_P(TestActionDerivedName, validate_action_derived_getter)
{
rcl_ret_t ret;
char dummy_char;
char * action_derived_name;
rcl_allocator_t default_allocator =
rcl_get_default_allocator();
rcl_allocator_t default_allocator = rcl_get_default_allocator();

char * action_derived_name = NULL;
const char * const null_action_name = NULL;
rcl_ret_t ret = test_subject.get_action_derived_name(
null_action_name, default_allocator,
&action_derived_name);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);

action_derived_name = NULL;
const char * const invalid_action_name = NULL;
const char * const invalid_action_name = "";
ret = test_subject.get_action_derived_name(
invalid_action_name, default_allocator,
&action_derived_name);
Expand All @@ -76,6 +79,7 @@ TEST_P(TestActionDerivedName, validate_action_derived_getter)
invalid_ptr_to_action_derived_name);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);

char dummy_char = '\0';
action_derived_name = &dummy_char;
ret = test_subject.get_action_derived_name(
test_subject.action_name, default_allocator,
Expand Down