Improvements to building against & using master/security-enabled/security-but-disabled versions#448
Conversation
If set, dds_q{set,get}_{prop,bprop} are available.
Signed-off-by: Erik Boasson <eb@ilities.com>
When built without support for DDS Security, any attempt to create a participant QoS settings in the security name space (those prefixed by "dds.sec.") must fail. Signed-off-by: Erik Boasson <eb@ilities.com>
dpotman
left a comment
dpotman
left a comment
There was a problem hiding this comment.
LGTM, I can't find any issues other than the error code when security is not present.
src/core/ddsi/src/q_entity.c
Outdated
| { | ||
| /* disallow creating a participant with a security configuration if there is support for security | ||
| has been left out */ | ||
| ret = DDS_RETCODE_NOT_ALLOWED_BY_SECURITY; |
There was a problem hiding this comment.
Maybe replace this error code by DDS_RETCODE_PRECONDITION_NOT_MET?
There was a problem hiding this comment.
I had a debate about it with myself before pushing it to GitHub, and I just don't know what is best. My reasoning was that "not allowed by security" at least makes it immediately obvious it has something to do with the security configuration, whereas "precondition not met" is one of those overly generic ones. But at the same time, it might well make one go looking in exactly the wrong place: trying to find out what is wrong with the configuration one has specified ...
I'll mull it over a bit in the hope of getting a few votes either way before having to make the call 😀
There was a problem hiding this comment.
Mulled it over in the absence of votes one way or the other. DDS_PRECONDITION_NOT_MET it will be. Thanks!
| /* Whether or not the "property list" QoS setting is supported in this version. If it is, | ||
| the "dds.sec." properties are treated specially, preventing the accidental creation of | ||
| an non-secure participant by an implementation built without support for DDS Security. */ | ||
| #define DDS_HAS_PROPERTY_LIST_QOS 1 |
There was a problem hiding this comment.
Can you expose a DDS_HAS_SECURITY_ENABLED (or similar) as well that can be consumed by the RMW? Otherwise we will need to assume security is enabled if DDS_HAS_PROPERTY_LIST_QOS is defined, which doesn't seem to be an accurate assumption to make unless I'm missing something.
There was a problem hiding this comment.
Yes, I can — and I have just pushed a commit that provides a DDS_HAS_SECURITY you can check at compile time.
It wasn't there at first because I thought it wasn't strictly necessary, because the non-security-enabled builds will (with this PR) outright refuse the creation of any DDS participant that requests security — and secondarily because it saved me from having to deal with CMake. But I think you're right that having it is indeed useful and the only downside (having to deal with CMake) is rather silly!
There was a problem hiding this comment.
because it saved me from having to deal with CMake
Haha, been there man. Thanks for adding!
Currently: * DDS_HAS_SECURITY for DDS Security support * DDS_HAS_LIFESPAN for lifespan QoS support * DDS_HAS_DEADLINE_MISSED for "deadline missed" event support These are defined to 1 if support for the feature is included in the build and left undefined if it isn't. Signed-off-by: Erik Boasson <eb@ilities.com>
As opposed to NOT_ALLOWED_BY_SECURITY. There is a meaningful difference between something being disallowed and something being impossible. Co-Authored-By: Kyle Fazzari <github@status.e4ward.com> Signed-off-by: Erik Boasson <eb@ilities.com>
eboasson
force-pushed
the
security-config
branch
from
d2246bd to
d864fe0
Compare
|
@kyrofa and @dennis-adlink thanks for the review comments |
3 participants
This PR adds a two key features (+ a test):
DDS_HAS_PROPERTY_LIST_QOSmacro (as discussed in Enable use of Cyclone DDS security features ros2/rmw_cyclonedds#123) to allow users of Cyclone to conditionally include support for setting the QoS properties needed for configuring security. This'll allow, e.g., the RMW the layer to cover both master & security.The diff's a bit bigger than strictly necessary merely for the above:
new_participant_guidhad grown unwieldy over time and which gave a memory leak (if a rare setting was enabled) a chance to hide. A big reason was the huge gob of code for creating the built-in endpoints. A bit of refactoring helps.