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

Jump to bottom

Fix conversions between rmw_localhost_only_t and bool #670

Merged
merged 7 commits into from
Jun 3, 2020
Merged

Fix conversions between rmw_localhost_only_t and bool #670

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
364b772
Check for case RMW_LOCALHOST_ONLY_DISABLED in init_options
Blast545 Jun 2, 2020
3093642
Take init value from node context
Blast545 Jun 2, 2020
442615d
Convert enum variable to boolean
Blast545 Jun 2, 2020
ffae768
Remove extra check
Blast545 Jun 2, 2020
c992146
Remove not needed reference and dereference
Blast545 Jun 2, 2020
d45ff22
Change rcl_get_localhost_only to match signature
Blast545 Jun 3, 2020
04dfa57
Add parenthesis to make clearer precedence
Blast545 Jun 3, 2020
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
4 changes: 4 additions & 0 deletions rcl/src/rcl/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ rcl_init(
fail_ret = ret;
goto fail;
}
} else if (RMW_LOCALHOST_ONLY_DISABLED == *localhost_only) {
// If the user sets the rmw init option to RMW_LOCALHOST_ONLY_DISABLED
// manually, save the boolean variable as 0
*localhost_only = 0;
Copy link
Member

Choose a reason for hiding this comment

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

why?
context->impl->init_options.impl->rmw_init_options.localhost_only is of type rmw_localhost_only_t

what needs to be fixed is rcl_get_localhost_only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rcl_get_localhost_only is not called if a user sets localhost_only=RMW_LOCALHOST_ONLY_DISABLED in the rmw_init_options_t struct. That's the case being covered by this portion of code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in that case rcl_get_localhost_only is not being called, and there's no need to modify localhost_only value either (i.e. when context->impl->init_options.impl->rmw_init_options.localhost_only == RMW_LOCALHOST_ONLY_DISABLED things are working well).

They are not working well when context->impl->init_options.impl->rmw_init_options.localhost_only == RMW_LOCALHOST_ONLY_DEFAULT because of this line. That needs to be corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not working well when context->impl->init_options.impl->rmw_init_options.localhost_only == RMW_LOCALHOST_ONLY_DEFAULT because of this line. That needs to be corrected.

That line is returning 1 or 0 properly (although its signature is misleading and has to be changed, left an issue opened for that).

But I do think it's not OK that all the functions receiving that enum have to make the boolean comparison against RMW_LOCALHOST_ONLY_DISABLED before using the value and IMO this should be addressed in this function where the context is initialized.

I'll remove this because the check you suggested me for node.c solves the case I was addressing but it would be great to come back at this at some point.

Copy link
Member

Choose a reason for hiding this comment

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

That line is returning 1 or 0 properly

It's returning 1 or 0 and setting that to a rmw_localhost_only_t which doesn't expect 1/0, so that line is wrong. It should be:

  *localhost_only = (ros_local_host_env_val != NULL && strcmp(ros_local_host_env_val, "1") == 0) ? RMW_LOCALHOST_ONLY_ENABLED ? RMW_LOCALHOST_ONLY_DISABLED;

(although its signature is misleading and has to be changed, left an issue opened for that).

The signature is ok and shouldn't be changed. The idea is that in the future we will have the following functions:

rcl_init_options_enable_localhost_only(rcl_init_options_t * init_options);
rcl_init_options_disable_localhost_only(rcl_init_options_t * init_options);

There would be equivalents in rclcpp/rclpy for those.
If the default constructed init_options isn't modified with those functions, localhost only will be set according to the environment variable (as by default it's value is RMW_LOCALHOST_ONLY_DEFAULT).
If the user explicitly enabled/disabled it, the environment variable won't be checked.

As a consequence, at rcl level the variable can be default/enable/disable, and at rmw implementation level can be only enable/disable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a PR for that in the rcl_get_localhost_only function. This PR is about passing the context setting to the node, mind giving your thoughts on it?

Copy link
Member

Choose a reason for hiding this comment

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

I'll open a PR for that in the rcl_get_localhost_only function. This PR is about passing the context setting to the node, mind giving your thoughts on it?

I don't see why opening another PR, as I think this change is incorrect and the only problem is in rcl_get_localhost_only.
Can you explain what this extra else/if does?

This PR is about passing the context setting to the node

Can you explain what you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why opening another PR, as I think this change is incorrect and the only problem is in rcl_get_localhost_only.

IMO there are two problems:

  1. The one fixed in this PR, localhost_only being loaded from environment variable in node.c although this variable was already loaded in init.c

  2. rcl_get_localhost_only returning boolean instead of rmw_localhost_only_t.

This PR fixes problem 1.

Can you explain what this extra else/if does?

Based on my conversation with you, that else/if was removed, please check the latest commit of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

rcl_get_localhost_only returning boolean instead of rmw_localhost_only_t.

It's not returning a boolean, it's returning rmw_localhost_only_t and doing the conversion incorrectly.
IMO, we should fix the logic in this same PR as today is the deadline for merging things before Foxy release (anyways we can backport the fix in a patch release, so it's not a big deal).

Based on my conversation with you, that else/if was removed, please check the latest commit of this PR.

I didn't see the commit that removed that 😂.

}

if (context->global_arguments.impl->enclave) {
Expand Down
6 changes: 1 addition & 5 deletions rcl/src/rcl/node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,7 @@ rcl_node_init(
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id);
node->impl->actual_domain_id = domain_id;

if (RMW_LOCALHOST_ONLY_DEFAULT == localhost_only) {
if (RMW_RET_OK != rcl_get_localhost_only(&localhost_only)) {
goto fail;
}
}
localhost_only = *(&context->impl->init_options.impl->rmw_init_options.localhost_only);
Blast545 marked this conversation as resolved.
Show resolved Hide resolved

node->impl->rmw_node_handle = rmw_create_node(
&(node->context->impl->rmw_context),
Expand Down