API changes to sync with one Participant per Context change in rmw_fastrtps #392

ivanpauno · 2020-02-28T21:15:38Z

hidmic · 2020-03-19T13:57:50Z

rmw_connext_cpp/src/rmw_init.cpp

@@ -51,20 +57,38 @@ rmw_init_options_copy(const rmw_init_options_t * src, rmw_init_options_t * dst)
    RMW_SET_ERROR_MSG("expected zero-initialized dst");
    return RMW_RET_INVALID_ARGUMENT;
  }
+  const rcutils_allocator_t * allocator = &src->allocator;


@ivanpauno hmm const pointer? I would've thought it should've been a mutable reference.

I can use a reference instead of a pointer.
About constness, we've some functions that take a const pointer, there are others taking it by value, and others taking a mutable pointer.

In the first and last case, you can do:

allocator->allocate(size, allocator->state);

In the second case, you can replace -> with ..

Why does this work? I will have to check, but I think that C structs constness is not propagated to its members. So, it doesn't matter that allocate takes a mutable pointer to state.

edit: I think my comment applies to both C and C++, if you have public member that it's a pointer constness is not propagated.

I can use a reference instead of a pointer.

I was mostly interested in the rationale behind explicitly introducing an indirection.

About constness, we've some functions that take a const pointer, there are others taking it by value, and others taking a mutable pointer.

I'm aware of how allocators are passed in all forms and colors here and there. That's something we can discuss in a follow-up issue though.

Why does this work? I will have to check, but I think that C structs constness is not propagated to its members. So, it doesn't matter that allocate takes a mutable pointer to state.

A const qualified struct in C/C++ implies that none of its members can be mutated. In the case you mention, you're actually not mutating state, the pointer. AFAIK cv-qualifiers do not propagate through indirections. That's why a const std::shared_ptr<SomeObject> will happily let you mutate SomeObject, whereas const std::shared_ptr<const SomeObject> will likely get you what you want.

I was mostly interested in the rationale behind explicitly introducing an indirection.

Ok, I can use a reference if you want.

I'm aware of how allocators are passed in all forms and colors here and there. That's something we can discuss in a follow-up issue though.

👍

A const qualified struct in C/C++ implies that none of its members can be mutated. In the case you mention, you're actually not mutating state, the pointer. AFAIK cv-qualifiers do not propagate through indirections. That's why a const std::shared_ptr will happily let you mutate SomeObject, whereas const std::shared_ptr will likely get you what you want.

Yes, exactly. I misunderstood your first comment and derailed a bit.

rmw_connext_shared_cpp/src/node.cpp

hidmic · 2020-03-19T14:48:17Z

rmw_connext_shared_cpp/src/node_names.cpp

