Clean up server data structures #94
Conversation
|
Cc @guenhter |
| historizing: attrs.historizing, | ||
| } | ||
| impl Default for VariableAttributes { | ||
| fn default() -> Self { |
There was a problem hiding this comment.
Extract the code into fn new()? std often implements a no-arg constructor that impl Default then delegates to. That would at least be consistent with Server, also part of this PR. I didn't check other occurrences.
There was a problem hiding this comment.
For ua::data_types I tend to avoid new() because it is not entirely clear what it does. Basically, there are two ways to create new instances of these data types:
init()which only creates an internally consistent but basically uninitialized state (safe to be dropped and passed around, but mostly meaningless because required attributes may be missing)default()which provides sensible defaults that can be used as-is (ClientConfig,BrowseDescription)
In this case we are dealing with the latter: while still mostly unset, it has all the required attributes pre-initialized that should be set: see ua_server_utils.c (mostly blanks, but there are some bit masks and types set).
| ); | ||
| /// This fails when the node cannot be added. | ||
| pub fn add_object_node(&mut self, node: ObjectNode) -> Result<()> { | ||
| let status_code = ua::StatusCode::new(unsafe { |
There was a problem hiding this comment.
Wouldn't it be desirable to restrict the unsafe parts to ua::Server? Unfortunately, I don't know how to achieve this 😅 So let's keep it here for now.
Maybe add a TODO comment to explain this decision?
There was a problem hiding this comment.
In principle, yes, unsafe code should (could) reside inside the ua module and we could deal only with safe abstractions when writing the higher-level code in Client/Server (outside of ua).
In this case, however, the abstraction is extremely thin and basically only forwards the call with an unsafe {} block around it. I'm not sure that we gain much by adding this indirection. I fear that it only makes it less obvious what is happening here.
But I'm not absolutely happy with the current state of affairs myself. Until we find a better way, let's at least keep it consistent between Client and Server to avoid surprises.
Description
This cleans up some of the data structures introduced in #89, fixing issues where ownership passed into the C library where it was not expected, potentially leaking memory. See the individual commits in the PR for details.