Optimize namespace node and topic validation by gaoethan · Pull Request #130 · ros2/rmw

gaoethan · 2017-11-22T05:26:11Z

Make the result reflection more accurate, which should be
different from the default NULL
Use rmw_namespace_validation_result_string instead
for namespace validation

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

gaoethan · 2017-11-22T05:48:02Z

#ros2/rclcpp#405

mikaelarguedas · 2017-11-22T06:19:44Z

It looks like the string returned was changed from "X is valid" to NULL in c8d25e6.
So I'll leave it to @wjwwood to give feedback on this as I don't remember what motivated the change.

My guess is that this "matches" a return code of 0 when it succeeds, allowing you to easily know that it went well without having to do costly string operations.

If this was the motivation I would recommend returning "unknown X validation result" in the default case rather than changing the return value when the topic/node/namespace name is valid.

Good catch for the use of the wrong function in the namspace tests 👍

clalancette · 2017-11-22T14:18:05Z

Doing a quick, dirty audit of the callers, it seems like almost none of them expect NULL to be returned from the validation_error_string APIs (most of them do something like RCUTILS_SET_ERROR_MSG, which will crash with a NULL error_string). Therefore, I think we should probably change it so that the validation_error_string APIs never return NULL (even in the default case), and audit all of the callers to make sure the correct thing will happen.

gaoethan · 2017-11-23T01:41:00Z

yes, I opt to unify all the specific error string in validation_error_string to avoid extra code to deal with NULL out.

gaoethan · 2017-11-28T07:22:42Z

@wjwwood @mikaelarguedas The NULL return is removed and unify the result sting therein, and the relevant adaption required in the components which are affected by it are referenced, please review and give your comments, thanks !

mikaelarguedas

Thanks @gaoethan for iterating.
I'll leave @wjwwood a chance to comment on #130 (comment) before reviewing this and related PRs.
I still think that having to do string comparison instead of checking for NULL is pretty expensive and would prefer to have NULL returned for the valid cases,

mikaelarguedas · 2017-11-28T19:07:57Z

rmw/test/test_validate_full_topic_name.cpp

  ASSERT_EQ(RMW_TOPIC_VALID, validation_result);

-  ASSERT_EQ((char *)NULL, rmw_full_topic_name_validation_result_string(validation_result));
+  ret = strcmp("valid topic name", rmw_full_topic_name_validation_result_string(validation_result));


ret is of type rmw_ret_t that may or may not be an int in the future. I'd recommend using

ASSERT_STREQ("valid topic name", rmw_full_topic_name_validation_result_string(validation_result));

instead. (same in all tests using strcmp).

wjwwood · 2017-11-28T20:12:21Z

I still think that having to do string comparison instead of checking for NULL is pretty expensive and would prefer to have NULL returned for the valid cases,

Agreed. -1 to string comparison. You could instead have it return a ret code and pass a cstring pointer in to be filled with the result. But it should be possible without string comparison to know if a valid string was passed.

gaoethan · 2017-11-29T02:57:34Z

actually, I also think it's a little heavy to compare with the specific string, so I agree with you to return NULL for valid case. however:

you know, when there is NULL returned, the valid case (e.g RMW_NODE_NAME_VALID) should be checked firstly before stepping next to rmw_*_validation_result_string to make sure no NULL will returned, or it's potential to deference to NULL like in RCUTILS_SET_ERROR_MSG and rclcpp PR 405
If merely checking not NULL in test to avoid specific string comparing, it's not sure for really at a certain illegal case, even though it's not a big problem here for the tests, you know, non-NULL has several illegal cases.

gaoethan · 2017-12-13T04:01:12Z

@ros2/team, any new comments ? I can tweak it if there is to avoid its pending 😄 thanks !

mikaelarguedas · 2017-12-15T01:27:17Z

sorry for the delay @gaoethan I'll review this and the corresponding PRs in the next ~~24h~~ 48h

mikaelarguedas

lgtm, thanks @gaoethan

mikaelarguedas · 2017-12-17T03:01:26Z

I just reviewed these. Once last comments are addressed we'll need to rerun CI on this set of PRs before merging