+
+      if (security_context_found != map.end()) {
+        security_context = std::string(
+          security_context_found->second.begin(), security_context_found->second.end());


@ivanpauno this is a peculiar way of copying an std::string and I suspect less efficient than a plain copy-assignment.

See https://github.com/ros2/rmw/blob/f76511333709a6f5d1e4543129ec73f2cd2b4f06/rmw/include/rmw/impl/cpp/key_value.hpp#L31.

That's a peculiar API. I wonder why a sequence of bytes. Is it to support other encodings in the future? cc @dirk-thomas

hidmic

LGTM though I have a few questions.

rmw_connext_shared_cpp/src/node.cpp

hidmic · 2020-03-25T21:01:16Z

rmw_connext_shared_cpp/src/node_names.cpp

  for (auto i = 0; i < named_nodes_num; ++i) {
    node_names->data[i] = rcutils_strdup(tmp_names_list.data[i], allocator);
    node_namespaces->data[i] = rcutils_strdup(tmp_namespaces_list.data[i], allocator);
+    if (security_contexts) {
+      security_contexts->data[i] = rcutils_strdup(tmp_security_contexts_list.data[i], allocator);
+    }


@ivanpauno meta: this predates your PR, but why does this code assume that rcutils_strdup will succeed? Also, simply moving strings in tmp arrays would be much more efficient than this (de)allocation dance.

I completely agree.
I will open an issue for this, because it's out of the scope of this PR IMO.

hidmic

LGTM

hidmic · 2020-03-26T18:56:46Z

rmw_connext_shared_cpp/src/node.cpp

  bool success = participant_qos.user_data.value.length(static_cast<DDS::Long>(length));
  if (!success) {
    RMW_SET_ERROR_MSG("failed to resize participant user_data");
    return NULL;
  }

-  int written = snprintf(
+  int written = std::snprintf(


@ivanpauno uh, why the added prefix?

Also, I just remembered that rcutils has a portable snprintf flavor.

@ivanpauno uh, why the added prefix?

it's not really needed. I can delete it again.

Also, I just remembered that rcutils has a portable snprintf flavor.

This seems to be portable, as it's currently working fine.
Do you know the rationale of having a rcutils flavor?

Not surprisingly, there's something peculiar about using snprintf in Windows. But @wjwwood may have deeper insight.

I mean, this code is passing CI on Windows with master, which does the same thing.
If CI is green, I wouldn't change anything else.

Fair enough. I won't block on it, though now I'm curious about rcutils_snprintf.

Maybe the std:: avoids it but on Windows it will usually generate a compiler warning if you try to use snprintf directly. Or at least that’s my recollection.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

hidmic

LGTM

ivanpauno added the in review Waiting for review (Kanban column) label Feb 28, 2020

ivanpauno requested review from wjwwood and hidmic February 28, 2020 21:15

ivanpauno self-assigned this Feb 28, 2020

wjwwood approved these changes Mar 2, 2020

View reviewed changes

ivanpauno mentioned this pull request Mar 3, 2020

Modify implementations to use one Participant per Context ros2/rmw#183

Closed

6 tasks

ivanpauno changed the title ~~Rename rmw_node_security_options_t to rmw_security_options_t~~ API changes to sync with one Participant per Context change in rmw_fastrtps Mar 4, 2020

ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 0835337 to bada946 Compare March 13, 2020 16:42

hidmic reviewed Mar 19, 2020

View reviewed changes

ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 0ca853b to c410e96 Compare March 19, 2020 21:33

ivanpauno requested a review from hidmic March 25, 2020 17:53

hidmic approved these changes Mar 25, 2020

View reviewed changes

ivanpauno requested a review from hidmic March 26, 2020 17:54

hidmic approved these changes Mar 26, 2020

View reviewed changes

ivanpauno added 7 commits March 30, 2020 17:52

Rename rmw_node_security_options_t to rmw_security_options_t

c4e936a

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Add rmw_use_node_name_in_security_directory_lookup

3824254

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Latest update after discussion about supporting sros2

678ef6a

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Delete unused function

4d2acfe

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Naming: replace context_name with security_context

95ee9b6

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Store context pointer in node data

3f55c0b

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Address peer review comments

5726630

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 3d53faf to 5726630 Compare March 30, 2020 20:55

ivanpauno mentioned this pull request Apr 2, 2020

Switch to one Participant per Context ros2/rmw_fastrtps#312

Merged

hidmic approved these changes Apr 2, 2020

View reviewed changes

ivanpauno merged commit 1b26f31 into master Apr 3, 2020

delete-merged-branch bot deleted the ivanpauno/one-participant-per-context branch April 3, 2020 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes to sync with one Participant per Context change in rmw_fastrtps #392

API changes to sync with one Participant per Context change in rmw_fastrtps #392

ivanpauno commented Feb 28, 2020

hidmic Mar 19, 2020

ivanpauno Mar 20, 2020 •

edited

Loading

hidmic Mar 25, 2020

ivanpauno Mar 26, 2020

hidmic Mar 19, 2020

ivanpauno Mar 20, 2020

hidmic Mar 25, 2020

hidmic left a comment

hidmic Mar 25, 2020

ivanpauno Mar 26, 2020

hidmic left a comment

hidmic Mar 26, 2020

hidmic Mar 26, 2020

ivanpauno Mar 26, 2020

hidmic Mar 26, 2020

ivanpauno Mar 26, 2020

hidmic Mar 26, 2020

wjwwood Mar 26, 2020

hidmic left a comment

API changes to sync with one Participant per Context change in rmw_fastrtps #392

API changes to sync with one Participant per Context change in rmw_fastrtps #392

Conversation

ivanpauno commented Feb 28, 2020

Choose a reason for hiding this comment

ivanpauno Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

ivanpauno Mar 20, 2020 •

edited

Loading