add general check for yaml on rcl_yaml_param_parser by iuhilnehc-ynos · Pull Request #809 · ros2/rcl

iuhilnehc-ynos · 2020-09-22T09:46:36Z

Fixes #301

Signed-off-by: Chen Lihui Lihui.Chen@sony.com

Updated: Besides fixing to not allow '//' in node name, adding the general check and some special rules about wildcards(including some new rules on ros2/design#303)

fujitatomoya · 2020-09-23T05:59:08Z

rcl_yaml_param_parser/src/namespace.c

+/// Validate a name whether it is a valid namespace
+///
+static rcutils_ret_t
+__validate_namespace(const char * name)


why it should not be dependent on rmw? could you elaborate a little bit? I think it would be better to use rmw to validate the namespace instead of having redundant code else where.

I found that using rmw_validate_namespace has some limitations because there are some extra rules, such as "/**" (maybe more in the future), need to be parsed.

you mean ros2/rclcpp#1265?

Thanks for your questions, @fujitatomoya

why it should not be dependent on rmw?

"/**" is a special rule at rclcpp, it is invalid checked by rmw_validate_namespace.
If using rmw_validate_namespace, we need to add more extra checking for these special rules inside __validate_namespace, which seems not a requirement of this issue.

maybe more in the future

you mean ros2/rclcpp#1265?

Yes.

Though this solves the reported issue, I think we should do a more general check if the node namespace (and node name) are valid. I would do this using rmw_validate_namespace and rmw_validate_node_name, and handle the special cases for wildcards * and **.

I suggest implementing this function as follows:

Make a copy of the name to validate

Substitute substrings * and ** with arbitrary valid strings (e.g. WILDCARD)

Pass the modified name to rmw_validate_namespace

By using the rmw functions we are consistent with what it means to be a valid name.

What do you think?

@jacobperron

I suggest implementing this function as follows

Thanks for your suggestion. I'll update it later.

jacobperron

I think we should also validate the node name. If you don't want to add it to this PR, we do it in a follow-up.

jacobperron · 2020-10-09T19:50:11Z

rcl_yaml_param_parser/src/namespace.c

+/// Validate a name whether it is a valid namespace
+///
+static rcutils_ret_t
+__validate_namespace(const char * name)


Though this solves the reported issue, I think we should do a more general check if the node namespace (and node name) are valid. I would do this using rmw_validate_namespace and rmw_validate_node_name, and handle the special cases for wildcards * and **.

I suggest implementing this function as follows:

Make a copy of the name to validate

Substitute substrings * and ** with arbitrary valid strings (e.g. WILDCARD)

Pass the modified name to rmw_validate_namespace

By using the rmw functions we are consistent with what it means to be a valid name.

What do you think?

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

This reverts commit b847544. Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

This reverts commit 52bb028. Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos · 2020-10-12T06:09:31Z

@jacobperron

After rebasing from master, I need to force push these commits to this branch.
Could you please help review it again?

BTW: I didn't add some extra rules which are not merged (such as ros2/rclcpp#1265) in this PR

jacobperron

LGTM.

Could you also add a test for the /** case? Thanks!

rcl_yaml_param_parser/src/parse.c

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos · 2020-10-13T06:08:32Z

@jacobperron

Could you also add a test for the /** case? Thanks!

I have added the test and added additional rules for wildcards based on ros2/design#303.
Could you review it again?

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

rcl_yaml_param_parser/src/parse.c

…partial matches Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

rcl_yaml_param_parser/src/parse.c

jacobperron

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos · 2020-10-16T03:10:07Z

The rcl_yaml_param_parser.cppcheck.error: syntaxError (src/parse.c:478) on macOS confused me, I'll rebuild it on my local environment tommorow because I have no environment now.

iuhilnehc-ynos · 2020-10-16T03:17:04Z

@ros-pull-request-builder retest this please

jacobperron · 2020-10-16T18:52:14Z

Linux
Linux-aarch64
macOS
Windows

jacobperron

I'm not sure about the cppcheck failure on macOS or the Windows build failure 🤔

rcl_yaml_param_parser/src/parse.c

jacobperron · 2020-10-16T19:30:53Z

rcl_yaml_param_parser/src/parse.c

+/// \return RCUTILS_RET_ERROR if an unspecified error occurred.
+RCL_YAML_PARAM_PARSER_LOCAL
+rcutils_ret_t
+__validate_namespace(const char * namespace);


I think we should use a single underscore. Double underscores are reserved identifiers.

Suggested change

__validate_namespace(const char * namespace);

_validate_namespace(const char * namespace);

Same for other functions names below.

That's good to know.

iuhilnehc-ynos · 2020-10-17T04:42:54Z

cppcheck failure on macOS

the version of cppcheck on macOS is 1.89, so I should not use 'namespace' as a variable name which is a keyword in C++.

Co-authored-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

jacobperron · 2020-10-19T16:15:07Z

Linux
Linux-aarch64
macOS
Windows

jacobperron

LGTM, thanks for iterating!

fujitatomoya reviewed Sep 23, 2020

View reviewed changes

clalancette assigned jacobperron Oct 9, 2020

jacobperron reviewed Oct 9, 2020

View reviewed changes

Chen Lihui added 6 commits October 12, 2020 11:08

Fix for allowing '//' in node name

e8e6e47

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Remove rmw dependency, just to check two slashes

52bb028

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Fix compile warning

b847544

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Revert "Fix compile warning"

deba99e

This reverts commit b847544. Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Revert "Remove rmw dependency, just to check two slashes"

83313f8

This reverts commit 52bb028. Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Update for a more general check

682d4db

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos force-pushed the topic-fix-two-slash-in-node branch from 1c5ac73 to 682d4db Compare October 12, 2020 06:04

iuhilnehc-ynos changed the title ~~Fix for allowing '//' in node name~~ Fix to not allow '//' in node name Oct 12, 2020

jacobperron reviewed Oct 12, 2020

View reviewed changes

rcl_yaml_param_parser/src/parse.c Outdated Show resolved Hide resolved

Chen Lihui added 3 commits October 13, 2020 10:00

Add a test case for wildcards

c91c40e

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

refactor for __validate_name

94f1278

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

Add extra special rules for wildcards

07a92fa

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos changed the title ~~Fix to not allow '//' in node name~~ add general check for yaml on rcl_yaml_param_parser Oct 13, 2020

Update for /* wildcards

97185bc

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos requested a review from jacobperron October 13, 2020 08:39

Add more test case

1b2f778

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

jacobperron reviewed Oct 13, 2020

View reviewed changes

rcl_yaml_param_parser/src/parse.c Outdated Show resolved Hide resolved

Update for allowing wildcards at any location and add test cases for …

dafd819

…partial matches Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos requested a review from jacobperron October 14, 2020 10:35

jacobperron reviewed Oct 14, 2020

View reviewed changes

rcl_yaml_param_parser/src/parse.c Show resolved Hide resolved

rcl_yaml_param_parser/src/parse.c Show resolved Hide resolved

jacobperron approved these changes Oct 16, 2020

View reviewed changes

Fix for ci on arch64 and windows platform

72f8dfc

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

jacobperron reviewed Oct 16, 2020

View reviewed changes

Fix for ci on macOS and windows platform

dda7f86

Co-authored-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>

iuhilnehc-ynos requested a review from jacobperron October 19, 2020 02:02

jacobperron approved these changes Oct 19, 2020

View reviewed changes

jacobperron merged commit 9f6b827 into ros2:master Oct 19, 2020

ivanpauno mentioned this pull request Oct 29, 2020

Describe wildcard usage in parameter files ros2/design#303

Merged

	__validate_namespace(const char * namespace);
	_validate_namespace(const char * namespace);

Conversation

iuhilnehc-ynos commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujitatomoya Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

jacobperron Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

jacobperron Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iuhilnehc-ynos commented Oct 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos commented Oct 16, 2020

Uh oh!

iuhilnehc-ynos commented Oct 16, 2020

Uh oh!

jacobperron commented Oct 16, 2020

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobperron Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos commented Oct 17, 2020

Uh oh!

jacobperron commented Oct 19, 2020

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iuhilnehc-ynos commented Sep 22, 2020 •

edited

Loading

iuhilnehc-ynos commented Oct 12, 2020 •

edited

Loading