mikaelarguedas · 2017-12-18T13:54:16Z

@gaoethan can you please rebase the branches of all these PRs? as we changed some things in the rmw API this cannot be built / tested on CI without being rebased

gaoethan · 2017-12-19T02:10:35Z

@mikaelarguedas All have already been rebased, and you can go ahead now, :) thanks !

make the valid result more accurate, which should be different from the default NULL use rmw_namespace_validation_result_string instead for namespace validation Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

and twaek the tests accordingly Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

mikaelarguedas · 2017-12-22T01:23:29Z

thanks @gaoethan for iterating, here's a round of ci on this set of PRs

Linux
Linux-aarch64
macOS
Windows

gaoethan · 2018-01-05T02:47:28Z

@mikaelarguedas it seems that CI has passed, should it be merged now ? thanks.

mikaelarguedas · 2018-01-08T01:34:12Z

Sorry @gaoethan for the delay, I meant to tag @wjwwood on this set of PRs to see if the change makes sense to him as he is the original author. This looks good to me and CI is still passing

I reran CI just to make sure nothing changes since.

Linux
Linux-aarch64
macOS (unrelated)
Windows (unrelated)

mikaelarguedas · 2018-01-08T19:01:26Z

Thanks @wjwwood I'll merge the connected PRs as well then

wjwwood · 2018-01-08T19:09:42Z

Oh, sorry I missed that there were connected pr's. Thanks.

gaoethan mentioned this pull request Nov 22, 2017

Fix the dereference to NULL ros2/rclcpp#405

Merged

mikaelarguedas added the in review Waiting for review (Kanban column) label Nov 24, 2017

This was referenced Nov 28, 2017

adapt to NULL removal from rmw result validation string ros2/rcl#193

Merged

adapt to the NULL removal from rmw result validation string ros2/rclpy#157

Merged

mikaelarguedas reviewed Nov 28, 2017

View reviewed changes

mikaelarguedas approved these changes Dec 17, 2017

View reviewed changes

gaoethan force-pushed the bugfix branch from 6a486a1 to 59df7e6 Compare December 19, 2017 02:01

gaoethan added 4 commits December 19, 2017 17:37

Optimize namespace node and topic validation

c42a3bf

make the valid result more accurate, which should be different from the default NULL use rmw_namespace_validation_result_string instead for namespace validation Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

no NULL returned from validation string

976a72d

and twaek the tests accordingly Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

keep NULL for the valid case and adapt tests

c49b6f7

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

tweak the default error string returned

59df7e6

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

wjwwood approved these changes Jan 8, 2018

View reviewed changes

wjwwood merged commit 589c69e into ros2:master Jan 8, 2018

wjwwood removed the in review Waiting for review (Kanban column) label Jan 8, 2018

gaoethan deleted the bugfix branch January 9, 2018 07:04

Conversation

gaoethan commented Nov 22, 2017

Uh oh!

gaoethan commented Nov 22, 2017

Uh oh!

mikaelarguedas commented Nov 22, 2017

Uh oh!

clalancette commented Nov 22, 2017

Uh oh!

gaoethan commented Nov 23, 2017

Uh oh!

gaoethan commented Nov 28, 2017

Uh oh!

mikaelarguedas left a comment • 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!

mikaelarguedas Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Nov 28, 2017

Uh oh!

gaoethan commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaoethan commented Dec 13, 2017 • edited by dirk-thomas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented Dec 18, 2017

Uh oh!

gaoethan commented Dec 19, 2017

Uh oh!

mikaelarguedas commented Dec 22, 2017

Uh oh!

gaoethan commented Jan 5, 2018

Uh oh!

mikaelarguedas commented Jan 8, 2018

Uh oh!

mikaelarguedas commented Jan 8, 2018

Uh oh!

wjwwood commented Jan 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikaelarguedas left a comment •

edited

Loading

gaoethan commented Nov 29, 2017 •

edited

Loading

gaoethan commented Dec 13, 2017 •

edited by dirk-thomas

Loading

mikaelarguedas commented Dec 15, 2017 •

edited

Loading

mikaelarguedas commented Dec 17, 2017 •

edited

Loading