diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 6ef4a5d..cbed688 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -656,13 +656,17 @@ rcl_action_expire_goals( } } } - ret_final = _recalculate_expire_timer( + rcl_ret_t expire_timer_ret = _recalculate_expire_timer( &action_server->impl->expire_timer, action_server->impl->options.result_timeout.nanoseconds, action_server->impl->goal_handles, action_server->impl->num_goal_handles, action_server->impl->clock); + if (RCL_RET_OK != expire_timer_ret) { + ret_final = expire_timer_ret; + } + // If argument is not null, then set it if (NULL != num_expired) { (*num_expired) = num_goals_expired; @@ -795,8 +799,9 @@ rcl_action_process_cancel_request( if (RCL_RET_OK != ret) { if (RCL_RET_BAD_ALLOC == ret) { ret_final = RCL_RET_BAD_ALLOC; // error already set + } else { + ret_final = RCL_RET_ERROR; // error already set } - ret_final = RCL_RET_ERROR; // error already set goto cleanup; } diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp index 4a555c1..6512c08 100644 --- a/rcl_action/test/rcl_action/test_action_server.cpp +++ b/rcl_action/test/rcl_action/test_action_server.cpp @@ -727,7 +727,11 @@ TEST_F(TestActionServer, test_action_server_get_goal_status_array) EXPECT_NE(status_array.msg.status_list.data, nullptr); EXPECT_EQ(status_array.msg.status_list.size, 1u); rcl_action_goal_info_t * goal_info_out = &status_array.msg.status_list.data[0].goal_info; - EXPECT_TRUE(uuidcmp(goal_info_out->goal_id.uuid, goal_info_in.goal_id.uuid)); + + // Passing this array by pointer can confuse scan-build into thinking the pointer is + // not checked for null. Passing by reference works fine + const auto & goal_info_out_uuid = goal_info_out->goal_id.uuid; + EXPECT_TRUE(uuidcmp(goal_info_out_uuid, goal_info_in.goal_id.uuid)); ret = rcl_action_goal_status_array_fini(&status_array); ASSERT_EQ(ret, RCL_RET_OK); @@ -870,7 +874,11 @@ TEST_F(TestActionServerCancelPolicy, test_action_process_cancel_request_single_g ASSERT_EQ(cancel_response.msg.goals_canceling.size, 1u); EXPECT_EQ(cancel_response.msg.return_code, action_msgs__srv__CancelGoal_Response__ERROR_NONE); rcl_action_goal_info_t * goal_info = &cancel_response.msg.goals_canceling.data[0]; - EXPECT_TRUE(uuidcmp(goal_info->goal_id.uuid, cancel_request.goal_info.goal_id.uuid)); + + // Passing this array by pointer can confuse scan-build into thinking the pointer is + // not checked for null. Passing by reference works fine + const auto & goal_info_uuid = goal_info->goal_id.uuid; + EXPECT_TRUE(uuidcmp(goal_info_uuid, cancel_request.goal_info.goal_id.uuid)); EXPECT_EQ(RCL_RET_OK, rcl_action_cancel_response_fini(&cancel_response)); } { diff --git a/rcl_action/test/rcl_action/test_wait.cpp b/rcl_action/test/rcl_action/test_wait.cpp index 9fd1c69..3529eef 100644 --- a/rcl_action/test/rcl_action/test_wait.cpp +++ b/rcl_action/test/rcl_action/test_wait.cpp @@ -796,6 +796,7 @@ TEST_F(TestActionServerWait, test_server_wait_set_get_entities_ready) { EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT); ret = rcl_wait_set_init(&wait_set, 0, 0, 1, 0, 3, 0, &this->context, rcl_get_default_allocator()); + ASSERT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; wait_set.services[0] = &this->action_server.impl->goal_service; this->action_server.impl->wait_set_goal_service_index = 0; wait_set.services[1] = &this->action_server.impl->cancel_service;