From 9aab2a84f82da825ca3d76475a3cb7ae029ce0ba Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:13:42 +0000 Subject: [PATCH 1/9] Refactor rcl_subscription_is_valid. Users should be able to query whether a subscription is valid without having an error set. Only set an error if an invalid pointer (e.g. NULL) was passed in. Signed-off-by: Chris Lalancette --- rcl/src/rcl/subscription.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 1ca6edd8c..18d83945b 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -761,11 +761,8 @@ bool rcl_subscription_is_valid(const rcl_subscription_t * subscription) { RCL_CHECK_FOR_NULL_WITH_MSG(subscription, "subscription pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - subscription->impl, "subscription's implementation is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - subscription->impl->rmw_handle, "subscription's rmw handle is invalid", return false); - return true; + + return subscription->impl != NULL && subscription->impl->rmw_handle != NULL; } rmw_ret_t From 97ddf0bbe9ca4829ba54bc8fa09bcc57f90df5b3 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:26:07 +0000 Subject: [PATCH 2/9] Simplify rcl_client_is_valid. A user should be able to query whether a client is valid without an error being set. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/client.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 355d92099..0da7b45d8 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -421,11 +421,8 @@ bool rcl_client_is_valid(const rcl_client_t * client) { RCL_CHECK_FOR_NULL_WITH_MSG(client, "client pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - client->impl, "client's rmw implementation is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - client->impl->rmw_handle, "client's rmw handle is invalid", return false); - return true; + + return client->impl != NULL && client->impl->rmw_handle != NULL; } const rmw_qos_profile_t * From 860ab94b53b96448469cf38a9b17b42c4ad2672c Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:27:09 +0000 Subject: [PATCH 3/9] Simplify rcl_event_is_valid. A user should be able to query whether an event is valid without an error being set. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/event.c | 10 +++++----- rcl/test/rcl/test_events.cpp | 19 ++++++------------- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/rcl/src/rcl/event.c b/rcl/src/rcl/event.c index 2bb2ffeec..47c42e897 100644 --- a/rcl/src/rcl/event.c +++ b/rcl/src/rcl/event.c @@ -220,13 +220,13 @@ bool rcl_event_is_valid(const rcl_event_t * event) { RCL_CHECK_FOR_NULL_WITH_MSG(event, "event pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG(event->impl, "event's implementation is invalid", return false); - if (event->impl->rmw_handle.event_type == RMW_EVENT_INVALID) { - RCUTILS_SET_ERROR_MSG("event's implementation not init"); + + if (event->impl == NULL || event->impl->rmw_handle.event_type == RMW_EVENT_INVALID) { return false; } - RCUTILS_CHECK_ALLOCATOR_WITH_MSG( - &event->impl->allocator, "not valid allocator", return false); + + RCL_CHECK_ALLOCATOR(&event->impl->allocator, return false); + return true; } diff --git a/rcl/test/rcl/test_events.cpp b/rcl/test/rcl/test_events.cpp index 34f1a0a11..585899930 100644 --- a/rcl/test/rcl/test_events.cpp +++ b/rcl/test/rcl/test_events.cpp @@ -289,15 +289,15 @@ wait_for_msgs_and_events( EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; if (nullptr != subscription) { - ret = rcl_wait_set_add_subscription(&wait_set, subscription, NULL); + ret = rcl_wait_set_add_subscription(&wait_set, subscription, nullptr); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; } if (nullptr != subscription_event) { - ret = rcl_wait_set_add_event(&wait_set, subscription_event, NULL); + ret = rcl_wait_set_add_event(&wait_set, subscription_event, nullptr); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; } if (nullptr != publisher_event) { - ret = rcl_wait_set_add_event(&wait_set, publisher_event, NULL); + ret = rcl_wait_set_add_event(&wait_set, publisher_event, nullptr); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; } @@ -712,8 +712,6 @@ TEST_F(TestEventFixture, test_event_is_valid) setup_publisher_subscriber(default_qos_profile, default_qos_profile); rcl_event_t publisher_event_test = rcl_get_zero_initialized_event(); EXPECT_FALSE(rcl_event_is_valid(&publisher_event_test)); - EXPECT_TRUE(rcl_error_is_set()); - rcl_reset_error(); rcl_ret_t ret = rcl_publisher_event_init( &publisher_event_test, &publisher, RCL_PUBLISHER_OFFERED_DEADLINE_MISSED); @@ -723,16 +721,12 @@ TEST_F(TestEventFixture, test_event_is_valid) rmw_event_type_t saved_event_type = publisher_event_test.impl->rmw_handle.event_type; publisher_event_test.impl->rmw_handle.event_type = RMW_EVENT_INVALID; EXPECT_FALSE(rcl_event_is_valid(&publisher_event_test)); - EXPECT_TRUE(rcl_error_is_set()); - rcl_reset_error(); publisher_event_test.impl->rmw_handle.event_type = saved_event_type; rcl_allocator_t saved_alloc = publisher_event_test.impl->allocator; rcl_allocator_t bad_alloc = rcutils_get_zero_initialized_allocator(); publisher_event_test.impl->allocator = bad_alloc; EXPECT_FALSE(rcl_event_is_valid(&publisher_event_test)); - EXPECT_TRUE(rcl_error_is_set()); - rcl_reset_error(); publisher_event_test.impl->allocator = saved_alloc; ret = rcl_event_fini(&publisher_event_test); @@ -744,18 +738,17 @@ TEST_F(TestEventFixture, test_event_is_valid) * Test passing not init to take_event/get_handle */ TEST_F(TestEventFixture, test_event_is_invalid) { - // nullptr rmw_offered_deadline_missed_status_t deadline_status; - EXPECT_EQ(RCL_RET_EVENT_INVALID, rcl_take_event(NULL, &deadline_status)); + EXPECT_EQ(RCL_RET_EVENT_INVALID, rcl_take_event(nullptr, &deadline_status)); rcl_reset_error(); - EXPECT_EQ(NULL, rcl_event_get_rmw_handle(NULL)); + EXPECT_EQ(nullptr, rcl_event_get_rmw_handle(nullptr)); rcl_reset_error(); // Zero Init, invalid rcl_event_t publisher_event_test = rcl_get_zero_initialized_event(); EXPECT_EQ(RCL_RET_EVENT_INVALID, rcl_take_event(&publisher_event_test, &deadline_status)); rcl_reset_error(); - EXPECT_EQ(NULL, rcl_event_get_rmw_handle(&publisher_event_test)); + EXPECT_EQ(nullptr, rcl_event_get_rmw_handle(&publisher_event_test)); rcl_reset_error(); } From 8e23cd13cfb82cb469c2fc530fed23ef1b8be62e Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:27:30 +0000 Subject: [PATCH 4/9] Simplify rcl_node_is_valid. A user should be able to query whether a node is valid without setting an error. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/node.c | 18 +++++------------- rcl/test/rcl/test_node.cpp | 2 -- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index b0fe0cfc2..1940a007f 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -429,24 +429,16 @@ bool rcl_node_is_valid_except_context(const rcl_node_t * node) { RCL_CHECK_FOR_NULL_WITH_MSG(node, "rcl node pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG(node->impl, "rcl node implementation is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - node->impl->rmw_node_handle, "rcl node's rmw handle is invalid", return false); - return true; + + return node->impl != NULL && node->impl->rmw_node_handle != NULL; } bool rcl_node_is_valid(const rcl_node_t * node) { - bool result = rcl_node_is_valid_except_context(node); - if (!result) { - return result; - } - if (!rcl_context_is_valid(node->context)) { - RCL_SET_ERROR_MSG("rcl node's context is invalid"); - return false; - } - return true; + RCL_CHECK_FOR_NULL_WITH_MSG(node, "rcl node pointer is invalid", return false); + + return rcl_node_is_valid_except_context(node) && rcl_context_is_valid(node->context); } const char * diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index 8ab234721..3917e3b01 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -283,8 +283,6 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors) rcl_reset_error(); ret = rcl_node_get_domain_id(&zero_node, &actual_domain_id); EXPECT_EQ(RCL_RET_NODE_INVALID, ret); - ASSERT_TRUE(rcl_error_is_set()); - rcl_reset_error(); ret = rcl_node_get_domain_id(&invalid_node, &actual_domain_id); EXPECT_EQ(RCL_RET_NODE_INVALID, ret); rcl_reset_error(); From d7e66647799cf920174dda560d242c2f2df0a32b Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:27:54 +0000 Subject: [PATCH 5/9] Simplify rcl_publisher_is_valid. A user should be able to query whether a publisher is valid without it setting an error. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/publisher.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 634f17241..4a2481a0a 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -417,27 +417,19 @@ rcl_publisher_get_context(const rcl_publisher_t * publisher) bool rcl_publisher_is_valid(const rcl_publisher_t * publisher) { - if (!rcl_publisher_is_valid_except_context(publisher)) { - return false; // error already set - } - if (!rcl_context_is_valid(publisher->impl->context)) { - RCL_SET_ERROR_MSG("publisher's context is invalid"); - return false; - } - RCL_CHECK_FOR_NULL_WITH_MSG( - publisher->impl->rmw_handle, "publisher's rmw handle is invalid", return false); - return true; + RCL_CHECK_FOR_NULL_WITH_MSG(publisher, "publisher pointer is invalid", return false); + + return rcl_publisher_is_valid_except_context(publisher) && + rcl_context_is_valid(publisher->impl->context) && + publisher->impl->rmw_handle != NULL; } bool rcl_publisher_is_valid_except_context(const rcl_publisher_t * publisher) { RCL_CHECK_FOR_NULL_WITH_MSG(publisher, "publisher pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - publisher->impl, "publisher implementation is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - publisher->impl->rmw_handle, "publisher's rmw handle is invalid", return false); - return true; + + return publisher->impl != NULL && publisher->impl->rmw_handle != NULL; } rcl_ret_t From f805b974880a0f820c49d2b2b6f2d90ed64f15ab Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:28:24 +0000 Subject: [PATCH 6/9] Simplify rcl_service_is_valid. A user should be able to query whether a service is valid without it setting an error. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/service.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 1cca57acc..050c035b7 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -423,11 +423,8 @@ bool rcl_service_is_valid(const rcl_service_t * service) { RCL_CHECK_FOR_NULL_WITH_MSG(service, "service pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - service->impl, "service's implementation is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - service->impl->rmw_handle, "service's rmw handle is invalid", return false); - return true; + + return service->impl != NULL && service->impl->rmw_handle != NULL; } const rmw_qos_profile_t * From a77b3c92f39960b5ee8a26a1308f80b4fb14d7c0 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:28:47 +0000 Subject: [PATCH 7/9] Simplify rcl_service_event_publisher_is_valid. A user should be able to query whether a service event publisher is valid without it setting an error. Fix that here. Signed-off-by: Chris Lalancette --- rcl/src/rcl/service_event_publisher.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/rcl/src/rcl/service_event_publisher.c b/rcl/src/rcl/service_event_publisher.c index f262dba70..f53af5605 100644 --- a/rcl/src/rcl/service_event_publisher.c +++ b/rcl/src/rcl/service_event_publisher.c @@ -40,14 +40,9 @@ rcl_service_event_publisher_is_valid(const rcl_service_event_publisher_t * servi { RCL_CHECK_FOR_NULL_WITH_MSG( service_event_publisher, "service_event_publisher is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - service_event_publisher->service_type_support, - "service_event_publisher's service type support is invalid", return false); - if (!rcl_clock_valid(service_event_publisher->clock)) { - RCL_SET_ERROR_MSG("service_event_publisher's clock is invalid"); - return false; - } - return true; + + return service_event_publisher->service_type_support != NULL && + rcl_clock_valid(service_event_publisher->clock); } static rcl_ret_t introspection_create_publisher( From 5ede8a8bbb6a066387926b71fbc13c130392cf7c Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 28 Jul 2023 16:29:48 +0000 Subject: [PATCH 8/9] Cleanup rcl_wait_set_is_valid. A user should be able to query whether a wait_set is valid without it setting an error, which it already does. However, passing a NULL into it is logically invalid; we can't possibly answer that question. So set an error in that case. Signed-off-by: Chris Lalancette --- rcl/src/rcl/wait.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rcl/src/rcl/wait.c b/rcl/src/rcl/wait.c index 1f79a6a54..3b0d4944c 100644 --- a/rcl/src/rcl/wait.c +++ b/rcl/src/rcl/wait.c @@ -84,7 +84,9 @@ rcl_get_zero_initialized_wait_set() bool rcl_wait_set_is_valid(const rcl_wait_set_t * wait_set) { - return wait_set && wait_set->impl; + RCL_CHECK_FOR_NULL_WITH_MSG(wait_set, "wait set pointer is invalid", return false); + + return wait_set->impl != NULL; } static void From 43f41ddd1d204a078f13bf56051e891fc5c1caff Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Sun, 6 Aug 2023 20:10:57 +0000 Subject: [PATCH 9/9] Fixes after CI. Signed-off-by: Chris Lalancette --- rcl_action/test/rcl_action/test_action_client.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/rcl_action/test/rcl_action/test_action_client.cpp b/rcl_action/test/rcl_action/test_action_client.cpp index b091809ce..3ace83289 100644 --- a/rcl_action/test/rcl_action/test_action_client.cpp +++ b/rcl_action/test/rcl_action/test_action_client.cpp @@ -381,7 +381,6 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini_maybe_fail) rcl_reset_error(); } } else { - EXPECT_TRUE(rcl_error_is_set()); rcl_reset_error(); } EXPECT_EQ(RCL_RET_OK, rcl_node_fini(&node));