Enhance OPC DA client functionality with new module structure and updates #9
Conversation
WalkthroughThis pull request introduces multiple changes across various files in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
opc_da/src/client/def.rs (2)
3-18: Add documentation for public APIConsider adding documentation comments for the
ServerVersionenum and its method to improve API usability. Include information about what each version represents and any compatibility considerations.Example:
+/// Represents different versions of OPC DA servers. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum ServerVersion { + /// OPC DA 1.0 specification Version10, + /// OPC DA 2.0 specification Version20, + /// OPC DA 3.0 specification Version30, } impl ServerVersion { + /// Converts the server version to its corresponding COM GUID/CLSID. pub fn to_guid(&self) -> windows_core::GUID {
50-58: Consider using Vec's dedup functionalityThe current implementation uses
HashSetfor deduplication. Consider using Vec's built-indedupmethod if order preservation is important:pub fn with_versions<I: IntoIterator<Item = ServerVersion>>(mut self, versions: I) -> Self { - let versions = versions - .into_iter() - .collect::<std::collections::HashSet<_>>(); - - self.available_versions = versions.into_iter().collect(); + self.available_versions = versions.into_iter().collect(); + self.available_versions.sort_unstable(); + self.available_versions.dedup(); self }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.vscode/settings.json(1 hunks)opc_da/src/client/def.rs(1 hunks)opc_da/src/client/iter.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)opc_da/src/com/client.rs(0 hunks)opc_da/src/com/mod.rs(0 hunks)
💤 Files with no reviewable changes (2)
- opc_da/src/com/mod.rs
- opc_da/src/com/client.rs
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🔇 Additional comments (2)
opc_da/src/client/def.rs (1)
20-41: 🛠️ Refactor suggestion
Add validation for empty version lists
The ServerFilter could potentially be constructed with empty version lists, which might cause issues when used with get_servers. Consider adding validation or documentation about the expected non-empty state.
Also, consider adding methods to modify requires_versions similar to available_versions for API completeness.
opc_da/src/client/mod.rs (1)
22-30: Address TODOs and improve error handling
There are two TODOs that need attention:
- Using
CoCreateInstanceEx- This should be implemented for better control over server creation - Converting from filters - The comment's purpose is unclear and should be clarified or removed
Also, consider adding error context:
- windows::Win32::System::Com::CoCreateInstance(
- &id,
- None,
- windows::Win32::System::Com::CLSCTX_ALL,
- )?
+ windows::Win32::System::Com::CoCreateInstance(
+ &id,
+ None,
+ windows::Win32::System::Com::CLSCTX_ALL,
+ ).map_err(|e| format!("Failed to create OPC server list: {:?}", e))?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
opc_da/src/client/iter.rs (2)
7-10: ImplementSendandSynctraits forClientif neededConsider implementing the
SendandSynctraits for theClientstruct if instances are intended to be shared across threads safely. Ensure that all contained types also implement these traits or provide appropriate synchronization mechanisms.
1-4: Improve documentation for theGuidIterstructTo enhance code readability and maintainability, consider expanding the documentation to explain the purpose and usage of the
GuidIterstruct more thoroughly.opc_da/src/client/mod.rs (2)
31-38: UseCoCreateInstanceExfor better control over instance creationThe TODO comment at line 31 suggests using
CoCreateInstanceExinstead ofCoCreateInstance.CoCreateInstanceExoffers more flexibility, especially in remote scenarios or when specifying security settings.Apply this change to use
CoCreateInstanceEx:let servers: opc_da_bindings::IOPCServerList = unsafe { - // TODO: Use CoCreateInstanceEx - windows::Win32::System::Com::CoCreateInstance( - &id, - None, - windows::Win32::System::Com::CLSCTX_ALL, - )? + let mut servers = None; + windows::Win32::System::Com::CoCreateInstanceEx( + &id, + None, + windows::Win32::System::Com::CLSCTX_ALL, + None, + &mut servers, + )?; + servers.unwrap() };
55-56: Provide more detailed error information inget_serversWhen handling errors in
get_servers, consider including the original error message to aid in debugging.Apply this diff to enhance the error message:
.map_err(|e| { - windows_core::Error::new(e.code(), "Failed to enumerate server classes") + windows_core::Error::new( + e.code(), + format!("Failed to enumerate server classes: {}", e.message()), + ) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
opc_da/src/client/iter.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)
🔇 Additional comments (2)
opc_da/src/client/mod.rs (2)
10-23: 🛠️ Refactor suggestion
Ensure thread safety when initializing COM in ensure_com
The use of static mut and unsafe blocks can introduce thread safety issues. Consider using std::sync::OnceLock (available in Rust 1.63 and above) or other synchronization primitives to safely manage COM initialization without unsafe code.
Apply this diff to improve safety:
-pub fn ensure_com() -> windows_core::Result<()> {
- static COM_INIT: std::sync::Once = std::sync::Once::new();
- static mut COM_RESULT: Option<windows_core::HRESULT> = None;
-
- unsafe {
- COM_INIT.call_once(|| {
- COM_RESULT = Some(windows::Win32::System::Com::CoInitializeEx(
- None,
- windows::Win32::System::Com::COINIT_MULTITHREADED,
- ));
- });
- COM_RESULT.unwrap_or(windows::Win32::Foundation::S_OK).ok()
- }
+pub fn ensure_com() -> windows_core::Result<()> {
+ use std::sync::OnceLock;
+ static COM_RESULT: OnceLock<windows_core::Result<()>> = OnceLock::new();
+
+ COM_RESULT
+ .get_or_init(|| {
+ unsafe {
+ windows::Win32::System::Com::CoInitializeEx(
+ None,
+ windows::Win32::System::Com::COINIT_MULTITHREADED,
+ )
+ .ok()
+ }
+ })
+ .clone()
}Likely invalid or redundant comment.
25-60: Verify that server filters are correctly converted to GUIDs
Ensure that the conversion from ServerVersion to GUIDs in the get_servers method accurately reflects the intended server versions for both available and required categories.
Run the following script to check the GUID conversion:
✅ Verification successful
Let me verify the actual GUIDs from the bindings to ensure they match the OPC DA specification.
Let me try searching in Windows metadata files which might contain these GUID definitions.
Server version GUIDs are correctly implemented and match OPC DA specification
The implementation correctly maps server versions to their standardized GUIDs:
- Version 1.0:
63d5f430-cfe4-11d1-b2c8-0060083ba1fb - Version 2.0:
63d5f432-cfe4-11d1-b2c8-0060083ba1fb - Version 3.0:
cc603642-66d7-48f1-b69a-b625e73652d7
These GUIDs are properly defined in the bindings and correctly used in the to_guid() implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ServerVersion variants correctly map to GUIDs.
# Expected GUID mappings
declare -A expected_guids=(
["Version1"]="GUID_FOR_VERSION1"
["Version2"]="GUID_FOR_VERSION2"
# Add other versions as necessary
)
# Check the to_guid() implementation in server_filter.rs
rg -A 5 'impl ServerVersion' opc_da/src/client/def.rs
# Confirm that each ServerVersion variant maps to the correct GUID
for version in "${!expected_guids[@]}"; do
guid="${expected_guids[$version]}"
# Use ast-grep to search for the mapping
ast-grep --pattern $'ServerVersion::'$version' => GUID::from_u128(0x'$guid'),'
done
Length of output: 817
Script:
#!/bin/bash
# Check the actual GUID constants in the bindings
rg "CATID_OPCDAServer" opc_da_bindings/src/lib.rs -A 1
Length of output: 54
Script:
#!/bin/bash
# Search for CATID_OPCDAServer in Windows metadata files
fd -e idl -e h -e hpp . | xargs rg "CATID_OPCDAServer" -A 1
# Also check if these are defined in any other Rust files
rg "CATID_OPCDAServer" -t rust -A 1
Length of output: 9840
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
opc_da/src/client/iter.rs (1)
1-10: Enhance documentation with usage examples and field descriptions.While the safety notice is good, the documentation would benefit from:
- Description of each field's purpose
- Example usage
- Explanation of the caching mechanism
Add the following to the documentation:
/// Iterator over COM GUIDs from IEnumGUID. /// /// # Safety /// This struct wraps a COM interface and must be used according to COM rules. +/// +/// # Fields +/// - `iter`: The underlying COM interface for enumerating GUIDs +/// - `cache`: Internal buffer to store batches of GUIDs (size: 16) +/// - `count`: Number of valid GUIDs remaining in the cache +/// - `finished`: Indicates if iteration is complete +/// +/// # Example +/// ```no_run +/// use opc_da::client::GuidIter; +/// +/// fn process_guids(iter: GuidIter) { +/// for guid in iter { +/// match guid { +/// Ok(id) => println!("Found GUID: {:?}", id), +/// Err(e) => eprintln!("Error: {:?}", e), +/// } +/// } +/// } +/// ```opc_da/src/client/com.rs (2)
3-6: Consider reducing visibility and adding documentation.The thread-local variables are marked as
pubbut appear to be implementation details. Consider:
- Reducing visibility to private unless these need to be accessed from other modules
- Adding documentation to explain their purpose and thread-safety guarantees
+/// Thread-local storage for tracking COM initialization state. thread_local! { - pub static COM_INIT: std::cell::RefCell<std::sync::Once> = const { std::cell::RefCell::new(std::sync::Once::new()) }; - pub static COM_RESULT: std::cell::RefCell<windows_core::HRESULT> = const { std::cell::RefCell::new(windows::Win32::Foundation::S_OK) }; + static COM_INIT: std::cell::RefCell<std::sync::Once> = const { std::cell::RefCell::new(std::sync::Once::new()) }; + static COM_RESULT: std::cell::RefCell<windows_core::HRESULT> = const { std::cell::RefCell::new(windows::Win32::Foundation::S_OK) }; }
1-23: Consider flexibility in COM threading model.Given that this is part of an OPC DA client implementation that will handle server version filtering and GUID iteration, consider:
- Making the COM threading model configurable to support different server requirements
- Adding support for apartment threading model (COINIT_APARTMENTTHREADED) which might be required by some OPC DA servers
- Documenting threading model requirements for different server versions
Would you like assistance in implementing a configurable threading model?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
opc_da/src/client/com.rs(1 hunks)opc_da/src/client/iter.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opc_da/src/client/mod.rs
🔇 Additional comments (2)
opc_da/src/client/iter.rs (2)
12-22: LGTM!
The constructor is well-implemented with appropriate visibility and initialization.
27-49:
Fix error handling and array bounds safety issues
The next method needs several improvements:
- Fix error handling and array bounds safety:
fn next(&mut self) -> Option<Self::Item> {
+ if self.finished {
+ return None;
+ }
+
if self.count == 0 {
let ids = &mut self.cache;
- let count = &mut self.count;
+ let mut fetched = 0u32;
- let code = unsafe { self.iter.Next(ids, Some(count)) };
- if code.is_ok() {
- if *count == 0 {
+ match unsafe { self.iter.Next(ids, Some(&mut fetched)) } {
+ Ok(_) => {
+ if fetched == 0 {
+ self.finished = true;
+ return None;
+ }
+ self.count = fetched;
+ }
+ Err(e) => {
self.finished = true;
- return None;
+ return Some(Err(windows_core::Error::new(
+ e.code(),
+ format!("Failed to enumerate GUIDs: {:?}", e),
+ )));
}
- } else {
- self.finished = true;
- return Some(Err(windows_core::Error::new(
- code,
- "Failed to get next GUID",
- )));
}
}
+ // Safe to subtract as we've verified count > 0
self.count -= 1;
- Some(Ok(self.cache[self.count as usize]))
+ // Safe to access as count was verified and decremented
+ Some(Ok(self.cache[self.count as usize]))
}Key improvements:
- Early return for finished state
- Better error handling with detailed messages
- Clearer separation of fetching and count management
- Added safety comments for array access
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
opc_da/src/client/actor.rs (2)
30-35: Consider using more specific error codes for better error handlingIn the
convert_errorfunction, usingwindows::Win32::Foundation::E_FAILas a generic error code might make debugging more difficult. Mapping specificMailboxErrorvariants to corresponding COM error codes can provide clearer insights into failures.Consider the following adjustment:
fn convert_error(err: MailboxError) -> windows_core::Error { let error_code = match err { MailboxError::Closed => windows::Win32::Foundation::E_ABORT, MailboxError::Timeout => windows::Win32::Foundation::RPC_E_TIMEOUT, }; windows_core::Error::new( error_code, format!("Failed to send message to client actor: {:?}", err), ) }This approach assigns more precise error codes based on the nature of the
MailboxError.
37-41: Simplify error handling by replacing the macro with a functionThe
convert_error!macro adds complexity without substantial benefit. Using a standard function can improve readability and maintainability.Apply this diff to replace the macro with a function:
-macro_rules! convert_error { - ($err:expr) => { - $err.map_err(convert_error)? - }; -} +fn convert_error_result<T>(result: Result<T, MailboxError>) -> windows_core::Result<T> { + result.map_err(convert_error) +}Update the usage in your code:
- convert_error!(self.send(GetServerGuids(filter)).await) + convert_error_result(self.send(GetServerGuids(filter)).await)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.vscode/settings.json(3 hunks)opc_da/Cargo.toml(1 hunks)opc_da/src/client/actor.rs(1 hunks)opc_da/src/client/com.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🔇 Additional comments (4)
opc_da/src/client/actor.rs (1)
59-61: Ensure COM interactions are correctly managed within the actor
COM objects require careful threading considerations. Since Client is used within an Actix actor, verify that COM is initialized on the actor's thread and that the threading model aligns with COM requirements.
Run the following script to check for COM initialization within the actor:
Ensure that CoInitializeEx is called on the actor's thread before any COM operations.
✅ Verification successful
This will help us understand:
- The context of COM initialization
- The threading model being used
- How the actor implementation relates to COM initialization
This will help us understand:
- How the Client is structured
- Where COM initialization happens in relation to the actor lifecycle
- Any documented threading requirements or COM safety considerations
COM initialization is properly handled with appropriate threading model
The verification shows that COM is correctly managed:
- COM is initialized during
Client::new()withCOINIT_MULTITHREADEDflag - The
Clientstruct is explicitly marked as notSendand notSyncusingPhantomData<*const ()> - COM initialization happens before the actor starts, as
Client::start()callsClient::new() - The actor runs in its own thread with the multithreaded COM apartment model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that COM is initialized in the actor context.
# Search for COM initialization in actor.rs
rg 'CoInitialize(Ex)?' opc_da/src/client/actor.rs
# Search for COM initialization in client module
rg 'CoInitialize(Ex)?' opc_da/src/client/*.rs
Length of output: 286
Script:
#!/bin/bash
# Let's check where CoInitializeEx is called and its context
rg -A 10 'CoInitializeEx' opc_da/src/client/com.rs
# Also check if there's any COM thread model specification
rg 'COINIT_' opc_da/src/client/*.rs
# Let's also see how the COM initialization is related to the actor
ast-grep --pattern 'impl Actor for Client'
Length of output: 1111
Script:
#!/bin/bash
# Let's check the Client struct implementation and initialization
ast-grep --pattern 'struct Client'
# Check if there's any COM initialization in the started method or constructor
rg -B 5 -A 10 'fn started|impl Client' opc_da/src/client/*.rs
# Look for any COM safety documentation or threading requirements
rg -l 'thread|COM|com' opc_da/src/client/*.rs
Length of output: 3639
opc_da/src/client/mod.rs (1)
66-70:
Avoid calling CoUninitialize multiple times in Drop implementation
Calling Self::uninitialize() in the Drop method may lead to undefined behavior if CoInitializeEx was not called the same number of times. Since COM initialization is per-thread and may be called only once, unconditionally uninitializing can cause issues.
Apply this diff to remove the Drop implementation:
-impl Drop for Client {
- fn drop(&mut self) {
- Self::uninitialize();
- }
-}Consider managing COM initialization and uninitialization at the application level, ensuring CoUninitialize is called exactly once when no longer needed.
opc_da/Cargo.toml (1)
22-22: Verify compatibility between actix and tokio dependencies
Adding actix = "0.13.5" introduces a dependency that relies on tokio. Ensure that the versions of actix and tokio are compatible to prevent runtime conflicts or unexpected behavior.
Run the following script to check for potential version mismatches:
Ensure that tokio is not pulled in with conflicting versions by different dependencies.
✅ Verification successful
Actix and Tokio versions are compatible
The codebase is using Tokio 1.41.1 with Actix 0.13.5, which are compatible versions. Actix 0.13.x is designed to work with Tokio 1.x series. Additionally, the workspace is using resolver = "2" which helps prevent dependency conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `actix` and `tokio` dependencies are compatible.
# Check `actix` dependency version
cargo tree | grep "actix v"
# Check `tokio` dependency versions
cargo tree | grep "tokio v"
# Look for multiple versions of `tokio` in the dependency graph
cargo tree | grep -E "tokio v[0-9\.]+" | sort | uniq -c
Length of output: 406
Script:
#!/bin/bash
# Let's check the dependencies directly from Cargo.toml files
# First, find all Cargo.toml files
fd "Cargo.toml" --type f --exec cat {} \; | grep -A 5 -B 5 "tokio\|actix"
# Also check if there are any workspace Cargo.toml files
fd "Cargo.toml" --type f --max-depth 1 --exec cat {}
Length of output: 668
opc_da/src/client/com.rs (1)
14-21:
Ensure COM initialization is called only once per thread
Calling CoInitializeEx multiple times on the same thread without matching CoUninitialize calls can lead to errors. Implement a mechanism to initialize COM only once per thread.
Consider using thread-local storage to track initialization:
pub(crate) fn initialize() -> windows_core::HRESULT {
use std::cell::Cell;
thread_local! {
static COM_INIT: Cell<bool> = Cell::new(false);
}
COM_INIT.with(|com_init| {
if !com_init.get() {
let hr = unsafe {
windows::Win32::System::Com::CoInitializeEx(
None,
windows::Win32::System::Com::COINIT_MULTITHREADED,
)
};
if hr.is_ok() {
com_init.set(true);
}
hr
} else {
windows_core::HRESULT(0) // S_OK
}
})
}This ensures COM is initialized only once per thread, preventing initialization errors.
…settings to ignore additional paths
Ronbb
changed the title
coderabbitai
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
opc_da/src/client/actor.rs (4)
5-11: Consider making thestartedmethod asynchronousCurrently, the
startedmethod does not perform any asynchronous operations. If future enhancements require asynchronous initialization, you might consider defining it as an asynchronous function to accommodate those needs.
15-19: Ensure proper handling when starting the actorUsing
Actor::start(value)directly starts the actor but may bypass any error handling or initialization you might need. Consider whether usingActor::createor incorporating error handling during the actor's startup process would be beneficial.
21-28: Use ofDerefmay lead to unintended consequencesImplementing
DerefforClientAsyncexposes all methods ofAddr<Client>, which might make the API less clear and could lead to unintended usage. It might be better to provide explicit methods for the functionality you intend to expose.
30-35: Use more specific error codes for clarityThe
convert_errorfunction usesE_FAIL, a generic failure code. Using a more specific error code relevant to the context can improve error handling and make debugging easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
opc_da/src/client/actor.rs(1 hunks)
🔇 Additional comments (2)
opc_da/src/client/actor.rs (2)
47-54: Handle potential actor communication failures gracefully
When sending a message to the actor with self.send(GetServerGuids(filter)).await, there's a possibility of failure if the actor is unavailable or has stopped. Ensure that such cases are appropriately handled to prevent unexpected crashes.
59-73:
Review the safety of unsafe blocks
The unsafe blocks at lines 63 and 68 are interacting with FFI functions and converting pointers to strings. Verify that these operations are safe and handle any potential null pointers or invalid data to prevent undefined behavior.
…ng unused files in Client
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
opc_da/src/client/com/server.rs (3)
28-29: UsePCWSTRinstead ofPWSTRfor constant wide stringsSince
group_nameis a read-only string passed to theAddGroupmethod, consider usingPCWSTRinstead ofPWSTRto represent a pointer to a constant wide string. This enhances clarity and correctly represents the intended usage.Apply this diff to use
PCWSTR:- let group_name = windows_core::PWSTR::from_raw(group_name.as_mut_ptr()); + let group_name = windows_core::PCWSTR::from_raw(group_name.as_ptr());
39-40: Pass optional pointers correctly inAddGroupcallThe parameters
pTimeBiasandpPercentDeadbandin theAddGroupmethod are optional and can be set toNULLif not used. If you do not intend to specify these values, passingstd::ptr::null()clarifies that they are intentionally omitted.Consider passing
std::ptr::null()for optional parameters:- &time_bias, - &percent_deadband, + std::ptr::null(), + std::ptr::null(),
32-46: Minimize the scope of unsafe codeTo enhance safety and readability, consider minimizing the scope of the
unsafeblock by limiting it to the direct call that requires it. This makes it clear which parts of the code are unsafe and why.Refactor the code to limit the
unsafeblock:// Prepare parameters // ... // Perform the unsafe call let hr = unsafe { self.raw.AddGroup( group_name, windows::Win32::Foundation::BOOL::from(active), update_rate, client_handle, &time_bias, &percent_deadband, locale_id, &mut group_server_handle, &mut revised_update_rate, &opc_da_bindings::IOPCItemMgt::IID, &mut group, ) }; // Check the result safely hr?; match group { None => Err(windows_core::Error::new( windows::Win32::Foundation::E_POINTER, "Failed to add group, returned null", )), Some(group) => Ok(group.cast()?), }opc_da/src/client/actor/client.rs (1)
25-31: Consider AvoidingDerefImplementation for ClarityImplementing
DerefforClientActorto dereference toAddr<Client>may lead to confusion, as it's not immediately clear whatClientActordereferences to. Consider providing explicit methods to access the innerAddr<Client>instead, which can improve code readability and maintainability.opc_da/src/client/com/client.rs (1)
10-15: Handle Potential Re-initialization of COMThe
Client::new()method callsSelf::initialize(), which initializes COM for the current thread. IfClient::new()is called multiple times on the same thread,CoInitializeExmay returnS_FALSE, indicating that COM was already initialized. Ensure that this scenario is handled correctly, and consider tracking initialization to prevent unintended behavior.opc_da/src/client/actor/mod.rs (1)
7-12: Provide More Specific Error Codes inconvert_errorThe
convert_errorfunction maps allactix::MailboxErrorinstances to a genericE_FAILHRESULT. Consider mapping different variants ofMailboxErrorto more specificHRESULTvalues to improve error diagnostics and handling.opc_da/src/client/actor/server.rs (1)
22-28: Consider AvoidingDerefImplementation for ClaritySimilar to
ClientActor, implementingDerefforServerActormay cause confusion. Providing explicit methods to access the innerAddr<Server>can make the code more understandable and maintainable.opc_da/src/client/com/def.rs (3)
11-17: Add documentation for the GUID mapping.While the implementation is correct, consider adding documentation explaining the significance of these GUIDs in the OPC DA context and their role in version identification.
impl ServerVersion { + /// Returns the Component Category ID (CATID) GUID associated with this OPC DA server version. + /// These GUIDs are standardized identifiers used for COM component categorization + /// and are essential for server enumeration and compatibility checking. pub fn to_guid(&self) -> windows_core::GUID {
20-24: Add documentation and consider using HashSet for versions.The struct lacks documentation explaining its purpose and the distinction between
available_versionsandrequires_versions. Additionally, since version uniqueness is important (as evidenced by thewith_versionsmethod), consider usingHashSetinstead ofVecfor these fields.#[derive(Debug, Clone)] +/// Represents filtering criteria for OPC DA server versions. +/// Used during server enumeration to filter servers based on their supported versions. pub struct ServerFilter { - pub(super) available_versions: Vec<ServerVersion>, - pub(super) requires_versions: Vec<ServerVersion>, + /// Versions that are acceptable for the server to support + pub(super) available_versions: std::collections::HashSet<ServerVersion>, + /// Versions that the server must support (subset of available_versions) + pub(super) requires_versions: std::collections::HashSet<ServerVersion>, }
26-41: Optimize default implementation to reduce code duplication.The current implementation duplicates the version list. Consider creating a const array or helper function to avoid duplication and improve maintainability.
+const ALL_VERSIONS: [ServerVersion; 3] = [ + ServerVersion::Version10, + ServerVersion::Version20, + ServerVersion::Version30, +]; + impl Default for ServerFilter { fn default() -> Self { Self { - available_versions: vec![ - ServerVersion::Version10, - ServerVersion::Version20, - ServerVersion::Version30, - ], - requires_versions: vec![ - ServerVersion::Version10, - ServerVersion::Version20, - ServerVersion::Version30, - ], + available_versions: ALL_VERSIONS.into_iter().collect(), + requires_versions: ALL_VERSIONS.into_iter().collect(), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
opc_da/src/client/actor/client.rs(1 hunks)opc_da/src/client/actor/mod.rs(1 hunks)opc_da/src/client/actor/server.rs(1 hunks)opc_da/src/client/com/client.rs(1 hunks)opc_da/src/client/com/def.rs(1 hunks)opc_da/src/client/com/iter.rs(1 hunks)opc_da/src/client/com/mod.rs(1 hunks)opc_da/src/client/com/server.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)opc_da/src/lib.rs(0 hunks)opc_da/src/server/com/group.rs(1 hunks)opc_da/src/server/com/mod.rs(0 hunks)opc_da/src/server/com/server.rs(1 hunks)opc_da/src/server/mod.rs(1 hunks)
💤 Files with no reviewable changes (2)
- opc_da/src/server/com/mod.rs
- opc_da/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
- opc_da/src/server/mod.rs
- opc_da/src/server/com/group.rs
- opc_da/src/server/com/server.rs
- opc_da/src/client/com/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- opc_da/src/client/mod.rs
🔇 Additional comments (3)
opc_da/src/client/actor/client.rs (1)
54-58: Ensure Safety of unsafe Blocks When Using COM Functions
The unsafe blocks around ProgIDFromCLSID and name.to_string() require careful handling to maintain safety guarantees. Verify that all pointers and references used are valid, and that any COM requirements are met.
Please confirm that the usage of ProgIDFromCLSID and BSTR to string conversion are correctly handled and that all necessary checks are in place.
✅ Verification successful
Let me analyze the code further to verify the safety of COM function usage.
COM String Conversions Are Properly Handled
The code correctly handles COM string conversions with appropriate error handling:
ProgIDFromCLSIDis wrapped inunsafeand its errors are properly mapped usingmap_err- The BSTR to string conversion is properly handled with the
?operator to propagate any conversion errors - The pattern is consistent with other BSTR conversions in the codebase, like in the variant handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure proper handling of `ProgIDFromCLSID` and `BSTR` conversion.
# Search for all usages of `ProgIDFromCLSID` and verify proper error handling.
rg 'ProgIDFromCLSID' -A 3
# Check for safe conversions from `BSTR` to `String`.
rg 'to_string\(\)' -A 2
Length of output: 5548
Script:
#!/bin/bash
# Search for the context around the unsafe block in client.rs
rg -B 5 -A 5 'unsafe \{ name\.to_string\(\) \}\?' opc_da/src/client/actor/client.rs
# Look for BSTR and string conversion patterns
ast-grep --pattern 'unsafe { $$.to_string() }'
# Check for any error handling patterns around BSTR conversions
rg -B 3 -A 3 'BSTR.*to_string'
Length of output: 1260
opc_da/src/client/com/def.rs (2)
3-8: LGTM! Well-structured enum with appropriate trait derivations.
The ServerVersion enum correctly represents the three OPC DA versions with proper trait implementations for comparison, copying, and hashing.
1-1: Verify COM interface safety and security implications.
Since this code interfaces with COM components, ensure:
- All COM interface usage follows proper initialization/cleanup patterns
- Error handling for COM operations is robust
- Security implications of exposing COM functionality are considered
Consider documenting COM safety requirements and adding a section about security considerations in the module documentation.
✅ Verification successful
COM interface usage is properly implemented with safe initialization and cleanup
The codebase demonstrates robust COM safety patterns:
-
Proper COM initialization/cleanup:
CoInitializeExis called during client creation withCOINIT_MULTITHREADEDCoUninitializeis automatically called viaDropimplementation- Cleanup is guaranteed through RAII pattern
-
Error handling:
- All COM operations return
windows_core::Result - Error codes and messages are properly propagated
- Client code properly handles COM errors through Result types
- All COM operations return
-
Safe COM interface usage:
- COM interfaces are properly wrapped in safe Rust types
- Memory management follows COM conventions
- Interface lifetimes are properly managed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for COM-related error handling patterns
rg -A 5 "windows_core::(Error|HRESULT|Interface)"
# Look for COM cleanup patterns
ast-grep --pattern 'impl Drop for $_ {
$$$
}'
Length of output: 117473
Script:
#!/bin/bash
# Look for Drop implementations and COM cleanup patterns
ast-grep --pattern 'impl Drop for $_ {
fn drop(&mut self) {
$$$
}
}'
# Look for COM initialization and cleanup
rg -A 5 "CoInitialize|CoUninitialize"
# Look for error handling patterns in client code
rg -A 5 "fn.*Result" opc_da/src/client/
Length of output: 4239
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
opc_da/src/client/com/server.rs (3)
7-13: Add safety documentation for Deref implementationWhile the Deref implementation is necessary for COM interop, it's recommended to document the safety considerations of directly accessing the underlying IOPCServer interface.
Add documentation like:
+/// Provides direct access to the underlying IOPCServer interface. +/// +/// # Safety +/// Care should be taken when directly accessing COM interface methods as they may have +/// specific safety requirements or invariants that must be maintained. impl std::ops::Deref for Server {
49-52: Consider using a more appropriate error code
E_POINTERtypically indicates a NULL pointer dereference, but this case represents a group creation failure. Consider usingE_FAILor a more specific OPC error code.- windows::Win32::Foundation::E_POINTER, + windows::Win32::Foundation::E_FAIL,
17-26: Add comprehensive documentation for the add_group methodThe method lacks documentation explaining its parameters, safety requirements, and potential errors.
Add documentation like:
/// Adds a new group to the OPC server. /// /// # Parameters /// * `name` - The name of the group /// * `active` - Initial active state of the group /// * `client_handle` - Client-supplied handle /// * `update_rate` - Requested update rate in milliseconds /// * `locale_id` - Locale ID for the group /// * `time_bias` - Time bias in minutes from UTC /// * `percent_deadband` - Percent deadband for analog items /// /// # Safety /// This method is safe to call but internally uses COM interface methods. /// /// # Returns /// Returns an `IOPCItemMgt` interface for the created group or an error if creation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
opc_da/src/client/com/server.rs(1 hunks)opc_da/src/server/traits/def.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- opc_da/src/server/traits/def.rs
🔇 Additional comments (2)
opc_da/src/client/com/server.rs (2)
1-5: LGTM! Well-structured wrapper for IOPCServer
The struct design follows best practices for safe COM interface wrapping with appropriate visibility modifiers.
31-32: Incorrect variable name for revised update rate
The variable name issue identified in the previous review remains: revised_percent_deadband should be revised_update_rate as it receives the server's revised update rate.
…ncing type safety and organization
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
opc_da/src/client/traits/server.rs (1)
3-51: Unused Trait MethodsThe methods
serverandadd_groupin theServerTraitare not used anywhere in the codebase.Consider removing these methods if they are not needed, or ensure they are called appropriately to justify their inclusion.
Would you like assistance in identifying possible usage scenarios or refactoring opportunities for these methods?
🧰 Tools
🪛 GitHub Check: clippy
[warning] 3-3:
methodsserverandadd_groupare never used
[warning] 8-8:
methodsserverandadd_groupare never used
[warning] 11-11:
methodsserverandadd_groupare never usedopc_da/src/client/actor/client.rs (2)
24-31: ImplementDerefCarefully to Avoid ConfusionThe
Derefimplementation forClientActormay lead to confusion when accessing the underlyingAddr<Client>. OverusingDereffor types that are not smart pointers can make the code less clear.Consider providing explicit methods to access the inner
Addr<Client>or implementing relevant proxy methods instead of relying onDeref.Apply this diff to remove the
Derefimplementation:-impl std::ops::Deref for ClientActor { - type Target = Addr<Client>; - - fn deref(&self) -> &Self::Target { - &self.0 - } -}And provide explicit accessors if necessary:
impl ClientActor { pub fn addr(&self) -> &Addr<Client> { &self.0 } }
38-43: Error Handling withconvert_error!MacroThe use of
convert_error!macro in theget_serversasynchronous method might obscure the error handling logic.Consider handling the error explicitly to improve code readability and maintainability.
Apply this diff to handle the error without the macro:
pub async fn get_servers( &self, filter: ServerFilter, ) -> windows_core::Result<Vec<(windows_core::GUID, String)>> { - convert_error!(self.send(GetServerGuids(filter)).await) + match self.send(GetServerGuids(filter)).await { + Ok(result) => result, + Err(e) => Err(windows_core::Error::new( + e.as_response_error().error_code(), + "Failed to send GetServerGuids message", + )), + } }opc_da/src/client/com/group.rs (2)
3-17: Remove commented-out code to improve clarityThe block comment from lines 3 to 17 contains commented-out imports. Keeping commented code can clutter the codebase and reduce readability. If this code is no longer needed, consider removing it. If you plan to use these imports later, it's better to document that intent elsewhere.
18-72: Address unused fields inGroupstructStatic analysis indicates that multiple fields in the
Groupstruct are never read. Having unused fields can lead to warnings and may increase the codebase's maintenance complexity.Consider:
- Using the fields: If these fields are intended to be used later, ensure they are accessed appropriately in your code.
- Suppressing warnings: If the fields are necessary but currently unused, you can suppress the warnings by prefixing the fields with an underscore or using
#[allow(dead_code)].- Removing the fields: If the fields are not needed, consider removing them to simplify the struct.
Example of suppressing warnings:
#[allow(dead_code)] pub(crate) item_mgt: opc_da_bindings::IOPCItemMgt,🧰 Tools
🪛 GitHub Check: clippy
[warning] 18-18:
multiple fields are never read
[warning] 22-22:
multiple fields are never read
[warning] 26-26:
multiple fields are never read
[warning] 30-30:
multiple fields are never read
[warning] 34-34:
multiple fields are never read
[warning] 38-38:
multiple fields are never read
[warning] 42-42:
multiple fields are never read
[warning] 46-46:
multiple fields are never read
[warning] 50-50:
multiple fields are never read
[warning] 54-54:
multiple fields are never read
[warning] 58-58:
multiple fields are never read
[warning] 62-62:
multiple fields are never read
[warning] 66-66:
multiple fields are never read
[warning] 71-71:
multiple fields are never readopc_da/src/client/com/mod.rs (1)
1-11: LGTM! Consider adding module documentation.The module structure is well-organized and follows Rust's module system best practices. The re-exports provide a clean public API.
Consider adding module-level documentation comments (
//!) to describe the purpose and contents of each module:+//! COM-related functionality for OPC DA client implementation. + pub mod client; pub mod def; pub mod group; pub mod iter; pub mod server;opc_da/src/client/actor/mod.rs (1)
7-12: Consider using more specific error codes.The error conversion uses the generic
E_FAILcode. Consider mapping specificMailboxErrorcases to more appropriate Windows error codes for better error handling.Example improvement:
fn convert_error(err: actix::MailboxError) -> windows_core::Error { + use actix::MailboxError; + let error_code = match err { + MailboxError::Closed => windows::Win32::Foundation::RPC_E_SERVER_UNAVAILABLE, + MailboxError::Timeout => windows::Win32::Foundation::RPC_E_TIMEOUT, + }; windows_core::Error::new( - windows::Win32::Foundation::E_FAIL, + error_code, format!("Failed to send message to client actor: {:?}", err), ) }opc_da/Cargo.toml (1)
22-22: Consider using caret requirement for actix dependency.While actix 0.13.5 is a stable version, using
^version requirement would allow compatible updates while maintaining API stability.-actix = "0.13.5" +actix = "^0.13.5"opc_da/src/client/actor/server.rs (1)
5-11: Consider documenting the mailbox capacity choiceThe implementation sets a fixed mailbox capacity of 128 messages. While this might be suitable for the current use case, it would be helpful to document why this specific capacity was chosen and what implications it has for message handling.
Consider adding a doc comment explaining the rationale:
impl Actor for Server { type Context = Context<Self>; + /// Sets the actor's mailbox capacity to 128 messages. + /// This limit helps prevent memory issues under high load while allowing + /// sufficient buffering for typical OPC server operations. fn started(&mut self, ctx: &mut Self::Context) { ctx.set_mailbox_capacity(128); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.vscode/settings.json(3 hunks)opc_da/Cargo.toml(1 hunks)opc_da/src/client/actor/client.rs(1 hunks)opc_da/src/client/actor/mod.rs(1 hunks)opc_da/src/client/actor/server.rs(1 hunks)opc_da/src/client/com/client.rs(1 hunks)opc_da/src/client/com/def.rs(1 hunks)opc_da/src/client/com/group.rs(1 hunks)opc_da/src/client/com/iter.rs(1 hunks)opc_da/src/client/com/mod.rs(1 hunks)opc_da/src/client/com/server.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)opc_da/src/client/traits/mod.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/com/client.rs(0 hunks)opc_da/src/lib.rs(0 hunks)opc_da/src/server/com/group.rs(1 hunks)opc_da/src/server/com/item.rs(1 hunks)opc_da/src/server/com/mod.rs(0 hunks)opc_da/src/server/com/server.rs(1 hunks)opc_da/src/server/mod.rs(1 hunks)opc_da/src/server/traits/def.rs(1 hunks)
💤 Files with no reviewable changes (3)
- opc_da/src/server/com/mod.rs
- opc_da/src/lib.rs
- opc_da/src/com/client.rs
🧰 Additional context used
🪛 GitHub Check: clippy
opc_da/src/client/com/group.rs
[warning] 18-18:
multiple fields are never read
[warning] 22-22:
multiple fields are never read
[warning] 26-26:
multiple fields are never read
[warning] 30-30:
multiple fields are never read
[warning] 34-34:
multiple fields are never read
[warning] 38-38:
multiple fields are never read
[warning] 42-42:
multiple fields are never read
[warning] 46-46:
multiple fields are never read
[warning] 50-50:
multiple fields are never read
[warning] 54-54:
multiple fields are never read
[warning] 58-58:
multiple fields are never read
[warning] 62-62:
multiple fields are never read
[warning] 66-66:
multiple fields are never read
[warning] 71-71:
multiple fields are never read
opc_da/src/client/com/server.rs
[warning] 17-17:
multiple fields are never read
[warning] 25-25:
multiple fields are never read
[warning] 29-29:
multiple fields are never read
[warning] 34-34:
multiple fields are never read
[warning] 38-38:
multiple fields are never read
[warning] 42-42:
multiple fields are never read
[warning] 46-46:
multiple fields are never read
[warning] 50-50:
multiple fields are never read
opc_da/src/client/traits/server.rs
[warning] 3-3:
methods server and add_group are never used
[warning] 8-8:
methods server and add_group are never used
[warning] 11-11:
methods server and add_group are never used
🔇 Additional comments (16)
opc_da/src/client/com/iter.rs (1)
27-49:
Potential Out-of-Bounds Access in next Method
The implementation of the next method may lead to an out-of-bounds access when self.count is zero. Decrementing self.count before indexing self.cache can cause an underflow, resulting in indexing with usize::MAX.
Apply this diff to prevent the out-of-bounds access:
fn next(&mut self) -> Option<Self::Item> {
if self.count == 0 {
// Existing logic to refill the cache
// ...
}
+ if self.count == 0 {
+ return None;
+ }
self.count -= 1;
Some(Ok(self.cache[self.count as usize]))
}This check ensures that if self.count is zero after attempting to refill the cache, the method returns None without attempting to index the cache.
opc_da/src/client/com/server.rs (2)
23-25:
Correct Variable Name for Revised Update Rate
The variable revised_percent_deadband appears to be misnamed. According to the OPC DA AddGroup method, the parameter at this position should be pRevisedUpdateRate, representing the server's revised update rate.
Apply this diff to rename the variable:
let mut revised_percent_deadband = 0;
+let mut revised_update_rate = 0u32;And update its usage in the function call:
self.server().AddGroup(
group_name,
windows::Win32::Foundation::BOOL::from(active),
update_rate,
client_handle,
&time_bias,
&percent_deadband,
locale_id,
- &mut group_server_handle,
- &mut revised_percent_deadband,
+ &mut revised_update_rate,
&opc_da_bindings::IOPCItemMgt::IID,
&mut group,
)?;🧰 Tools
🪛 GitHub Check: clippy
[warning] 25-25:
multiple fields are never read
23-25:
Potential Use-After-Free Risk with group_name
The group_name PWSTR is created from a mutable vector that may be dropped while the pointer is still in use, leading to a potential use-after-free error.
Extend the lifetime of the vector until after the AddGroup call:
let mut group_name_vec = name.encode_utf16().chain(Some(0)).collect::<Vec<_>>();
let group_name = windows_core::PWSTR::from_raw(group_name_vec.as_mut_ptr());
unsafe {
self.server().AddGroup(
group_name,
// ... other parameters ...
)?;
}
+// Ensure `group_name_vec` is not dropped before this point
+std::mem::drop(group_name_vec);🧰 Tools
🪛 GitHub Check: clippy
[warning] 25-25:
multiple fields are never read
opc_da/src/client/com/client.rs (1)
20-24:
Ensure Safe COM Uninitialization
Calling CoUninitialize in the Drop implementation may not be safe if COM is initialized multiple times on the same thread (incremented apartment counts). Uninitializing COM might interfere with other COM operations.
Consider tracking the initialization count or ensuring that CoUninitialize is called only when appropriate.
Run the following script to check for multiple initializations of COM in the codebase:
#!/bin/bash
# Description: Find all calls to CoInitializeEx to verify COM initialization patterns.
# Search for CoInitializeEx calls
rg 'CoInitializeEx' --type rust
# Search for CoUninitialize calls
rg 'CoUninitialize' --type rustThis script will help identify if there are multiple initializations that could lead to improper uninitialization.
opc_da/src/server/mod.rs (1)
1-2: Modules com and traits added successfully
The new modules com and traits are correctly added to the server module. This enhances the modularity and organization of the server-side code.
opc_da/src/client/traits/mod.rs (1)
1-3: Introduction of server module in traits
The server module is properly added under the traits module, and its public items are re-exported. This setup promotes better code organization and accessibility.
opc_da/src/client/mod.rs (1)
1-9: Client module restructured with new submodules
The addition of the actor, com, traits, and versioned modules (v1, v2, v3), along with public re-exports, improves the structure and organization of the client module. This change enhances modularity and readability.
opc_da/src/client/actor/mod.rs (1)
14-19: LGTM! Well-implemented macro.
The macro is properly exported, uses correct hygiene with $crate, and provides a clean error conversion interface.
opc_da/src/client/actor/server.rs (1)
21-28: LGTM: Clean Deref implementation
The Deref implementation provides a clean way to access the inner actor address. The implementation is correct and well-documented with a clear comment.
.vscode/settings.json (1)
4-5: LGTM: Comprehensive spell-checking configuration
The updates to the spell checker configuration appropriately cover the new technical terms introduced by the OPC DA client implementation and Actix framework integration.
Also applies to: 12-12, 28-28, 68-68, 74-75
opc_da/src/client/com/def.rs (2)
3-18: LGTM: Well-structured version enumeration
The ServerVersion enum is well-designed with appropriate derive macros and a clean GUID mapping implementation.
43-69:
Improve version handling consistency
This is a duplicate of a previous review comment. The methods still handle versions inconsistently and lack validation for version relationships.
The implementation should:
- Use HashSet consistently across all methods
- Validate that required versions are a subset of available versions
- Add documentation for each method
Apply the previously suggested fixes to improve consistency and add validation.
opc_da/src/server/traits/def.rs (1)
Line range hint 1-5: LGTM: Import path updated to reflect new module structure
The change correctly updates the import path to use the new module structure while maintaining the same imported types.
opc_da/src/server/com/server.rs (1)
1-1: LGTM: Import path aligned with new module structure
The import path has been correctly updated to reflect the new module organization.
opc_da/src/server/com/group.rs (1)
3-6: LGTM: Imports organized under server namespace
The imports have been correctly reorganized under the server namespace, improving code organization while maintaining the same functionality.
opc_da/src/server/com/item.rs (1)
1-1: Verify if this file needs updates to align with client changes
While this file has no changes, given the significant client-side COM functionality updates in this PR, please verify if the current implementation in item.rs aligns with and supports the new client features, particularly around "UID iteration" mentioned in the PR description.
Let's analyze the relationship between this file and the new client functionality:
… and group state management, and update imports for improved organization
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
opc_da/src/client/traits/group_state_mgt.rs (1)
5-80: Add documentation comments to public trait methodsAdding Rust documentation comments (
///) to the methods ofGroupStateMgtTraitwill improve code readability and provide useful information to users of the API. This aligns with Rust's best practices for public interfaces.opc_da/src/client/traits/server.rs (2)
52-61: Ensure safe handling of server status pointerIn the
get_statusmethod, you dereference a raw pointer obtained fromGetStatus(). To ensure safety, consider using theOption::ok_ormethod to handle the possibility of a null pointer.Apply this diff for safer error handling:
- let status = unsafe { self.server().GetStatus()?.as_ref() }; - match status { - Some(status) => Ok(*status), - None => Err(windows_core::Error::new( - windows::Win32::Foundation::E_FAIL, - "Failed to get server status", - )), - } + let status_ptr = unsafe { self.server().GetStatus()? }; + let status = unsafe { status_ptr.as_ref() } + .ok_or_else(|| windows_core::Error::new( + windows::Win32::Foundation::E_FAIL, + "Failed to get server status", + ))?; + Ok(*status)
3-82: Add documentation comments to public trait methodsAdding Rust documentation comments (
///) to the methods ofServerTraitwill enhance clarity and maintainability, providing valuable context and usage examples for developers.opc_da/src/client/memory.rs (1)
5-126: ImplementDebugandClonetraits for memory structuresImplementing the
DebugandClonetraits forArray<T>,Pointer<T>, andRef<T>can enhance usability, especially during development and testing.opc_da/src/client/mod.rs (1)
9-10: Consider explicit exports instead of wildcardsUsing
pub use *can accidentally expose internal implementation details. Consider explicitly listing the items you want to export:-pub use actor::*; -pub use com::*; +pub use actor::{ClientActor, /* other specific items */}; +pub use com::{Client, /* other specific items */};opc_da/src/client/traits/mod.rs (1)
6-9: Consider explicit trait exports for better API controlSimilar to the main module, using wildcard exports can lead to unintended trait exposure. Consider explicitly listing the traits:
-pub use group_state_mgt::*; -pub use group_state_mgt2::*; -pub use item_mgt::*; -pub use server::*; +pub use group_state_mgt::GroupStateMgtTrait; +pub use group_state_mgt2::GroupStateMgt2Trait; +pub use item_mgt::ItemMgtTrait; +pub use server::ServerTrait;opc_da/src/client/traits/group_state_mgt2.rs (2)
3-4: Add documentation for the trait and methodsConsider adding documentation to explain:
- Purpose of the trait
- Relationship with GroupStateMgtTrait
- Expected usage
- Safety requirements for implementing types
Example:
/// Extends group state management with OPC DA 2.0 specific functionality. /// /// # Safety /// Implementing types must ensure the underlying `IOPCGroupStateMgt2` /// interface is valid for the lifetime of the object. pub trait GroupStateMgt2Trait: GroupStateMgtTrait { /// Returns a reference to the underlying COM interface. fn group_state_mgt2(&self) -> &opc_da_bindings::IOPCGroupStateMgt2;
6-12: Document unsafe block requirementsThe unsafe blocks calling COM methods should have comments explaining their safety requirements:
fn set_keep_alive(&self, keep_alive_time: u32) -> windows_core::Result<u32> { + // SAFETY: The COM interface is guaranteed to be valid by the trait contract unsafe { self.group_state_mgt2().SetKeepAlive(keep_alive_time) } } fn get_keep_alive(&self) -> windows_core::Result<u32> { + // SAFETY: The COM interface is guaranteed to be valid by the trait contract unsafe { self.group_state_mgt2().GetKeepAlive() } }opc_da/src/def.rs (2)
49-55: Consider optimizing version filtering.The
with_versionsmethod could be more efficient by pre-allocating the HashSet capacity.pub fn with_versions<I: IntoIterator<Item = ServerVersion>>(mut self, versions: I) -> Self { - self.available_versions = versions.into_iter().collect(); + let versions_iter = versions.into_iter(); + let (size_hint_lower, _) = versions_iter.size_hint(); + self.available_versions = std::collections::HashSet::with_capacity(size_hint_lower); + self.available_versions.extend(versions_iter); self.requires_versions .retain(|v| self.available_versions.contains(v)); self }
63-73: Add documentation for GroupState fields.The struct would benefit from documentation explaining the purpose and valid ranges of each field, especially for fields like
time_bias,percent_deadband, andlocale_id.opc_da/src/client/com/group.rs (1)
5-19: Remove or utilize the unused interface list comment block.The block comment at the top lists interfaces that are already documented in the struct fields. Consider removing it or converting it into proper documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
opc_da/src/client/actor/client.rs(1 hunks)opc_da/src/client/com/client.rs(1 hunks)opc_da/src/client/com/group.rs(1 hunks)opc_da/src/client/com/mod.rs(1 hunks)opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)opc_da/src/client/traits/group_state_mgt.rs(1 hunks)opc_da/src/client/traits/group_state_mgt2.rs(1 hunks)opc_da/src/client/traits/item_mgt.rs(1 hunks)opc_da/src/client/traits/mod.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/def.rs(1 hunks)opc_da/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- opc_da/src/lib.rs
- opc_da/src/client/com/mod.rs
- opc_da/src/client/actor/client.rs
- opc_da/src/client/com/client.rs
🔇 Additional comments (7)
opc_da/src/client/traits/item_mgt.rs (1)
5-142: Well-implemented item management trait
The ItemMgtTrait methods are well-structured with appropriate error handling and make good use of the memory module for managing pointers and arrays. The use of unsafe blocks is justified and properly encapsulated.
opc_da/src/client/mod.rs (1)
1-7: LGTM! Well-organized module structure
The module organization follows good separation of concerns, with clear separation between:
- Actor system implementation (actor)
- COM interface handling (com)
- Memory management (memory)
- Trait definitions (traits)
- Version-specific implementations (v1, v2, v3)
opc_da/src/client/traits/mod.rs (1)
1-4: LGTM! Good trait organization by functionality
The traits are well-organized into logical groups:
- Group state management (basic and v2)
- Item management
- Server operations
opc_da/src/def.rs (2)
3-24: LGTM! Well-structured version management implementation.
The ServerVersion enum and its implementation provide a clean and type-safe way to handle OPC DA server versions and their corresponding GUIDs.
26-39: LGTM! Clean filter implementation with appropriate visibility.
The ServerFilter struct and its Default implementation provide a robust way to manage server version filtering.
opc_da/src/client/com/group.rs (2)
76-116: LGTM! Well-implemented COM interface conversion.
The implementation correctly handles both required and optional interfaces, with properly specified target types in all cast() calls as previously suggested.
118-128: LGTM! Clean trait implementations.
The trait implementations are concise and correctly provide access to the required interfaces.
…ce methods to use windows::core for improved consistency
… add empty checks for server_handles in various methods to improve error handling
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (21)
opc_da/src/client/traits/sync_io2.rs (3)
7-7: Document the reason for allowing type complexity.Add a doc comment explaining why this complexity is necessary for OPC DA operations.
+/// Complex return type is required to match the OPC DA specification for ReadMaxAge #[allow(clippy::type_complexity)]
32-42: Document unsafe block usage.Add a safety comment explaining why this unsafe block is necessary and what invariants must be maintained.
+ // SAFETY: The following invariants are guaranteed: + // - All pointers are valid as they come from slice::as_ptr() or RemoteArray::as_mut_ptr() + // - The length has been validated to be within u32 bounds + // - Memory is properly aligned as guaranteed by RemoteArray unsafe {
63-70: Document unsafe block usage.Add a safety comment explaining why this unsafe block is necessary and what invariants must be maintained.
+ // SAFETY: The following invariants are guaranteed: + // - All pointers are valid as they come from slice::as_ptr() or RemoteArray::as_mut_ptr() + // - The length has been validated to be within u32 bounds + // - Memory is properly aligned as guaranteed by RemoteArray unsafe {opc_da/src/client/unified/group.rs (1)
10-24: Consider removing or explaining the commented-out code blockThe large block of commented-out code from lines 10 to 24 may not be necessary if it's not used. Removing unused code can improve readability and maintainability.
opc_da/src/server/com/connection_point.rs (1)
Line range hint
40-47: Consider using async/await instead of blocking operationsThe
blocking_write()call in an async context could lead to thread pool starvation. Consider using the async versionwrite().awaitinstead.- fn Advise(&self, sink: Option<&windows::core::IUnknown>) -> windows::core::Result<u32> { + async fn Advise(&self, sink: Option<&windows::core::IUnknown>) -> windows::core::Result<u32> { let cookie = self .next_cookie .fetch_add(1, core::sync::atomic::Ordering::SeqCst); - self.connections - .blocking_write() + self.connections + .write() + .await .insert(cookie, sink.unwrap().clone()); Ok(cookie) }opc_da/src/client/traits/item_io.rs (1)
18-23: Enhance error message with actual lengthsThe error message could be more helpful by including the actual lengths of the arrays.
return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "Invalid arguments - arrays must be non-empty and have same length", + &format!( + "Invalid arguments - arrays must be non-empty and have same length. Got: item_ids={}, max_age={}", + item_ids.len(), + max_age.len() + ), ));opc_da/src/client/traits/server.rs (2)
41-48: Consider using map_or for cleaner error handlingThe match expression could be simplified using
map_orfor better readability.Apply this diff to simplify the error handling:
-match group { - None => Err(windows::core::Error::new( - windows::Win32::Foundation::E_POINTER, - "Failed to add group, returned null", - )), - Some(group) => group.try_into(), -} +group.map_or( + Err(windows::core::Error::new( + windows::Win32::Foundation::E_POINTER, + "Failed to add group, returned null", + )), + |g| g.try_into(), +)
51-60: Improve error handling in get_statusThe status check could be simplified using
ok_or_elsefor more idiomatic Rust.Apply this diff to improve the error handling:
-let status = unsafe { self.interface()?.GetStatus()?.as_ref() }; -match status { - Some(status) => Ok(*status), - None => Err(windows::core::Error::new( - windows::Win32::Foundation::E_FAIL, - "Failed to get server status", - )), -} +unsafe { + self.interface()? + .GetStatus()? + .as_ref() + .ok_or_else(|| { + windows::core::Error::new( + windows::Win32::Foundation::E_FAIL, + "Failed to get server status", + ) + }) + .map(|status| *status) +}opc_da/src/client/unified/iter.rs (1)
7-9: Consider using NonZeroU32 for countUsing
NonZeroU32for the count field could help optimize the struct's memory layout.Apply this diff to optimize memory layout:
- count: u32, + count: std::num::NonZeroU32,Note: This change would require adjusting the count handling logic throughout the implementation.
opc_da/src/client/traits/item_properties.rs (1)
33-37: Consider adding error handling for zero count.The code should handle the case where
countis 0 by returning an appropriate error or empty arrays.if count > 0 { property_ids.set_len(count); descriptions.set_len(count); datatypes.set_len(count); + } else { + return Ok(( + RemoteArray::new(0), + RemoteArray::new(0), + RemoteArray::new(0), + )); }opc_da/src/client/traits/item_sampling_mgt.rs (1)
19-32: Consider adding bounds checking for sampling rates.The
set_item_sampling_ratemethod should validate that sampling rates are within reasonable bounds to prevent potential issues.let len = server_handles.len().try_into()?; + // Validate sampling rates (e.g., minimum 100ms, maximum 1 hour) + const MIN_SAMPLING_RATE: u32 = 100; + const MAX_SAMPLING_RATE: u32 = 3_600_000; + if sampling_rates.iter().any(|&rate| rate < MIN_SAMPLING_RATE || rate > MAX_SAMPLING_RATE) { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + "sampling rates must be between 100ms and 1 hour", + )); + } let mut revised_rates = RemoteArray::new(len);opc_da/src/client/v1/mod.rs (1)
54-61: Consider documenting thread safety requirements.The
Groupstruct contains multiple COM interfaces that may have specific threading requirements. These should be documented.+/// Represents an OPC DA v1 group. +/// +/// # Thread Safety +/// This struct contains COM interfaces that are not thread-safe. +/// All methods should be called from the same thread that created the group. pub struct Group { pub(crate) item_mgt: opc_da_bindings::IOPCItemMgt,opc_da/src/client/v3/mod.rs (1)
9-61: Add documentation for Server struct and its interfaces.Consider adding documentation comments that explain:
- The purpose of each interface
- Version compatibility requirements
- Usage examples
opc_da/src/client/memory.rs (2)
65-69: Consider using RAII pattern for CoTaskMemFreeThe manual memory management with CoTaskMemFree could be wrapped in a RAII type to prevent potential memory leaks in case of panics.
+struct CoTaskMemPtr<T>(*mut T); + +impl<T> Drop for CoTaskMemPtr<T> { + fn drop(&mut self) { + if !self.0.is_null() { + unsafe { CoTaskMemFree(Some(self.0 as _)); } + } + } +} + impl<T: Sized> Drop for RemoteArray<T> { #[inline(always)] fn drop(&mut self) { - if !self.pointer.is_null() { - unsafe { - CoTaskMemFree(Some(self.pointer as _)); - } - } + let _ = CoTaskMemPtr(self.pointer); } }
206-214: Consider consolidating duplicate code in PWSTR/PCWSTR array conversionsThe as_pwstr_array and as_pcwstr_array methods contain similar logic that could be consolidated using a generic function.
+impl LocalPointer<Vec<Vec<u16>>> { + fn as_wstr_array<T, F>(&self, null_fn: F, convert_fn: F) -> Vec<T> + where + F: Fn(*const u16) -> T + { + match &self.inner { + Some(values) => values + .iter() + .map(|value| convert_fn(value.as_ptr())) + .collect(), + None => vec![null_fn(std::ptr::null())], + } + } + + #[inline(always)] + pub fn as_pwstr_array(&self) -> Vec<windows::core::PWSTR> { + self.as_wstr_array( + windows::core::PWSTR::null, + |ptr| unsafe { windows::core::PWSTR::from_raw(ptr as _) } + ) + } + + #[inline(always)] + pub fn as_pcwstr_array(&self) -> Vec<windows::core::PCWSTR> { + self.as_wstr_array( + windows::core::PCWSTR::null, + |ptr| unsafe { windows::core::PCWSTR::from_raw(ptr as _) } + ) + } }Also applies to: 217-225
opc_da/src/server/com/enumeration.rs (1)
Line range hint
134-182: Consider usingcopy_from_slicefor better performance.The array copying operations could be optimized by using
copy_from_sliceinstead of manual iteration where possible.Apply this optimization to improve performance:
-for (i, element) in slice.iter().enumerate() { - elements[i] = element.clone(); -} +elements[..slice.len()].copy_from_slice(slice);opc_da/src/server/traits/group.rs (1)
Line range hint
1-316: Consider adding documentation for all trait methods.While some methods like
data_adviseare well documented, many other methods lack documentation. Consider adding documentation for all trait methods to improve maintainability.Add documentation similar to the
data_advisemethod for other methods, for example:/// Adds items to the group. /// /// # Arguments /// * `items` - The items to add /// * `results` - Buffer to store the results /// * `errors` - Buffer to store any errors /// /// # Returns /// * `Ok(())` if successful /// * An error if the operation fails fn add_items( &self, items: &[opc_da_bindings::tagOPCITEMDEF], results: &mut [opc_da_bindings::tagOPCITEMRESULT], errors: &mut [windows::core::HRESULT], ) -> windows::core::Result<()>;opc_da/src/server/traits/def.rs (1)
219-222: Enhance error messages for invalid enum variants.While the error handling is correct, consider making the error messages more specific by including the actual invalid value received. This would make debugging easier.
Example improvement for BrowseFilter:
- _ => Err(windows::core::Error::new( - windows::Win32::Foundation::E_INVALIDARG, - "Invalid BrowseFilter", - )), + value => Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + &format!("Invalid BrowseFilter value: {}", value), + )),Also applies to: 257-260, 275-278, 301-304, 383-386
opc_da/src/client/unified/client.rs (2)
63-70: Refactor to useCoCreateInstanceExinstead ofCoCreateInstanceThe
get_serversmethod contains a TODO to useCoCreateInstanceEx. UsingCoCreateInstanceExallows specifying remote servers and provides more options for object creation, which can enhance flexibility and performance.Would you like assistance in refactoring this code to use
CoCreateInstanceEx?
68-68: Convert filters to appropriate COM class contextThe
CLSCTX_ALLflag is currently used as a placeholder. To better control COM object creation based on the providedfilter, consider converting the filters into appropriate class context flags that match your requirements.opc_da/src/client/traits/common.rs (1)
35-37: Avoid potential panic by handling null pointers inset_client_nameWhen converting the
nameto aLocalPointer, ensure that the pointer is valid before callingSetClientNameto avoid panics if the pointer is null.Consider adding a check:
fn set_client_name(&self, name: &str) -> windows::core::Result<()> { let name = LocalPointer::from(name); + if name.as_pcwstr().is_null() { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_POINTER, + "Client name pointer is null", + )); + } unsafe { self.interface()?.SetClientName(name.as_pcwstr()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
opc_da/Cargo.toml(2 hunks)opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/tests.rs(1 hunks)opc_da/src/client/traits/async_io.rs(1 hunks)opc_da/src/client/traits/async_io2.rs(1 hunks)opc_da/src/client/traits/async_io3.rs(1 hunks)opc_da/src/client/traits/browse.rs(1 hunks)opc_da/src/client/traits/browse_server_address_space.rs(1 hunks)opc_da/src/client/traits/common.rs(1 hunks)opc_da/src/client/traits/connection_point_container.rs(1 hunks)opc_da/src/client/traits/data_object.rs(1 hunks)opc_da/src/client/traits/group_state_mgt.rs(1 hunks)opc_da/src/client/traits/group_state_mgt2.rs(1 hunks)opc_da/src/client/traits/item_deadband_mgt.rs(1 hunks)opc_da/src/client/traits/item_io.rs(1 hunks)opc_da/src/client/traits/item_mgt.rs(1 hunks)opc_da/src/client/traits/item_properties.rs(1 hunks)opc_da/src/client/traits/item_sampling_mgt.rs(1 hunks)opc_da/src/client/traits/public_group_state_mgt.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/traits/server_public_groups.rs(1 hunks)opc_da/src/client/traits/sync_io.rs(1 hunks)opc_da/src/client/traits/sync_io2.rs(1 hunks)opc_da/src/client/unified/actor/client.rs(1 hunks)opc_da/src/client/unified/actor/mod.rs(1 hunks)opc_da/src/client/unified/client.rs(1 hunks)opc_da/src/client/unified/group.rs(1 hunks)opc_da/src/client/unified/iter.rs(1 hunks)opc_da/src/client/unified/server.rs(1 hunks)opc_da/src/client/v1/mod.rs(1 hunks)opc_da/src/client/v2/mod.rs(1 hunks)opc_da/src/client/v3/mod.rs(1 hunks)opc_da/src/def.rs(1 hunks)opc_da/src/server/com/connection_point.rs(3 hunks)opc_da/src/server/com/enumeration.rs(16 hunks)opc_da/src/server/com/group.rs(40 hunks)opc_da/src/server/com/memory.rs(12 hunks)opc_da/src/server/com/server.rs(25 hunks)opc_da/src/server/com/utils.rs(13 hunks)opc_da/src/server/com/variant.rs(4 hunks)opc_da/src/server/traits/def.rs(18 hunks)opc_da/src/server/traits/group.rs(3 hunks)opc_da/src/server/traits/server.rs(12 hunks)opc_da_bindings/Cargo.toml(0 hunks)
💤 Files with no reviewable changes (1)
- opc_da_bindings/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- opc_da/src/client/tests.rs
- opc_da/src/client/traits/public_group_state_mgt.rs
- opc_da/src/client/unified/actor/mod.rs
- opc_da/src/client/traits/group_state_mgt2.rs
- opc_da/Cargo.toml
- opc_da/src/client/traits/async_io3.rs
- opc_da/src/def.rs
- opc_da/src/client/traits/connection_point_container.rs
- opc_da/src/client/traits/server_public_groups.rs
- opc_da/src/client/traits/item_deadband_mgt.rs
- opc_da/src/client/v2/mod.rs
- opc_da/src/server/com/server.rs
- opc_da/src/server/com/group.rs
🧰 Additional context used
📓 Learnings (2)
opc_da/src/client/unified/group.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
opc_da/src/client/memory.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🔇 Additional comments (50)
opc_da/src/client/traits/sync_io2.rs (6)
1-5: LGTM! Clean trait definition with focused imports.
The trait and imports are well-structured for COM interop with OPC DA.
18-23:
Add empty array validation.
While the length comparison is good, we should also check for empty arrays.
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
if server_handles.len() != max_age.len() {25-25:
Add array size bounds checking.
Add validation to ensure array sizes don't exceed u32::MAX.
+ if server_handles.len() > u32::MAX as usize {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "array size exceeds maximum supported length",
+ ));
+ }
let len = server_handles.len().try_into()?;52-57:
Add empty array validation.
Similar to read_max_age, we should check for empty arrays.
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
if server_handles.len() != values.len() {59-59:
Add array size bounds checking.
Add validation to ensure array sizes don't exceed u32::MAX.
+ if server_handles.len() > u32::MAX as usize {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "array size exceeds maximum supported length",
+ ));
+ }
let len = server_handles.len().try_into()?;Likely invalid or redundant comment.
4-74: Verify trait implementation and usage.
Let's verify how this trait is being implemented and used across the codebase.
✅ Verification successful
The verification results show that the SyncIo2Trait is properly implemented and used across the codebase:
Trait implementation and usage verified successfully
- The trait is correctly implemented by two Group types:
opc_da/src/client/v3/mod.rs: Direct implementationopc_da/src/client/unified/group.rs: Implementation with proper error handling for unsupported cases
The methods read_max_age and write_vqt are consistently used in both client and server implementations, following the same parameter patterns and error handling approaches. The trait's design aligns well with both the COM interface requirements and its practical usage in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations and usage of SyncIo2Trait
# Find trait implementations
echo "=== SyncIo2Trait implementations ==="
rg "impl.*SyncIo2Trait.*for" -A 5
# Find trait usage in method calls
echo -e "\n=== SyncIo2Trait method usage ==="
rg "read_max_age|write_vqt" -A 3
Length of output: 5058
opc_da/src/client/unified/group.rs (2)
137-143: Refactor to reduce code duplication in error handling
The error handling code in the interface methods for optional traits is repeated multiple times. Consider refactoring to use a helper function to improve maintainability and readability.
Define a helper function:
fn interface_not_supported(interface_name: &str) -> windows::core::Error {
windows::core::Error::new(
windows::Win32::Foundation::E_NOTIMPL,
format!("{} not supported", interface_name),
)
}Then update your interface methods to use this helper function. For example:
- self.group_state_mgt2.as_ref().ok_or_else(|| {
- windows::core::Error::new(
- windows::Win32::Foundation::E_NOTIMPL,
- "IOPCGroupStateMgt2 not supported",
- )
- })
+ self.group_state_mgt2.as_ref().ok_or_else(|| interface_not_supported("IOPCGroupStateMgt2"))Apply this pattern to the following interfaces:
public_group_state_mgt(lines 148-154)sync_io2(lines 165-171)async_io(lines 177-183)async_io2(lines 187-193)async_io3(lines 198-204)item_deadband_mgt(lines 209-215)item_sampling_mgt(lines 220-226)connection_point_container(lines 233-239)data_object(lines 244-250)
Also applies to: 148-154, 165-171, 177-183, 187-193, 198-204, 209-215, 220-226, 233-239, 244-250
81-121: Well-structured implementation of TryFrom for Group
The TryFrom implementation correctly casts the required interfaces and gracefully handles optional ones. This approach ensures that all necessary interfaces are available while accommodating optional features.
opc_da/src/client/traits/sync_io.rs (2)
10-16: Add length check for empty server_handles in read method
The read method should ensure that server_handles is not empty before proceeding. Passing an empty slice to the OPC server may result in unexpected behavior or errors.
Apply this diff to add the check:
fn read(
&self,
source: opc_da_bindings::tagOPCDATASOURCE,
server_handles: &[u32],
) -> windows::core::Result<(
RemoteArray<opc_da_bindings::tagOPCITEMSTATE>,
RemoteArray<windows::core::HRESULT>,
)> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;35-43: Add length check for empty server_handles in write method
Similarly, the write method should check that server_handles is not empty before proceeding. An empty server_handles slice may lead to a no-op or unexpected behavior from the OPC server.
Apply this diff to add the check:
fn write(
&self,
server_handles: &[u32],
values: &[VARIANT],
) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
if server_handles.len() != values.len() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"server_handles and values must have the same length",
));opc_da/src/client/unified/actor/client.rs (2)
5-11: Critical Issue: Client Struct May Not Be Send but Is Used as an Actor
The Client struct is being used as an Actor in actix. Actors in actix are required to be Send because they may run in different threads. If Client is not Send, this can lead to runtime errors or undefined behavior.
16-19: Clean Initialization of ClientActor
The ClientActor::new() method correctly initializes the Client actor and starts it, encapsulating it within the ClientActor struct. This provides a clean and efficient way to manage the actor's lifecycle.
opc_da/src/client/traits/async_io.rs (2)
12-28: Add Check for Empty server_handles in read Method
The read method should verify that server_handles is not empty before proceeding. An empty server_handles can lead to unexpected behavior or errors from the OPC server.
36-51: Validate Lengths of server_handles and values in write Method
In the write method, ensure that the lengths of server_handles and values are equal. This validation prevents mismatches between handles and values, which could result in incorrect data being written.
opc_da/src/client/traits/data_object.rs (2)
16-18: Incorrect Return Type in query_get_data Method
The use of .ok() converts the Result into an Option, which doesn't match the expected return type windows::core::Result<()>. Remove .ok() to return the correct result type and properly propagate errors.
20-29: Incorrect Error Handling in get_canonical_format Method
Using .ok()? in the get_canonical_format method may suppress errors by converting them to Option::None. Since the method expects a windows::core::Result<FORMATETC>, you should return the result directly to properly handle errors.
opc_da/src/client/traits/async_io2.rs (2)
12-26: Add Validation for Empty server_handles in read Method
Before initiating a read operation, check that server_handles is not empty. An empty list can cause the OPC server to return errors or behave unexpectedly.
34-49: Ensure Matching Lengths of server_handles and values in write Method
In the write method, validate that server_handles and values have the same length. This ensures that each server handle corresponds to a value, preventing data mismatch issues.
opc_da/src/server/com/connection_point.rs (1)
Line range hint 57-65: Potential memory leak in EnumConnections
The ManuallyDrop::new(Some(v.clone())) creates a new reference to the COM object, but it's not clear if this is properly cleaned up when the CONNECTDATA struct is dropped.
Consider implementing proper cleanup or using windows::core::ComPtr instead.
opc_da/src/client/traits/group_state_mgt.rs (2)
68-69: Missing error propagation in set_name method
The unsafe call to SetName does not handle potential errors.
- unsafe { self.interface()?.SetName(name.as_pwstr()) }
+ unsafe { self.interface()?.SetName(name.as_pwstr())? }78-79: Missing error propagation in clone_group method
The unsafe call to CloneGroup does not handle potential errors.
- unsafe { self.interface()?.CloneGroup(name.as_pwstr(), id) }
+ unsafe { self.interface()?.CloneGroup(name.as_pwstr(), id)? }opc_da/src/client/traits/server.rs (1)
23-23:
Fix potential memory safety issue with PWSTR::from_raw
Using PWSTR::from_raw(group_name.as_mut_ptr()) is unsafe as it takes ownership of the raw pointer while group_name is still in scope. This could lead to a double free.
Apply this diff to fix the memory safety issue:
-let group_name = windows::core::PWSTR::from_raw(group_name.as_mut_ptr());
+let group_name = windows::core::PWSTR(group_name.as_mut_ptr());Likely invalid or redundant comment.
opc_da/src/client/unified/iter.rs (1)
47-49: Add bounds check before accessing cache array
The array access could panic if count becomes invalid.
This issue was previously identified in a past review. The same concerns about potential out-of-bounds access apply here.
opc_da/src/client/traits/item_properties.rs (2)
83-106: Duplicate error handling code detected.
The error handling code for empty property_ids is duplicated between get_item_properties and lookup_item_ids.
50-73:
Potential memory leak in error handling.
The get_item_properties method allocates memory for values and errors arrays before the COM call. If the call fails, these arrays should be properly cleaned up.
opc_da/src/client/traits/item_sampling_mgt.rs (1)
91-101:
Fix potential use-after-free in set_item_buffer_enable.
The enable_bool vector is dropped while its pointer is still being used in the unsafe block. This could lead to undefined behavior.
let mut errors = RemoteArray::new(len);
let enable_bool: Vec<BOOL> = enable.iter().map(|&v| BOOL::from(v)).collect();
+ let enable_ptr = enable_bool.as_ptr();
unsafe {
self.interface()?.SetItemBufferEnable(
len,
server_handles.as_ptr(),
- enable_bool.as_ptr(),
+ enable_ptr,
errors.as_mut_ptr(),
)?;
}Likely invalid or redundant comment.
opc_da/src/server/com/memory.rs (2)
Line range hint 1-16: LGTM! Well-structured trait definitions for memory management.
The traits provide a clear interface for reference conversion and memory management with proper error handling.
Line range hint 91-107: Verify memory allocation handling in IntoComArrayRef implementation.
The implementation allocates memory using CoTaskMemAlloc but should ensure proper cleanup in case of subsequent errors.
Run the following script to check for memory leak patterns:
✅ Verification successful
Let me analyze the memory allocation patterns further by checking for any cleanup mechanisms in the codebase.
Let me try a different pattern to check the trait implementation.
Memory management is properly handled in the implementation
The implementation is safe because:
- The
FreeRawtrait andsafe_callmacro ensure proper cleanup of allocated memory in case of errors - The allocated memory is either:
- Stored in the output pointer (
*self = new_pointer) and owned by the caller - Or automatically freed if an error occurs through the
safe_callmacro
- Stored in the output pointer (
- The error handling for allocation failures is correct, returning
E_POINTERwhen allocation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper memory cleanup patterns after CoTaskMemAlloc
# Look for CoTaskMemAlloc calls that might leak memory
rg -A 10 "CoTaskMemAlloc" --type rust
Length of output: 4347
Script:
#!/bin/bash
# Check for Drop implementations and CoTaskMemFree usage
rg -A 5 "impl Drop|CoTaskMemFree" --type rust
Length of output: 2056
Script:
#!/bin/bash
# Check the full implementation of the IntoComArrayRef trait
ast-grep --pattern 'impl$_ IntoComArrayRef<$_> for $_ {
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Check the implementation context around IntoComArrayRef
rg -B 5 -A 15 "fn into_com_array_ref" --type rust
Length of output: 8855
opc_da/src/client/v3/mod.rs (1)
63-152: Refactor duplicate error handling pattern.
The error handling pattern for unsupported interfaces is repeated across multiple trait implementations.
opc_da/src/client/unified/server.rs (1)
22-82: LGTM! Well-structured Server implementation with clear version compatibility documentation.
The implementation properly handles:
- Required vs optional interfaces
- Version compatibility requirements
- Error cases for interface casting
opc_da/src/server/com/variant.rs (1)
Line range hint 86-91: LGTM: Additional From implementation provides better type safety
The new From implementation for windows::core::imp::VARIANT provides a clean conversion path while maintaining type safety.
opc_da/src/client/memory.rs (2)
34-39: LGTM: Safe implementation of slice conversion
The implementation correctly checks for null pointers and empty arrays before creating the slice.
113-124: LGTM: Correct String conversion implementation
The implementation correctly handles the conversion from RemotePointer to String, with proper null pointer checks and error handling.
opc_da/src/server/traits/server.rs (1)
Line range hint 16-247: LGTM: Consistent use of windows::core::Result type
The changes consistently update the Result type from windows_core to windows::core across all trait methods, improving type alignment with the windows crate.
opc_da/src/server/com/enumeration.rs (2)
Line range hint 28-46: LGTM! Thread-safe implementation of COM enumerator pattern.
The implementation correctly uses atomic operations for thread-safe index management and follows Windows COM conventions for error handling.
Also applies to: 47-62, 63-66
80-93: LGTM! Well-structured COM interface implementations.
The enumerator types are correctly defined with transparent representation for ABI compatibility and proper COM interface implementations.
opc_da/src/server/com/utils.rs (1)
Line range hint 48-60: LGTM! Safe implementation of pointer operations.
The implementation includes proper null pointer checks and comprehensive error handling for memory operations.
Also applies to: 82-96, 104-130
opc_da/src/server/traits/def.rs (3)
15-15: LGTM! Consistent type updates across structs.
The field type updates from windows_core::* to windows::core::* are applied consistently across all relevant structs.
Also applies to: 20-20, 24-24, 34-34, 94-94, 105-105
Line range hint 155-387: LGTM! Consistent error type updates.
The Error type updates from windows_core::Error to windows::core::Error are applied consistently across all TryFrom implementations.
Line range hint 1-5: Verify module restructuring impact.
The import path has been updated from crate::com to crate::server::com, suggesting a module hierarchy change.
✅ Verification successful
Let me gather more information about the module structure to better understand this change.
Let me check the specific module structure and content to understand if this is a new module or a moved module.
Import path change is correct and consistent
The module structure shows that com is now correctly placed under the server module (opc_da/src/server/com/), and there are no remaining references to the old import path crate::com:: in the codebase. The new import path crate::server::com:: accurately reflects the current module hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the new module path across the codebase
# Check for any remaining old import paths
rg "use crate::com::" -t rust
# Check for consistent usage of new import path
rg "use crate::server::com::" -t rust
Length of output: 129
Script:
#!/bin/bash
# Check the module structure and imports in the codebase
fd -e rs -x cat {} | rg "^(mod|pub mod|use) (com|server)"
# Also check for any direct references to com module
rg "::com::" -t rust
Length of output: 275
Script:
#!/bin/bash
# Check the content and structure of com-related modules
fd -t f -e rs com.rs
fd -t d com
# Check the content of server module
fd -t f -e rs server.rs
fd -t d server
# Look for any mod.rs files in these directories
fd mod.rs
Length of output: 646
opc_da/src/client/unified/client.rs (1)
15-15:
Fix incorrect error handling in Client::new()
The call to Self::initialize().ok()? converts the HRESULT into an Option<()>, causing potential error codes to be ignored. To correctly propagate errors, use the ? operator directly on Self::initialize().
Apply this diff to fix the error handling:
pub fn new() -> windows::core::Result<Self> {
- Self::initialize().ok()?;
+ Self::initialize()?;
Ok(Self {
_marker: std::marker::PhantomData,
})
}opc_da/src/client/traits/item_mgt.rs (6)
15-15:
Validate that items is not empty in add_items
In the add_items method, if the items slice is empty, the COM call may not behave as expected and could lead to errors. Add a check to verify that items is not empty before proceeding.
Apply this diff to add the validation:
fn add_items(
&self,
items: &[opc_da_bindings::tagOPCITEMDEF],
) -> windows::core::Result<(
memory::RemoteArray<opc_da_bindings::tagOPCITEMRESULT>,
memory::RemoteArray<windows::core::HRESULT>,
)> {
+ if items.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "items cannot be empty",
+ ));
+ }
let len = items.len().try_into()?;
let mut results = memory::RemoteArray::new(len);
let mut errors = memory::RemoteArray::new(len);
// ...
}39-39:
Validate that items is not empty in validate_items
Ensure that the items slice is not empty before making the COM call in the validate_items method to prevent unexpected behavior.
Apply this diff to add the validation:
fn validate_items(
&self,
items: &[opc_da_bindings::tagOPCITEMDEF],
blob_update: bool,
) -> windows::core::Result<(
memory::RemoteArray<opc_da_bindings::tagOPCITEMRESULT>,
memory::RemoteArray<windows::core::HRESULT>,
)> {
+ if items.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "items cannot be empty",
+ ));
+ }
let len = items.len().try_into()?;
let mut results = memory::RemoteArray::new(len);
let mut errors = memory::RemoteArray::new(len);
// ...
}60-60:
Validate that server_handles is not empty in remove_items
The remove_items method should check that server_handles is not empty to avoid making a COM call with no items, which could lead to errors.
Apply this diff to add the validation:
fn remove_items(
&self,
server_handles: &[u32],
) -> windows::core::Result<memory::RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles cannot be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut errors = memory::RemoteArray::new(len);
// ...
}76-76:
Validate that server_handles is not empty in set_active_state
Before setting the active state, ensure that server_handles is not empty to prevent unnecessary COM calls and possible errors.
Apply this diff to add the validation:
fn set_active_state(
&self,
server_handles: &[u32],
active: bool,
) -> windows::core::Result<memory::RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles cannot be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut errors = memory::RemoteArray::new(len);
// ...
}96-96:
Ensure server_handles and client_handles have the same length
In set_client_handles, the lengths of server_handles and client_handles must match to associate each server handle with its corresponding client handle correctly.
Apply this diff to add the length check:
fn set_client_handles(
&self,
server_handles: &[u32],
client_handles: &[u32],
) -> windows::core::Result<memory::RemoteArray<windows::core::HRESULT>> {
+ if server_handles.len() != client_handles.len() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles and client_handles must have the same length",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut errors = memory::RemoteArray::new(len);
// ...
}116-116:
Ensure server_handles and requested_datatypes have the same length
In set_datatypes, the server_handles and requested_datatypes slices should be of equal length to set the data types for each server handle correctly.
Apply this diff to add the length check:
fn set_datatypes(
&self,
server_handles: &[u32],
requested_datatypes: &[u16],
) -> windows::core::Result<memory::RemoteArray<windows::core::HRESULT>> {
+ if server_handles.len() != requested_datatypes.len() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles and requested_datatypes must have the same length",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut errors = memory::RemoteArray::new(len);
// ...
}opc_da/src/client/traits/common.rs (1)
18-26:
Handle potential null pointers in query_available_locale_ids
The method query_available_locale_ids uses RemoteArray::emptry() to initialize locale_ids. Ensure that you handle the possibility of null pointers returned from the COM call to prevent runtime errors.
Apply this diff to check for null pointers:
fn query_available_locale_ids(&self) -> windows::core::Result<RemoteArray<u32>> {
let mut locale_ids = RemoteArray::emptry();
unsafe {
self.interface()?
.QueryAvailableLocaleIDs(locale_ids.as_mut_len_ptr(), locale_ids.as_mut_ptr())?;
+ if locale_ids.as_ptr().is_null() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null pointer from QueryAvailableLocaleIDs",
+ ));
+ }
}
Ok(locale_ids)
}opc_da/src/client/traits/browse_server_address_space.rs (2)
50-52:
Handle null pointer returns in get_item_id method
The COM method GetItemID may return a null pointer if the provided item_data_id is invalid. Attempting to convert a null pointer to a String will result in a runtime error. Add a check to handle null pointers.
Apply this diff to check for null pointers:
let output = unsafe { self.interface()?.GetItemID(item_data_id.as_pwstr())? };
+if output.is_null() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null pointer from GetItemID",
+ ));
+}
RemotePointer::from(output).try_into()59-60:
Check for null pointers in browse_access_paths method
Ensure that the BrowseAccessPaths method handles null pointer returns to prevent potential runtime errors when the item_id is invalid.
Apply this diff to handle null pointers:
let item_id = LocalPointer::from_str(item_id)?;
let result = unsafe { self.interface()?.BrowseAccessPaths(item_id.as_pwstr())? };
+if result.is_null() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null pointer from BrowseAccessPaths",
+ ));
+}
Ok(result)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (17)
opc_da/src/server/com/enumeration.rs (1)
Line range hint
140-334: Standardize error handling patterns across implementations.The implementations use different error handling patterns:
- Some methods return
windows::core::HRESULTdirectly- Others return
windows::core::Result<()>ItemAttributesEnumeratoruses thesafe_call!macroConsider standardizing the error handling approach across all implementations for better maintainability.
Example standardization using
Result:impl IEnumString_Impl for StringEnumerator_Impl { fn Next( &self, count: u32, range_elements: *mut windows::core::PWSTR, count_fetched: *mut u32, - ) -> windows::core::HRESULT { + ) -> windows::core::Result<()> { let fetched = count_fetched.into_ref()?; let elements = range_elements.into_array_ref(count)?; let mut strings = Vec::with_capacity(count as usize); - let code = self.0.next(count, fetched, &mut strings); - if code != S_OK { - return code; - } + self.0.next(count, fetched, &mut strings).ok()?; for (i, string) in strings.iter_mut().enumerate() { let pwstr = PWSTR(string.as_mut_ptr()); elements[i] = pwstr; } - S_OK + Ok(()) }opc_da/src/client/traits/server.rs (4)
11-51: Consider implementing a builder pattern for better parameter management.The method has many parameters which could be better managed using a builder pattern or parameter struct. This would improve readability and make it easier to maintain.
Example implementation:
pub struct GroupParams { name: String, active: bool, client_handle: u32, update_rate: u32, locale_id: u32, time_bias: i32, percent_deadband: f32, } impl GroupParams { pub fn new(name: String) -> Self { Self { name, active: true, client_handle: 0, update_rate: 1000, locale_id: 0, time_bias: 0, percent_deadband: 0.0, } } // Builder methods... }
26-26: Consider returning the revised percent deadband value.The method accepts a percent_deadband parameter and receives a revised value from the COM call, but this revised value is not returned to the caller.
Consider returning a struct that includes both the group and the revised value:
pub struct AddGroupResult<Group> { pub group: Group, pub revised_percent_deadband: f32, }Also applies to: 38-38
53-62: Add documentation for the get_status method.The method implementation looks good, but would benefit from documentation explaining the returned status structure and possible error conditions.
Add documentation like:
/// Retrieves the current status of the OPC server. /// /// # Returns /// Returns a Result containing the server status structure (tagOPCSERVERSTATUS) /// which includes information about the server state, start time, vendor info, etc. /// /// # Errors /// Returns an error if the server status cannot be retrieved or if the returned /// pointer is null.
64-70: Add documentation explaining the force parameter.The method implementation is correct, but the
forceparameter's implications should be documented.Add documentation like:
/// Removes a group from the server. /// /// # Arguments /// * `server_handle` - The server's handle for the group to remove /// * `force` - If true, the group will be removed even if it has active /// connections. If false, the removal will fail if there are active connections. /// /// # Errors /// Returns an error if the group cannot be removed or if it doesn't exist.opc_da/src/client/traits/item_mgt.rs (1)
119-151: Consider adding validation for emptyclient_handlesinset_client_handlesWhile you check that
server_handlesandclient_handleshave the same length and thatserver_handlesis not empty, it's a good practice to also verify thatclient_handlesis not empty to prevent potential issues.Apply this diff to add the validation:
if server_handles.len() != client_handles.len() { return Err(windows_core::Error::new( windows::Win32::Foundation::E_INVALIDARG, "server_handles and client_handles must have the same length", )); } + if client_handles.is_empty() { + return Err(windows_core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + "client_handles cannot be empty", + )); + } if server_handles.is_empty() { return Err(windows_core::Error::new( windows::Win32::Foundation::E_INVALIDARG, "server_handles cannot be empty", )); }opc_da/src/client/traits/sync_io.rs (2)
45-57: Consider Checking for Emptyserver_handlesBefore Length ComparisonIn the
writemethod, you first check ifserver_handles.len() != values.len(), and then check ifserver_handlesis empty. Ifserver_handlesis empty, the length comparison may be less meaningful. Consider checking ifserver_handlesis empty before comparing lengths to improve clarity and efficiency.Apply this diff to rearrange the validation checks:
fn write( &self, server_handles: &[u32], values: &[VARIANT], ) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> { + if server_handles.is_empty() { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + "server_handles cannot be empty", + )); + } if server_handles.len() != values.len() { return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, "server_handles and values must have the same length", )); }
22-22: Explicitly Handle Potential Conversion Failure oflenWhen converting
server_handles.len()usingtry_into(), there's a possibility of a conversion failure if the length exceeds the maximum value of the target integer type. Although the?operator will propagate the error, consider handling this case explicitly to provide a clearer error message specific to the context.Apply this diff to handle the conversion failure explicitly in both
readandwritemethods:// In the `read` method let len = server_handles.len().try_into().map_err(|_| { windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, "server_handles length is too large", ) })?; // In the `write` method let len = server_handles.len().try_into().map_err(|_| { windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, "server_handles length is too large", ) })?;Also applies to: 59-59
opc_da/src/client/traits/sync_io2.rs (7)
18-23: Include actual lengths in error message for clarityTo aid in debugging, consider including the lengths of
server_handlesandmax_agein the error message when they do not match.Apply this diff to enhance the error message:
return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "server_handles and max_age must have the same length", + &format!( + "server_handles (len={}) and max_age (len={}) must have the same length", + server_handles.len(), + max_age.len() + ), ));
25-30: Include length in empty array error messageFor better clarity, include the length of
server_handlesin the error message when it's empty.Apply this diff to the error message:
return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "server_handles cannot be empty", + &format!("server_handles cannot be empty (len={})", server_handles.len()), ));
32-32: Handle potential overflow when converting lengthUsing
try_into()?may return a generic error if the length exceedsu32::MAX. Consider adding a specific check to provide a clearer error message.Apply this diff to add an explicit check:
+ let len = server_handles.len(); + if len > u32::MAX as usize { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + &format!("Number of server handles ({}) exceeds maximum allowed ({})", len, u32::MAX), + )); + } + let len = len as u32;
59-64: Include actual lengths in error message for clarityTo aid in debugging, consider including the lengths of
server_handlesandvaluesin the error message when they do not match.Apply this diff to enhance the error message:
return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "server_handles and values must have the same length", + &format!( + "server_handles (len={}) and values (len={}) must have the same length", + server_handles.len(), + values.len() + ), ));
66-71: Include length in empty array error messageFor better clarity, include the length of
server_handlesin the error message when it's empty.Apply this diff to the error message:
return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "server_handles cannot be empty", + &format!("server_handles cannot be empty (len={})", server_handles.len()), ));
73-73: Handle potential overflow when converting lengthUsing
try_into()?may return a generic error if the length exceedsu32::MAX. Consider adding a specific check to provide a clearer error message.Apply this diff to add an explicit check:
+ let len = server_handles.len(); + if len > u32::MAX as usize { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + &format!("Number of server handles ({}) exceeds maximum allowed ({})", len, u32::MAX), + )); + } + let len = len as u32;
12-17: Consider introducing a type alias for complex return typeThe return type of
read_max_ageis complex and might affect readability. Introducing a type alias can enhance maintainability and clarity.Define a type alias for the return type:
type ReadMaxAgeResult = ( RemoteArray<VARIANT>, RemoteArray<u16>, RemoteArray<windows::Win32::Foundation::FILETIME>, RemoteArray<windows::core::HRESULT>, );Update the method signature:
- ) -> windows::core::Result<( - RemoteArray<VARIANT>, - RemoteArray<u16>, - RemoteArray<windows::Win32::Foundation::FILETIME>, - RemoteArray<windows::core::HRESULT>, - )> { + ) -> windows::core::Result<ReadMaxAgeResult> {opc_da/src/client/unified/actor/client.rs (2)
9-9: Consider making mailbox capacity configurableThe mailbox capacity is set to a hardcoded value of
128. Consider defining this value as a constant or making it configurable to improve maintainability and flexibility.You can apply the following diff to define a constant and use it:
+const MAILBOX_CAPACITY: usize = 128; impl Actor for Client { type Context = Context<Self>; fn started(&mut self, ctx: &mut Self::Context) { - ctx.set_mailbox_capacity(128); + ctx.set_mailbox_capacity(MAILBOX_CAPACITY); } }
35-41: Consider adding unit tests forget_serversCurrently, there are no unit tests covering the
get_serversmethod inClientActor. Adding tests will help ensure the method functions correctly and reliably.Would you like assistance in creating unit tests for this method, or should I open a new GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/traits/item_mgt.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/traits/sync_io.rs(1 hunks)opc_da/src/client/traits/sync_io2.rs(1 hunks)opc_da/src/client/unified/actor/client.rs(1 hunks)opc_da/src/server/com/enumeration.rs(16 hunks)opc_da/src/server/com/utils.rs(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- opc_da/src/server/com/utils.rs
- opc_da/src/client/memory.rs
🔇 Additional comments (14)
opc_da/src/server/com/enumeration.rs (2)
Line range hint 28-71: LGTM! Thread-safe implementation with proper bounds checking.
The implementation correctly uses atomic operations with appropriate memory ordering for thread safety, while properly handling edge cases and bounds checking.
Line range hint 1-334: Verify proper cleanup of COM resources.
Since this code deals with COM interfaces and memory management, let's verify that these enumerators are properly cleaned up when used in the codebase.
✅ Verification successful
COM enumerators are properly managed through Windows COM reference counting.
The code is safe because:
- The enumerators are implemented using Windows COM interfaces (
IEnumString,IEnumUnknown, etc.) which automatically handle reference counting through the COM infrastructure. - The
windows::core::ComObjectInner::into_object()and.into_interface()calls properly wrap the enumerators into COM objects. - The underlying data (
Vec<T>) is cloned when needed, ensuring no dangling references. - The COM objects are properly released when they go out of scope due to COM's built-in reference counting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper cleanup patterns of enumerators
# Look for Drop implementations or explicit cleanup calls
echo "Checking for Drop implementations and cleanup patterns..."
rg -A 5 "impl Drop for" opc_da/src/
rg -A 5 "::Release" opc_da/src/
# Look for usage patterns of these enumerators
echo "Checking usage patterns..."
rg -A 10 "StringEnumerator|UnknownEnumerator|ConnectionPointsEnumerator|ConnectionsEnumerator|ItemAttributesEnumerator" opc_da/src/
Length of output: 12971
opc_da/src/client/traits/server.rs (1)
1-10: LGTM! Well-structured trait definition with appropriate error handling.
The trait is properly defined with a generic Group type and appropriate constraints for COM interface operations.
opc_da/src/client/traits/item_mgt.rs (6)
1-6: Well-structured trait definition and interface method
The ItemMgtTrait and its interface method are properly defined, providing a clear contract for item management functionalities.
8-36: add_items method correctly handles empty input and COM interaction
The method includes a check for empty items, allocates result and error arrays appropriately, and safely calls the COM method AddItems.
38-68: Proper implementation of validate_items method
The validate_items function correctly validates the input, handles the blob_update parameter, and invokes the ValidateItems COM method with accurate parameters.
70-90: remove_items method is implemented correctly
The method validates that server_handles is not empty and properly calls the RemoveItems COM function.
92-117: set_active_state method functions as expected
Input validation for server_handles and the active state handling are correctly implemented, ensuring safe interaction with the COM SetActiveState method.
187-195: create_enumerator method is correctly implemented
The method safely invokes CreateEnumerator and properly casts the result to IEnumUnknown.
opc_da/src/client/traits/sync_io.rs (1)
7-38: Code Implementation in read Method Looks Good
The read method is well-implemented with appropriate error handling for empty server_handles and correct usage of the COM interface within the unsafe block.
opc_da/src/client/traits/sync_io2.rs (2)
39-49: Unsafe block is properly encapsulated
The usage of the unsafe block is necessary and well-contained. Pointers are correctly obtained from the slices, and the call to the COM interface is safely executed.
77-84: Unsafe block is properly encapsulated
The usage of the unsafe block is necessary and well-contained. Pointers are correctly obtained from the slices, and the call to the COM interface is safely executed.
opc_da/src/client/unified/actor/client.rs (2)
5-11:
Critical issue: Client is not Send or Sync, but used as an Actor
The Client struct is not Send or Sync due to the _marker field. Actix actors are required to be Send because they may run in a different thread from where they were created. Using a non-Send actor can lead to undefined behavior or runtime errors.
Consider modifying the actor to run in a single-threaded context using actix::prelude::LocalActor or redesign Client to safely implement Send and Sync if possible.
50-56:
Ensure safety in unsafe blocks within handle method
In the unsafe blocks where ProgIDFromCLSID is called and name.to_string() is used, please ensure that these operations are safe and that all necessary checks are performed to prevent undefined behavior.
…mpty method, and enhance error handling with additional checks for empty inputs in various traits
…de settings for improved clarity
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (17)
opc_da/src/client/traits/item_properties.rs (3)
8-47: Consider adding error handling for zero properties case.While the method correctly handles empty item_id and array length management, it might be worth adding explicit error handling when no properties are found (count == 0).
if count > 0 { property_ids.set_len(count); descriptions.set_len(count); datatypes.set_len(count); + } else { + return Err(windows::core::Error::new( + windows::Win32::Foundation::S_FALSE, + "No properties available for the specified item", + )); }
82-113: Consider consolidating length conversions.The method makes multiple try_into() calls for the same length value. Consider converting once and reusing the result.
- let mut new_item_ids = RemoteArray::new(property_ids.len().try_into()?); - let mut errors = RemoteArray::new(property_ids.len().try_into()?); + let len: u32 = property_ids.len().try_into()?; + let mut new_item_ids = RemoteArray::new(len); + let mut errors = RemoteArray::new(len); unsafe { self.interface()?.LookupItemIDs( item_id.as_pcwstr(), - property_ids.len().try_into()?, + len, property_ids.as_ptr(),
5-114: Consider adding documentation for the trait and its methods.The trait would benefit from documentation comments explaining:
- The purpose of the trait
- The contract that implementors must fulfill
- The behavior and error conditions of each method
- Examples of usage
This would improve maintainability and help future implementors.
opc_da/src/client/traits/item_deadband_mgt.rs (1)
1-4: Add documentation for the trait and its methods.Consider adding documentation comments to:
- Explain the purpose and usage of the
ItemDeadbandMgtTrait- Document the contract that implementors must fulfill
- Describe error conditions and safety requirements
Example documentation:
/// Trait for managing OPC DA item deadbands. /// /// This trait provides methods to set, get, and clear deadband values for OPC items. /// Implementors must provide access to an [`IOPCItemDeadbandMgt`] interface. /// /// # Safety /// Methods in this trait use unsafe code to interact with the OPC DA interface. pub trait ItemDeadbandMgtTrait { /// Returns a reference to the underlying [`IOPCItemDeadbandMgt`] interface. /// /// # Errors /// Returns an error if the interface is not available. fn interface(&self) -> windows::core::Result<&opc_da_bindings::IOPCItemDeadbandMgt>;opc_da/src/client/unified/iter.rs (6)
16-23: Consider removing unused constructor or marking it with#[allow(dead_code)]Static analysis indicates this constructor is never used. Either remove it if not needed or explicitly mark it as allowed dead code if it's intended for future use.
+ #[allow(dead_code)] pub fn new(iter: windows::Win32::System::Com::IEnumGUID) -> Self {
34-46: Enhance error handling with more specific error messagesThe error handling could be improved by including more context about the failure.
- return Some(Err(windows::core::Error::new( - code, - "Failed to get next GUID", - ))); + return Some(Err(windows::core::Error::new( + code, + format!("Failed to get next GUID from IEnumGUID: {:?}", code), + )));
9-9: Consider making the cache size configurableThe fixed cache size of 16 elements might not be optimal for all use cases. Consider making this configurable or defining it as a constant.
+const GUID_CACHE_SIZE: usize = 16; + pub struct GuidIterator { iter: windows::Win32::System::Com::IEnumGUID, - cache: [windows::core::GUID; 16], + cache: [windows::core::GUID; GUID_CACHE_SIZE],
63-70: Consider removing unused constructor or marking it with#[allow(dead_code)]Static analysis indicates this constructor is never used. Either remove it if not needed or explicitly mark it as allowed dead code if it's intended for future use.
+ #[allow(dead_code)] pub fn new(iter: windows::Win32::System::Com::IEnumString) -> Self {
57-57: Maintain consistency in cache size configurationSimilar to GuidIterator, consider making the cache size configurable and consistent across both iterators.
+const STRING_CACHE_SIZE: usize = 16; + pub struct StringIterator { iter: windows::Win32::System::Com::IEnumString, - cache: [windows::core::PWSTR; 16], + cache: [windows::core::PWSTR; STRING_CACHE_SIZE],
1-100: Consider refactoring to reduce code duplicationThe
GuidIteratorandStringIteratorshare similar patterns and could benefit from being refactored to use a common trait or generic implementation. This would:
- Reduce code duplication
- Ensure consistent error handling
- Share configuration constants
- Maintain consistent safety checks
Consider creating a trait like:
trait ComIterator { type Item; type ComInterface; fn next_batch(&mut self) -> windows::core::Result<u32>; fn get_cached_item(&self, index: usize) -> windows::core::Result<Self::Item>; }Then implement the common iteration logic in a generic wrapper struct that uses this trait.
opc_da/src/client/traits/common.rs (2)
7-8: Excellent trait design pattern!The trait is well-designed with a single required method
interface()that other methods build upon. This follows the interface segregation principle and makes the trait easy to implement.This design pattern allows for:
- Easy mocking in tests
- Flexibility in implementation
- Clear separation of concerns
18-27: Add documentation for error conditions.The
query_available_locale_idsmethod would benefit from documentation explaining possible error conditions and the ownership of returned array.Add documentation like this:
+ /// Queries the available locale IDs supported by the server. + /// + /// # Errors + /// Returns an error if: + /// - The COM call fails + /// - Memory allocation fails + /// - The server is not respondingopc_da/src/client/traits/async_io2.rs (1)
74-76: Avoid usingmapin unsafe block forget_enablemethodUsing
mapwithin anunsafeblock can lead to unintended side effects. Refactor the method to handle the result more explicitly.Apply this diff:
fn get_enable(&self) -> windows::core::Result<bool> { - unsafe { self.interface()?.GetEnable().map(|v| v.as_bool()) } + unsafe { + let mut enabled = windows::Win32::Foundation::BOOL::default(); + self.interface()?.GetEnable(&mut enabled)?; + Ok(enabled.as_bool()) + } }opc_da/src/client/traits/server.rs (1)
45-67: Minimize the scope of the unsafe blockThe unsafe block in the
add_groupmethod encompasses more code than necessary. To enhance safety, it's advisable to minimize the unsafe block to include only the COM call toAddGroup. This reduces the risk of unsound code within the unsafe block.Apply this diff to minimize the unsafe block:
- unsafe { self.interface()?.AddGroup( group_name, windows::Win32::Foundation::BOOL::from(active), update_rate, client_handle, &time_bias, &percent_deadband, locale_id, &mut group_server_handle, &mut revised_percent_deadband, &opc_da_bindings::IOPCItemMgt::IID, &mut group, )?; - match group { - None => Err(windows::core::Error::new( - windows::Win32::Foundation::E_POINTER, - "Failed to add group, returned null", - )), - Some(group) => group.try_into(), - } - } + } + + match group { + None => Err(windows::core::Error::new( + windows::Win32::Foundation::E_POINTER, + "Failed to add group, returned null", + )), + Some(group) => group.try_into(), + }opc_da/src/client/memory.rs (3)
113-124: Consider adding error context to the pointer conversionWhile the implementation is correct, consider adding more context to the error when the pointer is null.
fn try_from(value: RemotePointer<u16>) -> Result<Self, Self::Error> { if value.inner.is_null() { - return Err(windows::Win32::Foundation::E_POINTER.into()); + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_POINTER, + "Cannot convert null PWSTR to String".into(), + )); } Ok(unsafe { PWSTR(value.inner).to_string() }?) }
185-190: Consider pre-calculating string length for performanceThe current implementation chains an extra null terminator after encoding. For long strings, pre-allocating the vector with the correct capacity would be more efficient.
impl From<&str> for LocalPointer<Vec<u16>> { #[inline(always)] fn from(s: &str) -> Self { - Self::new(Some(s.encode_utf16().chain(Some(0)).collect())) + let mut vec = Vec::with_capacity(s.len() + 1); + vec.extend(s.encode_utf16()); + vec.push(0); + Self::new(Some(vec)) } }
192-202: Consider optimizing memory allocation for string arraysWhen converting multiple strings, consider pre-allocating the vector capacity to reduce reallocations.
impl From<&[String]> for LocalPointer<Vec<Vec<u16>>> { #[inline(always)] fn from(values: &[String]) -> Self { + let mut vec = Vec::with_capacity(values.len()); + for s in values { + let mut inner = Vec::with_capacity(s.len() + 1); + inner.extend(s.encode_utf16()); + inner.push(0); + vec.push(inner); + } - Self::new(Some( - values - .iter() - .map(|s| s.encode_utf16().chain(Some(0)).collect()) - .collect(), - )) + Self::new(Some(vec)) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/traits/async_io.rs(1 hunks)opc_da/src/client/traits/async_io2.rs(1 hunks)opc_da/src/client/traits/async_io3.rs(1 hunks)opc_da/src/client/traits/browse.rs(1 hunks)opc_da/src/client/traits/common.rs(1 hunks)opc_da/src/client/traits/group_state_mgt.rs(1 hunks)opc_da/src/client/traits/item_deadband_mgt.rs(1 hunks)opc_da/src/client/traits/item_mgt.rs(1 hunks)opc_da/src/client/traits/item_properties.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/unified/actor/client.rs(1 hunks)opc_da/src/client/unified/actor/mod.rs(1 hunks)opc_da/src/client/unified/actor/server.rs(1 hunks)opc_da/src/client/unified/iter.rs(1 hunks)opc_da/src/server/com/base/variant.rs(0 hunks)opc_da/src/server/com/enumeration.rs(16 hunks)opc_da/src/server/com/variant.rs(3 hunks)
💤 Files with no reviewable changes (1)
- opc_da/src/server/com/base/variant.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- opc_da/src/client/unified/actor/server.rs
- opc_da/src/client/unified/actor/mod.rs
- opc_da/src/server/com/variant.rs
- opc_da/src/client/unified/actor/client.rs
- opc_da/src/server/com/enumeration.rs
🧰 Additional context used
📓 Learnings (1)
opc_da/src/client/memory.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🔇 Additional comments (25)
opc_da/src/client/traits/item_properties.rs (3)
1-5: LGTM! Clean and focused imports.
The imports are minimal and appropriate for the trait's functionality.
6-6: LGTM! Well-defined interface method.
The method provides a clean abstraction for accessing the underlying COM interface.
49-80: Consider deduplicating error handling code.
The error handling code for empty property_ids is duplicated between get_item_properties and lookup_item_ids.
opc_da/src/client/traits/item_deadband_mgt.rs (4)
34-53: Add empty check for server_handles.
This is a duplicate of a previous review comment. The method should validate that server_handles is not empty.
55-72: Add empty check for server_handles.
This is a duplicate of a previous review comment. The method should validate that server_handles is not empty.
22-29: Unsafe blocks are well-contained and necessary.
The unsafe blocks are appropriately used for FFI calls and are well-contained within the methods. The code maintains safety by:
- Validating inputs before entering unsafe blocks
- Using appropriate pointer types
- Properly managing memory with RemoteArray
Also applies to: 43-50, 63-69
6-32: 🛠️ Refactor suggestion
Add empty check validation.
While the length comparison is good, consider adding an empty check similar to the suggestion in the past review comments.
Apply this diff:
fn set_item_deadband(
&self,
server_handles: &[u32],
dead_bands: &[f32],
) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
if server_handles.len() != dead_bands.len() {
return Err(windows::core::Error::new(Likely invalid or redundant comment.
opc_da/src/client/unified/iter.rs (1)
96-99:
Add bounds check and null pointer validation
The current implementation lacks important safety checks:
- Array bounds check before accessing
self.cache - Null pointer validation before converting PWSTR
self.count -= 1;
+ assert!(self.count < self.cache.len() as u32);
+ let ptr = self.cache[self.count as usize];
+
+ if ptr.is_null() {
+ return Some(Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null string pointer from COM",
+ )));
+ }
- Some(RemotePointer::from(self.cache[self.count as usize]).try_into())
+ Some(RemotePointer::from(ptr).try_into())Likely invalid or redundant comment.
opc_da/src/client/traits/common.rs (3)
1-6: LGTM! Well-organized imports with clear memory management.
The imports are properly structured, bringing in necessary memory management types and the OPC DA COM interface.
29-38: LGTM! Proper memory management for string conversions.
The string handling in both get_error_string and set_client_name methods properly manages memory conversion between Rust and COM strings using RemotePointer and LocalPointer.
21-24: Verify error handling in unsafe block.
The unsafe block contains COM interface calls and memory operations. Let's verify the error handling is comprehensive.
✅ Verification successful
Let me gather more context about the error handling patterns in this codebase.
Let me check one more pattern to understand the error handling in the codebase better.
Let me check one final pattern to understand the error handling in the RemoteArray implementation.
Based on the comprehensive analysis of the code, I can now provide the final response:
Error handling in the unsafe block is properly implemented
The code shows a well-structured approach to memory safety and error handling:
- The
RemoteArraytype properly manages COM memory allocation and deallocation through its Drop implementation - The interface call is properly wrapped in Result with
?operator for error propagation - Memory cleanup is guaranteed through RAII pattern in RemoteArray's Drop implementation
- The COM interface call follows the established pattern used throughout the codebase for similar operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of COM error handling in the codebase
ast-grep --pattern 'unsafe {
$$$
.QueryAvailableLocaleIDs($$$)
$$$
}'
# Look for any error handling patterns with these COM interfaces
rg -A 3 'IOPCCommon.*QueryAvailableLocaleIDs'
Length of output: 715
Script:
#!/bin/bash
# Look for the full implementation context of this method
rg -B 5 -A 10 "QueryAvailableLocaleIDs" opc_da/src/client/traits/common.rs
# Check error handling patterns in similar unsafe COM interface calls
ast-grep --pattern 'unsafe {
$$$
self.interface()?.$_($$$)?
$$$
}'
# Look for the ComInterface trait implementation
rg -A 5 "impl.*ComInterface"
Length of output: 728
Script:
#!/bin/bash
# Look for RemoteArray usage and error handling
rg -B 3 -A 3 "RemoteArray"
# Check the interface() method implementation
rg -B 3 -A 3 "fn interface"
# Look for similar unsafe blocks with COM interface calls
rg -B 3 -A 3 "unsafe.*self\.interface\(\)\?"
Length of output: 71614
Script:
#!/bin/bash
# Look for RemoteArray implementation and error handling
rg -B 5 -A 10 "impl.*RemoteArray"
# Check for any Drop implementation that might handle cleanup
rg -B 3 -A 10 "Drop for RemoteArray"
# Look for memory safety patterns in the codebase
rg -A 5 "unsafe impl" opc_da/src/client/memory.rs
Length of output: 2276
opc_da/src/client/traits/async_io2.rs (2)
12-17: Add check for empty server_handles in read method
To prevent unexpected behavior when server_handles is empty, add a check to ensure it is not empty before proceeding with the read operation.
Apply this diff:
fn read(
&self,
server_handles: &[u32],
transaction_id: u32,
) -> windows::core::Result<(u32, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;36-49: Add length and empty checks in write method
Ensure that server_handles and values have the same length and that server_handles is not empty to prevent mismatches and errors during the write operation.
Apply this diff:
fn write(
&self,
server_handles: &[u32],
values: &[VARIANT],
transaction_id: u32,
) -> windows::core::Result<(u32, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.len() != values.len() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles and values must have the same length",
+ ));
+ } else if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;opc_da/src/client/traits/async_io3.rs (2)
12-17: Add check for empty server_handles in read_max_age method
Ensure that server_handles is not empty before performing the read operation to prevent undefined behavior.
Apply this diff:
if server_handles.len() != max_age.len() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"server_handles and max_age must have the same length",
));
+} else if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
}44-49: Add check for empty server_handles in write_vqt method
To prevent errors during the write operation, check that server_handles is not empty and matches the length of values.
Apply this diff:
if server_handles.len() != values.len() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"server_handles and values must have the same length",
));
+} else if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
}opc_da/src/client/traits/async_io.rs (1)
4-89: Code implementation is correct and follows best practices
The AsyncIoTrait is well-defined, and all methods correctly handle input validation and error cases.
opc_da/src/client/traits/server.rs (1)
98-98:
Handle potential None value from CreateGroupEnumerator
The CreateGroupEnumerator method may return None, and calling cast() on a None value can lead to unexpected errors. It's important to handle this case explicitly to prevent a potential panic or undefined behavior.
Apply this diff to handle the potential None value:
};
-
- enumerator.cast()
+ match enumerator {
+ None => Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Failed to create group enumerator",
+ )),
+ Some(enumerator) => enumerator.cast(),
+ }opc_da/src/client/traits/group_state_mgt.rs (3)
71-71:
Missing error propagation in set_name
The call to SetName may fail and return an error. The method currently does not propagate this error, which may lead to silent failures. Using the ? operator ensures that any error is returned to the caller.
Apply this diff to handle the potential error:
unsafe { self.interface()?.SetName(name.as_pwstr()) }
+ ?81-81:
Missing error propagation in clone_group
The call to CloneGroup may fail and return an error. Without propagating this error, the caller may not be aware of the failure. Using the ? operator addresses this issue by forwarding the error.
Apply this diff to handle the potential error:
unsafe { self.interface()?.CloneGroup(name.as_pwstr(), id) }
+ ?17-32:
Potential use-after-free in get_state method
The conversion of name to a String occurs after the unsafe block. If an error arises before this conversion, the memory allocated for name may have been freed, leading to a use-after-free scenario.
To prevent this, convert name immediately after the GetState call succeeds:
}?;
+ state.name = name.try_into()?;
state.active = active.as_bool();
- state.name = name.try_into()?;opc_da/src/client/traits/browse.rs (2)
24-24: Validate length conversion before RemoteArray creation
The conversion of item_ids.len() to u32 using try_into() could fail for very large arrays. Adding explicit validation improves robustness by handling potential conversion errors gracefully.
Apply this diff to add explicit validation:
let item_ptrs: LocalPointer<Vec<Vec<u16>>> = LocalPointer::from(item_ids);
let item_ptrs = item_ptrs.as_pcwstr_array();
- let mut results = RemoteArray::new(item_ids.len().try_into()?);
+ let count: u32 = item_ids.len().try_into().map_err(|_| {
+ windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "item_ids length exceeds u32::MAX",
+ )
+ })?;
+ let mut results = RemoteArray::new(count);76-78:
Add bounds check before setting elements length
When setting the length of elements, it's important to ensure that count does not exceed max_elements to prevent buffer overflows or unexpected behavior.
Apply this diff to add the bounds check:
}
+ if count > max_elements {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "Server returned more elements than requested",
+ ));
+ }
if count > 0 {
elements.set_len(count);
}opc_da/src/client/memory.rs (3)
34-39: LGTM! Safe implementation of slice conversion
The implementation correctly handles null pointers and empty arrays, preventing undefined behavior.
62-71: LGTM! Proper COM memory cleanup
The Drop implementation correctly frees COM-allocated memory using CoTaskMemFree with appropriate null checks.
204-226: Consider adding validation for empty arrays
The current implementation returns a vector with a null pointer for None case, but doesn't validate empty arrays in the Some case.
impl LocalPointer<Vec<Vec<u16>>> {
#[inline(always)]
pub fn as_pwstr_array(&self) -> Vec<windows::core::PWSTR> {
match &self.inner {
- Some(values) => values
+ Some(values) if !values.is_empty() => values
.iter()
.map(|value| windows::core::PWSTR(value.as_ptr() as _))
.collect(),
- None => vec![windows::core::PWSTR::null()],
+ _ => vec![windows::core::PWSTR::null()],
}
}
#[inline(always)]
pub fn as_pcwstr_array(&self) -> Vec<windows::core::PCWSTR> {
match &self.inner {
- Some(values) => values
+ Some(values) if !values.is_empty() => values
.iter()
.map(|value| windows::core::PCWSTR::from_raw(value.as_ptr() as _))
.collect(),
- None => vec![windows::core::PCWSTR::null()],
+ _ => vec![windows::core::PCWSTR::null()],
}
}
}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/rust-clippy.yml(2 hunks).github/workflows/rust.yml(1 hunks).vscode/settings.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/rust-clippy.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/rust.yml
15-15: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/rust.yml (1)
4-4: LGTM!
Branch specifications are correctly formatted and aligned.
Also applies to: 6-6
…ance VSCode settings for improved clarity
|
@coderabbitai If I use flag COINIT_MULTITHREADED to initialize com for client, can i use actix actor with async context? |
|
Tip For best results, initiate chat on the files or code changes. @Ronbb Yes, if you initialize COM for the client with the However, be cautious if you're interacting with COM objects that require a single-threaded apartment (STA), as they may not function correctly in an MTA environment. Ensure that the COM interfaces and objects you're using are thread-safe and support being called from an MTA. If they require an STA, you might need to run them in a dedicated thread initialized with |
… improved error handling
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (16)
opc_da/src/client/unified/mod.rs (1)
8-13: Consider explicit exports instead of wildcards.While wildcard exports (
*) make the API more accessible, they can lead to maintenance challenges:
- Harder to track the origin of specific items
- Potential name conflicts between modules
- Difficulty in maintaining API stability
Consider explicitly listing the items you want to re-export:
-pub use actor::*; -pub use client::*; -pub use group::*; -pub use guard::*; -pub use iter::*; -pub use server::*; +// Re-export specific items from actor +pub use actor::{ClientActor, ClientMessage}; +// Re-export specific items from client +pub use client::Client; +// Re-export specific items from group +pub use group::Group; +// Re-export specific items from guard +pub use guard::Guard; +// Re-export specific items from iter +pub use iter::{GuidIter, StringIter}; +// Re-export specific items from server +pub use server::Server;opc_da/src/client/unified/group.rs (2)
6-10: LGTM! Clean version abstraction design.The
Groupenum effectively encapsulates different OPC DA versions, providing a unified interface while maintaining version-specific implementations. This design pattern allows for easy extension if new versions are added in the future.Consider documenting the version compatibility requirements and any version-specific behaviors in the enum's documentation.
13-14: Consider ifinline(always)is necessary.The
inline(always)attribute forces the compiler to inline this function in all cases. For such a simple pattern match, the compiler's default inlining heuristics might be sufficient.- #[inline(always)] + #[inline] fn item_mgt(&self) -> &dyn ItemMgtTrait {opc_da/src/client/memory.rs (5)
130-136: Consider adding documentation for string conversion failure casesThe String conversion can fail not just due to null pointers but also due to invalid UTF-16 sequences. This should be documented for API users.
+/// Converts the remote pointer to a String. +/// +/// # Errors +/// +/// Returns `E_POINTER` if the pointer is null. +/// Returns an error if the underlying UTF-16 sequence is invalid. #[inline(always)] fn try_from(value: RemotePointer<u16>) -> Result<Self, Self::Error> {
141-143: Add safety documentation for pointer castingThe pointer casting in
as_mut_pwstr_ptrinvolves multiple conversions that require careful handling by the caller.+/// Returns a mutable pointer suitable for COM APIs expecting PWSTR*. +/// +/// # Safety +/// +/// The caller must ensure that: +/// - The pointer remains valid for the duration of the COM call +/// - The COM API will not write beyond the allocated memory #[inline(always)] pub fn as_mut_pwstr_ptr(&mut self) -> *mut PWSTR {
272-280: Consider caching PWSTR array conversionsCreating new Vec on each call to
as_pwstr_arraycould be inefficient for frequently accessed data.Consider adding a cached version that updates only when the underlying data changes:
pub struct LocalPointer<T: Sized> { inner: Option<Box<T>>, pwstr_cache: Option<Vec<windows::core::PWSTR>>, // Add cache field }
213-214: Consider reusing string encoding logicThe UTF-16 encoding logic is duplicated between the
From<&str>andFrom<&String>implementations.+fn encode_to_utf16_vec(s: &str) -> Vec<u16> { + s.encode_utf16().chain(Some(0)).collect() +} impl From<&str> for LocalPointer<Vec<u16>> { #[inline(always)] fn from(s: &str) -> Self { - Self::new(Some(s.encode_utf16().chain(Some(0)).collect())) + Self::new(Some(encode_to_utf16_vec(s))) } } impl From<&String> for LocalPointer<Vec<u16>> { #[inline(always)] fn from(s: &String) -> Self { - Self::new(Some(s.encode_utf16().chain(Some(0)).collect())) + Self::new(Some(encode_to_utf16_vec(s))) } }Also applies to: 220-221
6-310: Overall assessment: Well-structured memory management implementationThe implementation provides a robust foundation for managing both COM-allocated and local memory with proper safety considerations. The use of generics and trait implementations makes it flexible and reusable.
Some notable strengths:
- Clear separation between remote (COM) and local memory management
- Proper implementation of Drop traits for cleanup
- Comprehensive null pointer handling
- Good use of Rust's type system for safety
Consider adding the following improvements in future iterations:
- Add comprehensive documentation for unsafe operations
- Consider implementing
Send/Synctraits where appropriate for concurrent usage- Add unit tests to verify memory management behavior
opc_da/src/client/unified/actor/mod.rs (3)
11-16: Consider a more descriptive function nameThe function
mb_errorcould be renamed to something more descriptive likeconvert_mailbox_errorormap_actor_mailbox_errorto better indicate its purpose.-fn mb_error(err: actix::MailboxError) -> windows::core::Error { +fn convert_mailbox_error(err: actix::MailboxError) -> windows::core::Error {
18-23: Add documentation for the macro and consider renamingThe macro would benefit from documentation explaining its purpose and usage. Additionally, if you rename the
mb_errorfunction, update the macro name accordingly.#[macro_export] +/// Converts an actix::MailboxError to windows::core::Error in a Result. +/// +/// # Example +/// ``` +/// let result = send_message_to_actor().map_err(convert_mailbox_error)?; +/// ``` macro_rules! mb_error {
1-23: Consider documenting COM initialization requirementsSince this module implements actor-based functionality and there are concerns about COM initialization (COINIT_MULTITHREADED), consider adding documentation about the threading model requirements and initialization prerequisites.
This is particularly important as the actor-based architecture needs to work correctly with COM objects in both MTA and STA scenarios.
opc_da/src/client/unified/actor/runtime.rs (1)
11-13: Consider propagating errors instead of panicking increate_runtimeUsing
expectincreate_runtimewill cause a panic if the runtime creation fails. If failure to create the runtime is recoverable or should be handled by the caller, consider returning aResultinstead.You can modify the function to return a
Result:-pub fn create_runtime() -> tokio::runtime::Runtime { - try_create_runtime().expect("Failed to create runtime") +pub fn create_runtime() -> std::io::Result<tokio::runtime::Runtime> { + try_create_runtime() }Then handle the error in the calling code as appropriate.
opc_da/src/client/traits/server.rs (2)
44-58: Minimize unsafe block scope.The unsafe block currently encompasses more code than necessary. Consider wrapping only the COM call in the unsafe block.
- unsafe { self.interface()?.AddGroup( + unsafe { group_name, windows::Win32::Foundation::BOOL::from(active), update_rate, client_handle, &time_bias, &percent_deadband, locale_id, &mut group_server_handle, &mut revised_percent_deadband, &opc_da_bindings::IOPCItemMgt::IID, &mut group, - )?; + })?;
69-78: Consider using map_or for more idiomatic Rust.The match expression could be simplified using
map_or.- match status { - Some(status) => Ok(*status), - None => Err(windows::core::Error::new( - windows::Win32::Foundation::E_FAIL, - "Failed to get server status", - )), - } + status.map_or( + Err(windows::core::Error::new( + windows::Win32::Foundation::E_FAIL, + "Failed to get server status", + )), + |status| Ok(*status) + )opc_da/src/client/tests.rs (1)
16-18: Enhance panic messages with more context.The panic messages could be more informative by including actual values and context.
- panic!("No servers found"); + panic!("No OPC DA servers found in the system"); - .expect("No server found") - .expect("Failed to get server id"); + .expect("Failed to get first server from the list") + .expect("Failed to extract server ID from server info");Also applies to: 23-24
opc_da/src/client/v2/mod.rs (1)
86-95: LGTM! Consider adding documentation.The Group struct is well-implemented. Consider adding documentation to describe the purpose of each field and their optional nature.
+/// Represents an OPC DA group with its associated COM interfaces. pub struct Group { + /// Interface for managing items within the group. pub(crate) item_mgt: opc_da_bindings::IOPCItemMgt, + /// Interface for managing group state. pub(crate) group_state_mgt: opc_da_bindings::IOPCGroupStateMgt, // ... add docs for other fields }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/tests.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/unified/actor/client.rs(1 hunks)opc_da/src/client/unified/actor/mod.rs(1 hunks)opc_da/src/client/unified/actor/runtime.rs(1 hunks)opc_da/src/client/unified/actor/tests.rs(1 hunks)opc_da/src/client/unified/client.rs(1 hunks)opc_da/src/client/unified/group.rs(1 hunks)opc_da/src/client/unified/guard.rs(1 hunks)opc_da/src/client/unified/mod.rs(1 hunks)opc_da/src/client/unified/server.rs(1 hunks)opc_da/src/client/v1/mod.rs(1 hunks)opc_da/src/client/v2/mod.rs(1 hunks)opc_da/src/client/v3/mod.rs(1 hunks)opc_da/src/def.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opc_da/src/client/unified/actor/client.rs
🧰 Additional context used
📓 Learnings (1)
opc_da/src/client/memory.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🔇 Additional comments (17)
opc_da/src/client/unified/mod.rs (1)
1-6: LGTM! Well-structured module organization.
The module structure demonstrates good separation of concerns, with clear boundaries between different functionalities (actor, client, group, guard, iter, server). This organization will facilitate maintenance and future extensions.
opc_da/src/client/unified/group.rs (2)
1-4: LGTM! Well-organized imports following the bridge pattern.
The imports are clearly structured and demonstrate good separation of concerns between different versions and trait implementations.
22-40: 🛠️ Refactor suggestion
Refactor to reduce code duplication in error handling
Similar to the past review comment, consider implementing a helper function for error handling to improve code maintainability.
opc_da/src/client/unified/actor/mod.rs (1)
1-10: Consider re-exporting the server module for consistency
The server module is declared but not re-exported like the other modules. If this is intentional, consider documenting why. If not, consider adding pub use server::*; for consistency.
✅ Verification successful
The server module's private status is correct
The server module only contains an Actor implementation for an existing type and doesn't define any public items of its own. Not re-exporting it is the correct design choice as all the public types it uses are already available through other modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if server module contains public items that should be re-exported
ast-grep --pattern 'pub $$$' server.rs
Length of output: 97
Script:
#!/bin/bash
# Let's first find the correct path to the server module
fd server.rs --full-path -t f | grep -i "opc_da/src/client/unified/actor"
# Then check its contents for public items
find . -type f -path "*opc_da/src/client/unified/actor/server.rs" -exec ast-grep --pattern 'pub $$$' {} \;
Length of output: 227
Script:
#!/bin/bash
# Let's examine the actual contents of the server module
cat opc_da/src/client/unified/actor/server.rs
Length of output: 172
opc_da/src/client/unified/client.rs (3)
14-20: Code structure for get_version_id is sound
The implementation correctly returns the appropriate GUID for each client version, ensuring proper identification of OPC DA server versions.
22-48: Proper error handling in get_servers method
The method effectively retrieves the server list and handles potential errors during COM interactions. The use of map_err ensures that errors are wrapped with meaningful messages.
50-66: Creation of servers is correctly implemented
The create_server method appropriately instantiates the server objects based on the client version, with proper casting and error handling.
opc_da/src/client/v1/mod.rs (2)
14-24: Consider adding COM initialization check in Server's TryFrom implementation
Ensure that COM is initialized before casting COM interfaces. Adding a check can help prevent undefined behavior if COM is not initialized.
58-80: 🛠️ Refactor suggestion
Implement Drop trait for Group to ensure proper resource cleanup
The Group struct manages several COM interfaces that need to be released to prevent resource leaks. Implementing the Drop trait for Group will ensure that all COM interfaces are correctly released when the Group instance goes out of scope.
Apply this diff to implement Drop for Group:
impl Drop for Group {
fn drop(&mut self) {
unsafe {
self.data_object.Release();
self.async_io.Release();
self.sync_io.Release();
if let Some(ref public_group) = self.public_group_state_mgt {
public_group.Release();
}
self.group_state_mgt.Release();
self.item_mgt.Release();
}
}
}opc_da/src/client/v3/mod.rs (3)
99-103: Consider refactoring to reduce code duplication in trait implementations
The interface methods in your trait implementations for Group have similar patterns that can be refactored to reduce code duplication. Consider using a macro to generate these methods automatically.
You could define a macro like this:
macro_rules! impl_interface_trait {
($trait_name:ident, $interface_type:path, $field_name:ident) => {
impl $trait_name for Group {
fn interface(&self) -> windows::core::Result<&$interface_type> {
Ok(&self.$field_name)
}
}
};
}Then use the macro to implement the traits:
impl_interface_trait!(ItemMgtTrait, opc_da_bindings::IOPCItemMgt, item_mgt);
impl_interface_trait!(GroupStateMgtTrait, opc_da_bindings::IOPCGroupStateMgt, group_state_mgt);
impl_interface_trait!(GroupStateMgt2Trait, opc_da_bindings::IOPCGroupStateMgt2, group_state_mgt2);
impl_interface_trait!(SyncIoTrait, opc_da_bindings::IOPCSyncIO, sync_io);
impl_interface_trait!(SyncIo2Trait, opc_da_bindings::IOPCSyncIO2, sync_io2);
impl_interface_trait!(AsyncIo2Trait, opc_da_bindings::IOPCAsyncIO2, async_io2);
impl_interface_trait!(AsyncIo3Trait, opc_da_bindings::IOPCAsyncIO3, async_io3);
impl_interface_trait!(ItemDeadbandMgtTrait, opc_da_bindings::IOPCItemDeadbandMgt, item_deadband_mgt);Also applies to: 105-109, 111-115, 117-121, 123-127, 129-133, 135-139, 141-145
147-156: Ensure consistent error handling for unsupported interfaces
In the ItemSamplingMgtTrait implementation, you manually handle the E_NOTIMPL error for the unsupported interface. To maintain consistency and reduce duplication, consider using the same refactoring approach for error handling as suggested above.
You could extend the macro to handle optional interfaces:
macro_rules! impl_optional_interface_trait {
($trait_name:ident, $interface_type:path, $field_name:ident, $interface_str:expr) => {
impl $trait_name for Group {
fn interface(&self) -> windows::core::Result<&$interface_type> {
self.$field_name.as_ref().ok_or_else(|| {
windows::core::Error::new(
windows::Win32::Foundation::E_NOTIMPL,
concat!($interface_str, " not supported"),
)
})
}
}
};
}Then implement the trait:
impl_optional_interface_trait!(
ItemSamplingMgtTrait,
opc_da_bindings::IOPCItemSamplingMgt,
item_sampling_mgt,
"IOPCItemSamplingMgt"
);1-164: Overall implementation looks good
The Server and Group structs and their trait implementations are well-defined, and the code is structured effectively to encapsulate the necessary COM interfaces.
opc_da/src/client/unified/server.rs (1)
1-7: Enum Server implementation is correct
The Server enum correctly encapsulates the different server versions (V1, V2, V3), allowing for flexible handling of various implementations.
opc_da/src/client/unified/actor/tests.rs (1)
5-13: Test test_actor is correctly implemented
The test successfully verifies that the ClientActor can be created and that it retrieves a non-empty list of servers, ensuring the actor's functionality works as expected.
opc_da/src/client/unified/actor/runtime.rs (1)
3-9: Runtime creation function is correctly implemented
The try_create_runtime function builds a multi-threaded Tokio runtime with appropriate initialization and uninitialization handlers using Guard, ensuring proper COM threading model management.
opc_da/src/client/traits/server.rs (1)
5-8: LGTM! Well-designed trait definition.
The trait is well-structured with appropriate use of generics and follows Rust best practices for COM interface access.
opc_da/src/client/v2/mod.rs (1)
10-84: LGTM! Well-structured Server implementation.
The Server struct and its trait implementations are well-designed with:
- Proper handling of optional COM interfaces
- Clear error messages for unsupported features
- Good separation of concerns
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (17)
opc_da/src/client/memory.rs (4)
6-10: Add documentation for public APIThis struct is part of the public API and handles memory management. Consider adding documentation comments to explain its purpose, safety guarantees, and usage examples.
Add documentation like this:
+/// A safe wrapper around a COM-allocated array of type T. +/// +/// # Safety +/// The pointer is automatically freed using `CoTaskMemFree` when dropped. +/// +/// # Example +/// ``` +/// let mut array = RemoteArray::<i32>::new(5); +/// // Use array... +/// ``` #[derive(Debug, Clone, PartialEq)] pub struct RemoteArray<T: Sized> { pointer: *mut T, len: u32, }
86-89: Document the purpose of#[repr(transparent)]The
repr(transparent)attribute is used here but its purpose isn't documented. Add documentation explaining why this representation is necessary.Add documentation:
+/// A pointer to COM-allocated memory containing a single value. +/// +/// Uses `#[repr(transparent)]` to ensure the struct has the same ABI layout +/// as the inner pointer, allowing for FFI compatibility. #[repr(transparent)] pub struct RemotePointer<T: Sized> { inner: *mut T, }
212-214: Optimize string encoding for UTF-16 conversionThe current implementation allocates a new vector and then chains a null terminator. This could be optimized by pre-allocating the correct size.
Consider this optimization:
#[inline(always)] fn from(s: &str) -> Self { - Self::new(Some(s.encode_utf16().chain(Some(0)).collect())) + let mut vec = Vec::with_capacity(s.len() + 1); + vec.extend(s.encode_utf16()); + vec.push(0); + Self::new(Some(vec)) }Apply similar optimization to the String implementation.
Also applies to: 219-221
272-280: Consider caching PWSTR/PCWSTR arraysThe
as_pwstr_arrayandas_pcwstr_arraymethods create new vectors on each call. For repeated calls, consider caching the results.Consider adding caching:
pub struct LocalPointer<Vec<Vec<u16>>> { inner: Option<Box<Vec<Vec<u16>>>>, + pwstr_cache: Option<Vec<windows::core::PWSTR>>, + pcwstr_cache: Option<Vec<windows::core::PCWSTR>>, }Then update the array methods to use and update the cache.
Also applies to: 283-291
opc_da/src/client/unified/actor/mod.rs (1)
11-16: Consider using a more specific HRESULT code inmb_errorCurrently, the
mb_errorfunction usesE_FAILas a generic error code. Using a more specific HRESULT code related to the failure reason can provide better context for error handling and debugging.opc_da/src/client/unified/client.rs (1)
42-44: Include original error details when mapping errorsWhen mapping errors in
.map_err, consider including the original error details to provide more context for debugging.Apply this diff to improve error reporting:
-.map_err(|e| { - windows::core::Error::new(e.code(), "Failed to enumerate server classes") -}) +.map_err(|e| { + windows::core::Error::new( + e.code(), + format!("Failed to enumerate server classes: {:?}", e) + ) +})opc_da/src/client/unified/actor/tests.rs (1)
6-13: Consider using#[actix::test]and improving error handlingWhile the test is functional, consider these improvements:
- Use
#[actix::test]instead of manually creating the system- Add more descriptive error messages
- Consider adding cleanup/teardown of resources
Here's a suggested improvement:
-#[test] -fn test_actor() { - actix::System::with_tokio_rt(create_runtime).block_on(async { - let client = ClientActor::new(Client::V2).expect("Failed to create client actor"); - let servers = client.get_servers().await.expect("Failed to get servers"); - - assert!(!servers.is_empty()); - }); -} +#[actix::test] +async fn test_actor() { + let client = ClientActor::new(Client::V2) + .expect("Failed to create ClientActor with V2 client implementation"); + + let servers = client.get_servers() + .await + .expect("Failed to retrieve server list from OPC DA server"); + + assert!(!servers.is_empty(), "Expected at least one server in the list"); + + // Clean up resources + drop(client); +}opc_da/src/client/unified/actor/runtime.rs (2)
3-9: Add documentation for public functionsThese public functions are crucial for runtime initialization but lack documentation explaining their purpose, requirements, and behavior.
Add documentation comments:
+/// Attempts to create a multi-threaded Tokio runtime with COM initialization. +/// +/// This function creates a runtime that: +/// - Uses multiple threads for task execution +/// - Enables all Tokio features +/// - Initializes COM on thread start +/// - Uninitializes COM on thread stop +/// +/// # Errors +/// Returns an error if runtime creation fails or COM initialization fails pub fn try_create_runtime() -> std::io::Result<tokio::runtime::Runtime> {
11-13: Consider more specific error handlingThe
expectmessage could be more descriptive about what might have caused the failure.- try_create_runtime().expect("Failed to create runtime") + try_create_runtime().expect("Failed to create Tokio runtime with COM initialization. Ensure COM is available on this system")opc_da/src/client/unified/group.rs (2)
6-10: Add documentation for the Group enumThe
Groupenum is a key type but lacks documentation explaining its purpose and version differences.Add documentation:
+/// Represents an OPC DA group across different versions of the protocol. +/// +/// This enum unifies the handling of groups from OPC DA versions 1.0, 2.0, and 3.0, +/// allowing for version-agnostic group operations while maintaining version-specific +/// functionality when needed. pub enum Group { V1(v1::Group), V2(v2::Group), V3(v3::Group), }
26-27: Add error context for bridge conversionsThe bridge conversion and native conversion errors could benefit from more context.
- let mut bridge = items.into_bridge(); - let (results, errors) = self.item_mgt().add_items(&bridge.to_native()?)?; + let mut bridge = items.into_bridge(); + let native_items = bridge.to_native() + .map_err(|e| windows::core::Error::new( + e.code(), + format!("Failed to convert items to native format: {}", e) + ))?; + let (results, errors) = self.item_mgt() + .add_items(&native_items) + .map_err(|e| windows::core::Error::new( + e.code(), + format!("Failed to add items to group: {}", e) + ))?;opc_da/src/client/traits/server.rs (2)
28-67: Consider minimizing unsafe block scope.The unsafe block currently encompasses more code than necessary. It could be reduced to only wrap the COM call to
AddGroup.Consider this structure:
fn add_group( &self, name: &str, active: bool, client_handle: u32, update_rate: u32, locale_id: u32, time_bias: i32, percent_deadband: f32, ) -> windows::core::Result<Group> { let mut group = None; let mut group_server_handle = 0u32; let group_name = LocalPointer::from(name); let group_name = group_name.as_pcwstr(); let mut revised_percent_deadband = 0; - unsafe { - self.interface()?.AddGroup( + unsafe { self.interface()?.AddGroup( group_name, windows::Win32::Foundation::BOOL::from(active), update_rate, client_handle, &time_bias, &percent_deadband, locale_id, &mut group_server_handle, &mut revised_percent_deadband, &opc_da_bindings::IOPCItemMgt::IID, &mut group, - )?; + )? }; match group { None => Err(windows::core::Error::new( windows::Win32::Foundation::E_POINTER, "Failed to add group, returned null", )), Some(group) => self.create_group(group), - } }
69-78: Add documentation for the get_status method.Consider adding documentation to describe the method's purpose, safety requirements, and return value interpretation.
Add documentation like this:
/// Retrieves the current status of the OPC server. /// /// # Safety /// This method is unsafe because it calls into COM interfaces. /// The caller must ensure that the COM server is properly initialized. /// /// # Returns /// Returns the server status structure containing information about the server state, /// vendor info, and time information. fn get_status(&self) -> windows::core::Result<opc_da_bindings::tagOPCSERVERSTATUS>opc_da/src/client/tests.rs (2)
8-35: Enhance error messages for better debugging.The panic messages could be more descriptive to help identify the root cause of failures.
Consider adding more context:
- .expect("Failed to create guard"); + .expect("Failed to create COM guard with V2 client"); - .expect("Failed to get servers") + .expect("Failed to enumerate available OPC DA servers"); - .expect("No server found") + .expect("Failed to get first server from the enumerated list"); - panic!("Expected V2 server"), + panic!("Server type mismatch: expected V2 server, got different version"),
71-73: Use meaningful test parameters.The group parameters (update rate, locale, etc.) are all set to 0, which might not exercise the functionality properly.
Consider using more realistic values:
- .add_group("test", true, 0, 0, 0, 0, 0.0) + .add_group( + "test_group", + true, + 1, // client handle + 1000, // update rate in milliseconds + 1033, // locale ID (e.g., en-US) + 0, // time bias + 0.1 // percent deadband + )opc_da/src/client/v2/mod.rs (1)
10-17: Add documentation for the Server struct.Consider adding documentation to describe the purpose of each field and their optionality.
Add documentation like this:
/// Represents an OPC DA v2 server implementation. /// /// This struct encapsulates the COM interfaces required for interacting with an OPC DA v2 server. /// Some interfaces are optional and may not be supported by all servers. pub struct Server { /// The main OPC server interface pub(crate) server: opc_da_bindings::IOPCServer, /// Common interface for server configuration pub(crate) common: opc_da_bindings::IOPCCommon, // ... document other fields }opc_da/src/client/v1/mod.rs (1)
38-43: Refactor error handling to reduce code duplicationThe error handling in the
interfacemethods for optional interfaces is repetitive. Consider creating a helper function to generate consistentE_NOTIMPLerrors, improving maintainability.Define a helper function:
fn interface_not_supported(interface_name: &str) -> windows::core::Error { windows::core::Error::new( windows::Win32::Foundation::E_NOTIMPL, format!("{} not supported", interface_name), ) }Update your methods:
self.server_public_groups .as_ref() .ok_or_else(|| interface_not_supported("IOPCServerPublicGroups"))Also applies to: 49-55, 96-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/tests.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/unified/actor/client.rs(1 hunks)opc_da/src/client/unified/actor/mod.rs(1 hunks)opc_da/src/client/unified/actor/runtime.rs(1 hunks)opc_da/src/client/unified/actor/tests.rs(1 hunks)opc_da/src/client/unified/client.rs(1 hunks)opc_da/src/client/unified/group.rs(1 hunks)opc_da/src/client/unified/guard.rs(1 hunks)opc_da/src/client/unified/mod.rs(1 hunks)opc_da/src/client/unified/server.rs(1 hunks)opc_da/src/client/v1/mod.rs(1 hunks)opc_da/src/client/v2/mod.rs(1 hunks)opc_da/src/client/v3/mod.rs(1 hunks)opc_da/src/def.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opc_da/src/client/unified/actor/client.rs
🧰 Additional context used
📓 Learnings (1)
opc_da/src/client/memory.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🔇 Additional comments (12)
opc_da/src/client/memory.rs (1)
1-310: Overall implementation looks solid! 👍
The memory management implementation is well-structured with proper COM memory handling and safety checks. The suggestions above are mainly about documentation and optimizations, but the core functionality is sound.
opc_da/src/client/unified/guard.rs (1)
15-17:
Fix error handling in Guard::new()
The call to Self::try_initialize().ok()? may not properly handle errors because .ok()? converts the HRESULT into an Option<()>, which can cause the error code to be ignored. Instead, you should use .ok() without the question mark to convert the HRESULT into a Result<(), Error>, and then use the ? operator to propagate the error correctly.
Apply this diff to fix the error handling:
- Self::try_initialize().ok()?;
+ Self::try_initialize()?;opc_da/src/client/unified/client.rs (1)
22-25: Verify that CLSIDFromProgID retrieves the correct CLSID
Ensure that the call to CLSIDFromProgID(w!("OPC.ServerList.1")) retrieves the correct CLSID for the OPC Server List. Any mismatch could cause issues when creating the server list instance.
Run the following script to verify the CLSID:
✅ Verification successful
CLSIDFromProgID("OPC.ServerList.1") usage is verified to be correct
The code is using the correct ProgID "OPC.ServerList.1" to obtain the CLSID. This is verified by:
- The example code in the repository's README demonstrates the same exact usage
- This is a standard COM registration for the OPC Server List component
- The code follows the same pattern used in the official OPC DA specification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that CLSIDFromProgID("OPC.ServerList.1") returns the expected CLSID.
# Test: Retrieve the CLSID and display it. Expect: CLSID matches the one used in the code.
powershell -Command "New-Object -ComObject OPC.ServerList.1 | Get-CLSID"
Length of output: 175
Script:
#!/bin/bash
# Let's analyze the codebase to understand the OPC Server List implementation
# Search for any constants, tests, or documentation related to the CLSID
# Search for any hardcoded CLSID or OPC Server List related constants
rg -i "OPC.ServerList" --type rust -A 3 -B 3
# Look for any test files that might verify the CLSID
fd -e rs test -X rg -i "serverlist|clsid" --type rust
# Check if there's any documentation about the CLSID
fd "README|GUIDE|DOC" -X cat {}
Length of output: 2258
opc_da/src/def.rs (1)
125-133: Ensure safe handling of raw pointers in ItemResult
When constructing ItemResult from the native tagOPCITEMRESULT, ensure that the raw pointer pBlob and the dwBlobSize are correctly managed to prevent undefined behavior.
Run the following script to check for correct usage of RemoteArray:
✅ Verification successful
Let me gather more information about the RemoteArray implementation to verify its safety.
Let me search for the implementation of RemoteArray::from_raw specifically.
Let me try a different approach to find the implementation.
Based on the implementation found, I can now generate the final response.
Raw pointer handling in RemoteArray is safely implemented
The RemoteArray::from_raw implementation is safe because:
- It properly wraps the raw pointer and size in a struct that implements Drop
- The Drop implementation correctly frees the memory using
CoTaskMemFreewhen the array is dropped - The struct maintains the size information alongside the pointer, preventing buffer overruns
- Memory access is properly guarded with unsafe blocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for implementations of `RemoteArray::from_raw` to verify correct usage.
# Test: Look for instances of `RemoteArray::from_raw`. Expect: Proper handling of pointers and sizes.
rg -A 3 "RemoteArray::from_raw"
Length of output: 196
Script:
#!/bin/bash
# Search for RemoteArray definition and implementation
ast-grep --pattern 'struct RemoteArray { $$$ }'
# Also search for any methods or implementations related to RemoteArray
rg -B 2 -A 5 "RemoteArray"
Length of output: 45960
Script:
#!/bin/bash
# Search for RemoteArray::from_raw implementation
ast-grep --pattern 'impl$$$RemoteArray$$$from_raw$$$'
# Also search for any Drop implementation details
ast-grep --pattern 'impl$$$Drop for RemoteArray$$$'
Length of output: 109
Script:
#!/bin/bash
# Search for from_raw method in RemoteArray implementation
rg -B 2 -A 5 "fn from_raw" opc_da/src/client/memory.rs
# Also search for any unsafe implementations in the same file
rg -B 2 -A 5 "unsafe" opc_da/src/client/memory.rs
Length of output: 1116
opc_da/src/client/unified/group.rs (1)
22-40: Consider using const generics for version handling
The current implementation uses runtime matching for version handling. Consider using const generics to make version selection compile-time.
Example approach:
pub struct Group<const VERSION: u8> {
inner: VersionedGroup<VERSION>,
}
impl<const VERSION: u8> Group<VERSION> {
pub fn add_items(&self, items: Vec<ItemDef>) -> windows::core::Result<Vec<windows::core::Result<ItemResult>>> {
// Implementation remains similar but without runtime matching
}
}Also, regarding error handling duplication mentioned in previous reviews:
The suggestion to create a helper function for interface error handling is still applicable here.
opc_da/src/client/traits/server.rs (2)
5-8: LGTM! Well-designed trait definition.
The trait is properly generic over the Group type and provides a clean interface for server operations.
97-97:
Handle potential cast failure explicitly.
The cast() operation could fail silently. Consider adding explicit error handling.
Replace with:
- enumerator.cast()
+ enumerator.ok_or_else(|| windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Failed to create group enumerator",
+ ))?.cast()Likely invalid or redundant comment.
opc_da/src/client/v2/mod.rs (1)
86-95: Clarify async IO interface requirements.
The struct requires IOPCAsyncIO2 but makes IOPCAsyncIO optional. Consider documenting why both are needed and their relationship.
Let's verify the interface requirements:
✅ Verification successful
Let me gather more information about the version differences and interface requirements.
Based on the gathered information, I can now provide a final response:
IOPCAsyncIO2 is required as it's the primary async interface for OPC DA v2
The implementation is correct. Here's why:
- OPC DA v1 uses
IOPCAsyncIO(required) - OPC DA v2 uses both interfaces:
IOPCAsyncIO(optional, for backward compatibility)IOPCAsyncIO2(required, primary async interface for v2)
- OPC DA v3 uses
IOPCAsyncIO2andIOPCAsyncIO3(both required)
The optional IOPCAsyncIO in v2 implementation maintains compatibility with v1 clients while ensuring v2 functionality through the required IOPCAsyncIO2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or usage patterns of IOPCAsyncIO and IOPCAsyncIO2
rg -A 5 "IOPCAsyncIO2?" opc_da/src/
Length of output: 6541
Script:
#!/bin/bash
# Search for documentation about OPC DA versions and their interfaces
rg -B 2 -A 5 "v1|v2|v3" opc_da/src/client/ --type rust
# Search for any implementation details or comments about async IO interfaces
rg -B 2 -A 5 "async.*io" opc_da/src/client/ --type rust --ignore-case
Length of output: 12076
opc_da/src/client/v1/mod.rs (2)
14-24:
Add COM initialization check in Server::try_from
Before casting interfaces in the try_from method, ensure that COM has been initialized. This prevents undefined behavior when interacting with COM objects.
You can add a COM initialization check like this:
fn try_from(value: windows::core::IUnknown) -> windows::core::Result<Self> {
// Verify COM initialization
unsafe {
let mut apt_type = windows::Win32::System::Com::APTTYPE::default();
let mut apt_reference = windows::Win32::System::Com::APTTYPEQUALIFIER::default();
if windows::Win32::System::Com::CoGetApartmentType(&mut apt_type, &mut apt_reference).is_err() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::CO_E_NOTINITIALIZED,
"COM not initialized",
));
}
}
Ok(Self {
server: value.cast()?,
server_public_groups: value.cast().ok(),
browse_server_address_space: value.cast().ok(),
})
}58-80: 🛠️ Refactor suggestion
Implement Drop trait for proper COM interface cleanup
The Group struct holds several COM interfaces that should be released when the struct is dropped to prevent resource leaks. Implementing the Drop trait ensures that all interfaces are properly released.
Here's how you can implement the Drop trait:
impl Drop for Group {
fn drop(&mut self) {
unsafe {
if let Some(ref public_group) = self.public_group_state_mgt {
public_group.Release();
}
self.data_object.Release();
self.async_io.Release();
self.sync_io.Release();
self.group_state_mgt.Release();
self.item_mgt.Release();
}
}
}opc_da/src/client/unified/server.rs (1)
3-7: Unified Server enum effectively encapsulates server versions
The Server enum provides a clean and extensible way to handle different server versions, enhancing code readability and maintainability.
opc_da/src/client/unified/mod.rs (1)
1-13: Module structure is organized and exports are appropriately managed
The unified module's organization with clear sub-modules and re-exports allows for straightforward access to its components, improving usability.
…or handling in blob size conversion, and add new methods for public group state management and keep-alive time settings
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
opc_da/src/client/traits/common.rs (2)
13-13: LGTM! Well-designed interface method.The method signature correctly returns a Result-wrapped reference to IOPCCommon, following COM interface access patterns. This design allows implementors to handle interface acquisition while the trait provides default implementations for common operations.
Consider documenting the thread safety requirements for implementors, especially given the PR's context about
COINIT_MULTITHREADEDusage.
75-78: Consider adding input validation for the client name.While the implementation is correct and safely handles string conversion, consider adding validation for the client name parameter (e.g., length limits, invalid characters).
fn set_client_name(&self, name: &str) -> windows::core::Result<()> { + // Validate input + if name.is_empty() || name.len() > 255 { + return Err(windows::core::Error::new( + windows::core::HRESULT(0x80070057), // E_INVALIDARG + "Client name must be between 1 and 255 characters", + )); + } let name = LocalPointer::from(name); unsafe { self.interface()?.SetClientName(name.as_pcwstr()) } }opc_da/src/client/traits/connection_point_container.rs (3)
4-9: Consider enhancing trait documentation with safety notes.While the documentation clearly explains the purpose, it would be beneficial to add:
- Safety considerations for COM operations
- Thread-safety implications
- Lifetime requirements for the COM objects
Example addition:
/// COM connection point container functionality. /// /// Provides methods to establish connections between event sources /// and event sinks in the OPC COM architecture. Used primarily for /// handling asynchronous callbacks. +/// +/// # Safety +/// This trait involves unsafe COM operations. Implementors must ensure: +/// - COM is properly initialized with appropriate threading model +/// - COM objects are properly released +/// - Connection points are properly disconnected when no longer needed
10-13: Add documentation for the interface method.The
interface()method lacks documentation explaining its purpose, safety requirements, and potential error conditions.Add documentation:
+/// Returns the underlying IConnectionPointContainer interface. +/// +/// # Returns +/// A reference to the COM IConnectionPointContainer interface +/// +/// # Errors +/// Returns an error if the COM interface is not available fn interface( &self, ) -> windows::core::Result<&windows::Win32::System::Com::IConnectionPointContainer>;
23-26: Update example code with a concrete GUID.The example code references an undefined
GUID_CALLBACK_INTERFACE. Consider using a well-known GUID or providing context about where to obtain the appropriate GUID.Example improvement:
/// ```no_run /// use windows::core::GUID; -/// let cp = container.find_connection_point(&GUID_CALLBACK_INTERFACE); +/// // Example using the IOPCDataCallback interface GUID +/// let callback_guid = GUID::from("39c13a70-011e-11d0-9675-0020afd8adb3"); +/// let cp = container.find_connection_point(&callback_guid)?; /// ```opc_da/src/client/unified/guard.rs (1)
79-86: Ensure proper error propagation intry_initializemethodUsing
.ok()on the result ofCoInitializeExmay obscure the specific error code on failure. Instead, consider using theResultdirectly to preserve detailed error information for better diagnostics.Apply this diff to improve error handling:
unsafe { windows::Win32::System::Com::CoInitializeEx( None, windows::Win32::System::Com::COINIT_MULTITHREADED, ) -} -.ok() +}opc_da/src/client/traits/item_io.rs (3)
54-63: Uselenvariable instead of recalculatingitem_ids.len()For consistency and to ensure that the length used is the validated value, use the
lenvariable instead of recomputingitem_ids.len() as u32in theReadcall.Apply this diff to use
len:unsafe { self.interface()?.Read( - item_ids.len() as u32, + len, item_ptrs.as_ptr(), max_age.as_ptr(), values.as_mut_ptr(), qualities.as_mut_ptr(), timestamps.as_mut_ptr(), errors.as_mut_ptr(), )?; }
44-46: Handle potential emptyitem_idsgracefullyWhile you check for empty
item_idsearlier, consider adding a specific error message ifitem_idsis empty to provide clearer feedback to the caller.For example:
if item_ids.is_empty() || max_age.is_empty() || item_ids.len() != max_age.len() { return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "Invalid arguments - arrays must be non-empty and have same length", + "Invalid arguments - item_ids and max_age must be non-empty and have the same length", )); }
85-90: Ensure consistent error handling inwrite_vqtSimilar to the
readmethod, you might want to ensure that bothitem_idsanditem_vqtsare non-empty and provide a clear error message.Consider refining the error message for clarity:
if item_ids.is_empty() || item_vqts.is_empty() || item_ids.len() != item_vqts.len() { return Err(windows::core::Error::new( windows::Win32::Foundation::E_INVALIDARG, - "Invalid arguments - arrays must be non-empty and have same length", + "Invalid arguments - item_ids and item_vqts must be non-empty and have the same length", )); }opc_da/src/client/traits/group_state_mgt2.rs (1)
30-32: Add documentation about potential errorsThe documentation should mention potential error cases.
Add error documentation:
/// # Returns /// The current keep-alive time in milliseconds. A value of 0 indicates /// that keep-alive is disabled. + /// + /// # Errors + /// Returns an error if the COM call fails or if the server doesn't support + /// the IOPCGroupStateMgt2 interface. fn get_keep_alive(&self) -> windows::core::Result<u32> {opc_da/src/client/traits/group_state_mgt.rs (1)
60-90: Add validation for update_rate and percent_deadband parameters.The set_state method accepts parameters without validation, which could lead to invalid states.
Add parameter validation:
fn set_state( &self, update_rate: Option<u32>, active: Option<bool>, time_bias: Option<i32>, percent_deadband: Option<f32>, locale_id: Option<u32>, client_group_handle: Option<u32>, ) -> windows::core::Result<u32> { + if let Some(rate) = update_rate { + if rate == 0 { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + "update_rate cannot be zero", + )); + } + } + + if let Some(deadband) = percent_deadband { + if !(0.0..=100.0).contains(&deadband) { + return Err(windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + "percent_deadband must be between 0 and 100", + )); + } + }opc_da/src/client/traits/sync_io2.rs (1)
58-68: Add documentation for unsafe block.The unsafe block contains COM interface calls but lacks documentation explaining the safety guarantees.
Add safety documentation:
+ // SAFETY: + // - server_handles and max_age lengths are validated to be equal and non-zero + // - all pointers are valid for the duration of the call: + // - server_handles.as_ptr() is valid for len elements + // - max_age.as_ptr() is valid for len elements + // - RemoteArray pointers are allocated for len elements unsafe { self.interface()?.ReadMaxAge( len,opc_da/src/client/traits/browse.rs (1)
72-82: Consider using builder pattern for better API ergonomicsThe function has many parameters which makes it harder to use correctly. Consider implementing a builder pattern.
Example implementation:
pub struct BrowseOptions<'a> { pub item_id: &'a str, pub max_elements: u32, pub browse_filter: tagOPCBROWSEFILTER, pub element_name_filter: &'a str, pub vendor_filter: &'a str, pub return_all_properties: bool, pub return_property_values: bool, pub property_ids: &'a [u32], }opc_da/src/client/traits/item_sampling_mgt.rs (1)
4-8: Consider adding thread safety documentationSince this trait deals with sampling rates and buffering, it would be helpful to document any thread safety considerations.
Add a note about thread safety requirements, for example:
/// Item sampling management functionality (OPC DA 3.0). /// /// Provides methods to control sampling rates and buffering behavior /// for individual items in an OPC group. /// /// # Thread Safety /// /// The methods in this trait are safe to call from multiple threads /// when COM is initialized with COINIT_MULTITHREADED.opc_da/src/client/v2/mod.rs (1)
126-141: Consider adding documentation for optional interface supportThe
Groupimplementation includes several optional interfaces. Consider documenting which interfaces are required vs optional for OPC DA 2.0 compliance.Add documentation like:
/// An OPC DA 2.0 group implementation. /// /// Required interfaces: /// - IOPCItemMgt /// - IOPCGroupStateMgt /// - IOPCSyncIO /// - IOPCAsyncIO2 /// /// Optional interfaces: /// - IOPCPublicGroupStateMgt /// - IOPCAsyncIO /// - IDataObjectopc_da/src/client/memory.rs (1)
94-97: Add documentation forRemotePointermemory ownershipConsider adding documentation explaining the memory ownership model and when
CoTaskMemFreeis called.Add documentation like:
/// A pointer to memory allocated by COM's task allocator. /// /// The memory is automatically freed using `CoTaskMemFree` when the pointer is dropped. /// This type is particularly useful for COM interfaces that return allocated memory /// that needs to be freed by the caller. #[repr(transparent)] pub struct RemotePointer<T: Sized> { inner: *mut T, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
.vscode/settings.json(4 hunks)opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/mod.rs(1 hunks)opc_da/src/client/tests.rs(1 hunks)opc_da/src/client/traits/async_io.rs(1 hunks)opc_da/src/client/traits/async_io2.rs(1 hunks)opc_da/src/client/traits/async_io3.rs(1 hunks)opc_da/src/client/traits/browse.rs(1 hunks)opc_da/src/client/traits/browse_server_address_space.rs(1 hunks)opc_da/src/client/traits/common.rs(1 hunks)opc_da/src/client/traits/connection_point_container.rs(1 hunks)opc_da/src/client/traits/data_object.rs(1 hunks)opc_da/src/client/traits/group_state_mgt.rs(1 hunks)opc_da/src/client/traits/group_state_mgt2.rs(1 hunks)opc_da/src/client/traits/item_deadband_mgt.rs(1 hunks)opc_da/src/client/traits/item_io.rs(1 hunks)opc_da/src/client/traits/item_mgt.rs(1 hunks)opc_da/src/client/traits/item_properties.rs(1 hunks)opc_da/src/client/traits/item_sampling_mgt.rs(1 hunks)opc_da/src/client/traits/mod.rs(1 hunks)opc_da/src/client/traits/public_group_state_mgt.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)opc_da/src/client/traits/server_public_groups.rs(1 hunks)opc_da/src/client/traits/sync_io.rs(1 hunks)opc_da/src/client/traits/sync_io2.rs(1 hunks)opc_da/src/client/unified/guard.rs(1 hunks)opc_da/src/client/v1/mod.rs(1 hunks)opc_da/src/client/v2/mod.rs(1 hunks)opc_da/src/client/v3/mod.rs(1 hunks)opc_da/src/def.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- opc_da/src/client/mod.rs
- .vscode/settings.json
- opc_da/src/client/traits/mod.rs
- opc_da/src/client/traits/server_public_groups.rs
🧰 Additional context used
📓 Learnings (3)
opc_da/src/client/v3/mod.rs (2)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
opc_da/src/client/traits/item_mgt.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In `opc_da/src/client/traits/item_mgt.rs`, the `CreateEnumerator` method returns a `Result`, so there is no need to handle `None` values.
opc_da/src/client/memory.rs (1)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🔇 Additional comments (53)
opc_da/src/client/traits/common.rs (3)
1-12: LGTM! Well-organized imports and clear documentation.
The imports are properly scoped, and the trait documentation clearly explains its purpose and functionality.
62-66: LGTM! Safe string conversion with proper error handling.
The implementation safely handles COM string conversion using RemotePointer and properly propagates errors through the Result type.
28-53: LGTM! Safe memory management with proper error handling.
The locale management methods are well-implemented with:
- Properly contained unsafe blocks
- Safe memory management using RemoteArray
- Appropriate error handling without unwrap calls
Let's verify the locale handling implementation across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the locale handling implementation across the codebase:
Locale handling implementation is consistent and properly managed
The locale management methods are correctly implemented with:
- Safe memory management using
RemoteArray<T>for handling arrays of locale IDs - Proper error handling through
windows::core::Result - Consistent locale ID handling across both client and server traits
- Well-documented interfaces with clear LCID (Windows Locale ID) usage
Key verified locations:
opc_da/src/client/traits/common.rs: Core locale management implementationopc_da/src/server/traits/server.rs: Server-side locale handlingopc_da/src/def.rs: Locale ID integration in group definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent locale handling patterns
# Look for other locale-related implementations that might need similar safety patterns
# Search for locale-related methods
ast-grep --pattern 'fn $_(locale$_) { $$$ }'
# Search for RemoteArray usage with locales
rg -l 'RemoteArray.*locale'
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for locale-related code and RemoteArray usage
# Search for any locale-related code
rg -i "locale" -A 3 -B 3
# Search for RemoteArray usage to verify memory management patterns
rg "RemoteArray" -A 3 -B 3
# Search specifically for LCID-related code as it's used in the locale methods
rg "LCID" -A 3 -B 3
Length of output: 66914
opc_da/src/client/traits/connection_point_container.rs (2)
1-3: LGTM! Clean and focused imports.
The imports are minimal and correctly use the newer windows-rs crate path structure.
1-40: Verify COM initialization in async context.
Since this trait is used with Actix actors, we should verify that COM is properly initialized with the correct threading model across the codebase.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
COM initialization is properly handled through a dedicated guard mechanism
The codebase shows a well-structured approach to COM initialization:
- COM is initialized with
COINIT_MULTITHREADEDthrough a dedicated guard inunified/guard.rs - The trait is implemented for both v2 and v3 versions of Server and Group components
- Actix actors are implemented as thin wrappers using
SyncContextfor Server and regularContextfor Client, which is compatible with the COM threading model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check COM initialization patterns across the codebase
# Look for COM initialization calls
echo "Checking COM initialization patterns:"
rg -A 5 "CoInitializeEx|COINIT_"
# Look for Actix actor implementations using this trait
echo -e "\nChecking Actix actor usage:"
ast-grep --pattern 'struct $ACTOR {
$$$
}
impl Actor for $ACTOR {
$$$
}'
# Look for unsafe COM operations
echo -e "\nChecking unsafe COM operations:"
rg -A 5 "unsafe.*Com::"
Length of output: 3171
Script:
#!/bin/bash
# Let's check how the COM initialization guard is used in the codebase
# Check for guard usage
echo "Checking COM guard usage:"
rg -A 5 "guard::(try_)?initialize"
# Check for any actor implementations that might use the ConnectionPointContainerTrait
echo -e "\nChecking for ConnectionPointContainerTrait implementations:"
rg -A 5 "impl.*ConnectionPointContainerTrait"
# Look for actix-related COM operations
echo -e "\nChecking for actix-related COM operations:"
rg -A 5 "use actix::"
Length of output: 2717
Script:
#!/bin/bash
# Let's examine the actor implementations and COM initialization in the unified actors
# Check the Server actor implementation
echo "Checking Server actor implementation:"
rg -A 20 "impl Actor for Server" opc_da/src/client/unified/actor/server.rs
# Check the Client actor implementation
echo -e "\nChecking Client actor implementation:"
rg -A 20 "impl Actor for Client" opc_da/src/client/unified/actor/client.rs
# Check how COM guard is used in actor contexts
echo -e "\nChecking COM guard usage in actor contexts:"
rg -A 10 "guard" opc_da/src/client/unified/actor/
Length of output: 984
opc_da/src/client/traits/sync_io.rs (2)
24-55: read method is well-implemented with necessary input validations
The read method correctly checks for empty server_handles and ensures proper error handling. The usage of unsafe blocks is appropriate, and the COM interface is utilized correctly.
68-101: write method is well-implemented with necessary input validations
The write method properly validates that server_handles and values have the same length and are not empty. Error handling is correctly implemented, and the COM interactions are safely wrapped in unsafe blocks.
opc_da/src/client/unified/guard.rs (2)
21-26: Appropriate use of PhantomData to enforce thread affinity
Utilizing PhantomData<*const ()> effectively ensures that Guard<T> is neither Send nor Sync, maintaining COM's thread-affinity requirements.
61-65:
Potential issue with uninitializing COM in Drop implementation
Calling CoUninitialize in the Drop method may lead to unintended behavior if COM was initialized elsewhere in the application. Uninitializing COM could prematurely uninitialize COM for the entire thread, affecting other COM objects. Ensure that each CoInitializeEx call is balanced with a corresponding CoUninitialize, and consider managing COM initialization and uninitialization at the application level.
opc_da/src/client/traits/async_io3.rs (2)
24-49:
Add length check for empty server_handles in read_max_age method
Ensure that server_handles is not empty to prevent undefined behavior when calling the COM method with zero items.
Apply this diff to add the check:
if server_handles.len() != max_age.len() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"server_handles and max_age must have the same length",
));
+} else if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
}60-90:
Add length check for empty server_handles in write_vqt method
Ensure that server_handles is not empty to prevent issues with the COM call when no items are provided.
Apply this diff to add the check:
if server_handles.len() != values.len() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"server_handles and values must have the same length",
));
+} else if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
}opc_da/src/client/traits/browse_server_address_space.rs (2)
83-85:
Handle possible null pointer return in get_item_id method
The GetItemID method may return a null pointer if item_data_id is invalid. Attempting to convert a null pointer to a String will result in a runtime error. Please add a null check before converting.
Apply this diff to handle null pointers:
let output = unsafe { self.interface()?.GetItemID(item_data_id.as_pwstr())? };
+if output.is_null() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null pointer from GetItemID",
+ ));
+}
RemotePointer::from(output).try_into()100-101:
Check for null pointer in browse_access_paths method
The BrowseAccessPaths method may return a null pointer. Attempting to use a null IEnumString can lead to runtime errors. Please add a null check after calling the method.
Apply this diff to handle null pointers:
let item_id = LocalPointer::from_str(item_id)?;
-let result = unsafe { self.interface()?.BrowseAccessPaths(item_id.as_pwstr()) };
+let result = unsafe { self.interface()?.BrowseAccessPaths(item_id.as_pwstr()) }?;
+if result.is_null() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "Received null pointer from BrowseAccessPaths",
+ ));
+}
+Ok(result)opc_da/src/client/traits/item_io.rs (1)
100-106:
Validate the lengths of item_ids and item_vqts
Ensure that the lengths of item_ids and item_vqts do not exceed u32::MAX to prevent potential overflow issues when converting to u32.
Apply this diff to add a check:
let len = item_ids.len().try_into()?;
+if item_ids.len() != len as usize {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "Array length exceeds u32::MAX",
+ ));
+}Likely invalid or redundant comment.
opc_da/src/def.rs (3)
100-115:
Handle potential null pointer when blob is empty
When blob is empty, calling as_mut_array_ptr() may return a null pointer, which could cause undefined behavior if the COM API doesn't handle null pointers correctly. Ensure that you provide a valid pointer or handle the null case safely.
Apply this diff to handle empty blob:
Ok(Self::Native {
szAccessPath: self.access_path.as_pwstr(),
szItemID: self.item_id.as_pwstr(),
bActive: self.active.into(),
hClient: self.item_client_handle,
vtRequestedDataType: self.requested_data_type,
dwBlobSize: self.blob.len().try_into().map_err(|_| {
windows::core::Error::new(
windows::Win32::Foundation::E_INVALIDARG,
"Blob size exceeds u32 maximum value",
)
})?,
- pBlob: self.blob.as_mut_array_ptr(),
+ pBlob: if self.blob.is_empty() {
+ std::ptr::null_mut()
+ } else {
+ self.blob.as_mut_array_ptr()
+ },
wReserved: 0,
})107-112:
Check for overflow when converting blob length
The conversion of self.blob.len() from usize to u32 may fail if the length exceeds u32::MAX. It's important to handle this potential overflow to prevent runtime errors.
Ensure that you provide a clear error message when the blob size is too large, as shown in your current code.
130-137: Ensure safe handling of raw pointers in try_from_native
When creating a RemoteArray from a raw pointer, ensure that the blob_size and pBlob are valid and handle any potential null pointers or invalid sizes.
Run the following script to check for potential issues:
✅ Verification successful
Based on the search results, I can now see the implementation of RemoteArray and its from_raw method. Let me check for any safety-related implementations.
Safe handling of raw pointers is properly implemented
The implementation of RemoteArray already includes comprehensive safety measures:
- The
Dropimplementation safely frees memory when the pointer is non-null as_slice()method has proper null pointer and zero-length checks- Memory is properly managed through
CoTaskMemFree - The struct maintains size information and validates it before use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `pBlob` and `dwBlobSize` are valid in `try_from_native` implementation.
# Expected: `dwBlobSize` is zero when `pBlob` is null, and non-zero when `pBlob` is not null.
# Search for instances where `dwBlobSize` is non-zero but `pBlob` is null.
ast-grep --pattern $'opc_da_bindings::tagOPCITEMRESULT {
$$$
dwBlobSize: $_,
pBlob: std::ptr::null_mut(),
$$$
}' opc_da/src/**/*.rs
Length of output: 234
Script:
#!/bin/bash
# Let's check the implementation of RemoteArray and its usage
rg -A 10 "struct RemoteArray" opc_da/src/
# Check for any safety checks or validation in the implementation
rg -A 10 "from_raw" opc_da/src/
# Look for any tests related to RemoteArray or blob handling
rg -A 5 "test.*RemoteArray|test.*blob" opc_da/src/
Length of output: 7052
Script:
#!/bin/bash
# Check for Drop implementation and safety-related code
rg -A 10 "impl.*Drop.*RemoteArray|unsafe impl.*RemoteArray" opc_da/src/
# Check if there are any other usages of RemoteArray::from_raw
rg -B 5 -A 5 "RemoteArray::from_raw" opc_da/src/
# Look for any null pointer checks or size validations
ast-grep --pattern 'if $_.is_null()' opc_da/src/client/memory.rs
Length of output: 1206
Script:
#!/bin/bash
# Let's check the implementation of as_slice() and other methods in RemoteArray
rg -B 5 -A 10 "fn as_slice|fn is_empty" opc_da/src/client/memory.rs
# Check if there are any safety comments or documentation
rg -B 2 -A 5 "/// |//! " opc_da/src/client/memory.rs
# Look for any other safety checks in memory.rs
ast-grep --pattern 'if $_ {
$$$
}' opc_da/src/client/memory.rs
Length of output: 2639
opc_da/src/client/traits/server.rs (1)
114-118:
Handle potential null pointer from CreateGroupEnumerator
The method CreateGroupEnumerator may return a null pointer. Ensure that you check for None and handle the error appropriately to prevent undefined behavior.
Modify your code to handle the null case:
let enumerator = unsafe {
self.interface()?
.CreateGroupEnumerator(scope, &windows::Win32::System::Com::IEnumUnknown::IID)?
-};
+}?.ok_or_else(|| windows::core::Error::new(
+ windows::Win32::Foundation::E_POINTER,
+ "CreateGroupEnumerator returned null",
+))?;
enumerator.cast()opc_da/src/client/traits/async_io2.rs (2)
23-43:
Check for empty server_handles in read method
To prevent potential issues with the OPC server, ensure that server_handles is not empty before performing the read operation.
Add a check at the beginning of the read method:
fn read(
&self,
server_handles: &[u32],
transaction_id: u32,
) -> windows::core::Result<(u32, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut cancel_id = 0;
let mut errors = RemoteArray::new(len);57-79:
Validate lengths of server_handles and values in write method
Ensure that the lengths of server_handles and values match to correctly associate each handle with its corresponding value.
Add a check to validate the lengths:
fn write(
&self,
server_handles: &[u32],
values: &[VARIANT],
transaction_id: u32,
) -> windows::core::Result<(u32, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.len() != values.len() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles and values must have the same length",
+ ));
+ }
let len = server_handles.len().try_into()?;
let mut cancel_id = 0;
let mut errors = RemoteArray::new(len);opc_da/src/client/traits/async_io.rs (2)
25-54: Validation of server_handles in read method is correctly implemented
The read method properly checks for empty server_handles and returns an appropriate error. This prevents unexpected behavior when no server handles are provided.
70-106: Thorough input validation in write method
The write method correctly checks that server_handles and values arrays are not empty and have the same length. This ensures data consistency and prevents potential errors due to mismatched input.
opc_da/src/client/traits/data_object.rs (2)
54-61:
Fix incorrect error handling in get_canonical_format method
Similar to the previous issue, using .ok()? improperly converts the Result into an Option, which can lead to incorrect error propagation. Remove .ok()? to correctly handle errors.
Apply this diff to correct the error handling:
fn get_canonical_format(&self, format_in: &FORMATETC) -> windows::core::Result<FORMATETC> {
let mut output = FORMATETC::default();
unsafe {
self.interface()?
.GetCanonicalFormatEtc(format_in, &mut output)
- }
- .ok()?;
+ }?;
Ok(output)
}41-43:
Fix incorrect usage of .ok() in query_get_data method
The method query_get_data should return a windows::core::Result<()>, but using .ok() converts the Result into an Option, leading to incorrect error handling. Remove .ok() to return the proper Result type.
Apply this diff to fix the issue:
fn query_get_data(&self, format: &FORMATETC) -> windows::core::Result<()> {
- unsafe { self.interface()?.QueryGetData(format) }.ok()
+ unsafe { self.interface()?.QueryGetData(format) }
}Likely invalid or redundant comment.
opc_da/src/client/v1/mod.rs (3)
35-42: 🛠️ Refactor suggestion
Consider adding COM initialization check in Server::try_from
To prevent undefined behavior, ensure that COM is initialized before casting interfaces in the Server::try_from method.
Apply this diff to add the COM initialization check:
fn try_from(value: windows::core::IUnknown) -> windows::core::Result<Self> {
+ // Verify COM initialization
+ unsafe {
+ let mut apt_type = windows::Win32::System::Com::APTTYPE::default();
+ let mut apt_type_qualifier = windows::Win32::System::Com::APTTYPEQUALIFIER::default();
+ if windows::Win32::System::Com::CoGetApartmentType(&mut apt_type, &mut apt_type_qualifier).is_err() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::CO_E_NOTINITIALIZED.into(),
+ "COM not initialized",
+ ));
+ }
+ }
Ok(Self {
server: value.cast()?,
server_public_groups: value.cast().ok(),
browse_server_address_space: value.cast().ok(),
})
}94-107:
Add COM initialization check in Group::try_from
Ensure that COM is initialized before casting interfaces in the Group::try_from method to prevent potential undefined behavior.
Apply this diff to add the COM initialization check:
fn try_from(value: windows::core::IUnknown) -> windows::core::Result<Self> {
+ // Verify COM initialization
+ unsafe {
+ let mut apt_type = windows::Win32::System::Com::APTTYPE::default();
+ let mut apt_type_qualifier = windows::Win32::System::Com::APTTYPEQUALIFIER::default();
+ if windows::Win32::System::Com::CoGetApartmentType(&mut apt_type, &mut apt_type_qualifier).is_err() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::CO_E_NOTINITIALIZED.into(),
+ "COM not initialized",
+ ));
+ }
+ }
Ok(Self {
item_mgt: value.cast()?,
group_state_mgt: value.cast()?,
public_group_state_mgt: value.cast().ok(),
sync_io: value.cast()?,
async_io: value.cast()?,
data_object: value.cast()?,
})
}85-107: 🛠️ Refactor suggestion
Implement Drop trait for Group to ensure proper cleanup
To prevent resource leaks and ensure proper release of COM interfaces, implement the Drop trait for the Group struct.
Apply this diff to add the Drop implementation:
impl Drop for Group {
fn drop(&mut self) {
// Release COM interfaces in reverse order of acquisition
unsafe {
self.data_object.Release();
self.async_io.Release();
self.sync_io.Release();
if let Some(ref public_group) = self.public_group_state_mgt {
public_group.Release();
}
self.group_state_mgt.Release();
self.item_mgt.Release();
}
}
}opc_da/src/client/traits/item_mgt.rs (7)
32-37: Proper validation of empty items slice in add_items
The add_items method correctly checks if the items slice is empty and returns an appropriate error. This prevents unnecessary COM calls with invalid inputs.
73-78: Correct handling of empty items slice in validate_items
The validate_items method properly checks for an empty items slice and returns an error if it is empty, ensuring robust input validation.
111-116: Validation of server_handles in remove_items
The remove_items method correctly validates that server_handles is not empty before proceeding, preventing potential errors due to invalid input.
145-150: Input validation in set_active_state is properly implemented
The method checks that server_handles is not empty, ensuring that the operation is performed on a valid set of items.
183-195: Consistent length checks in set_client_handles
The set_client_handles method validates that server_handles and client_handles have the same length and are not empty. This prevents mismatches and ensures correct handle associations.
228-240: Proper validation in set_datatypes method
The method checks that server_handles and requested_datatypes have the same length and are not empty, ensuring that data types are set correctly for each handle.
268-271: create_enumerator method correctly handles the Result
As per previous discussions, the CreateEnumerator method returns a Result, and your implementation correctly handles this without unnecessary handling of None values.
opc_da/src/client/traits/public_group_state_mgt.rs (2)
13-15:
Add error propagation for COM call
The unsafe COM call needs proper error propagation.
Apply this diff:
- unsafe { self.interface()?.GetState() }.map(|v| v.as_bool())
+ unsafe { self.interface()?.GetState()? }.map(|v| v.as_bool())25-27:
Add error propagation for COM call
The unsafe COM call needs proper error propagation.
Apply this diff:
- unsafe { self.interface()?.MoveToPublic() }
+ unsafe { self.interface()?.MoveToPublic()? }opc_da/src/client/traits/group_state_mgt2.rs (1)
21-23:
Add validation for keep_alive_time parameter
The keep-alive time should be validated to ensure it's within reasonable bounds.
Apply this diff:
fn set_keep_alive(&self, keep_alive_time: u32) -> windows::core::Result<u32> {
+ // OPC DA spec typically recommends keep-alive times between 100ms and 60000ms
+ if keep_alive_time < 100 || keep_alive_time > 60000 {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "keep_alive_time must be between 100ms and 60000ms",
+ ));
+ }
unsafe { self.interface()?.SetKeepAlive(keep_alive_time) }
}opc_da/src/client/traits/item_deadband_mgt.rs (2)
59-78:
Add empty check for server_handles
The method should validate that server_handles is not empty.
Apply this diff:
fn get_item_deadband(
&self,
server_handles: &[u32],
) -> windows::core::Result<(RemoteArray<f32>, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;87-104:
Add empty check for server_handles
The method should validate that server_handles is not empty.
Apply this diff:
fn clear_item_deadband(
&self,
server_handles: &[u32],
) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;opc_da/src/client/tests.rs (2)
104-115: 🛠️ Refactor suggestion
Improve error handling in read operation.
The error handling for the read operation uses panic-based assertions which are not ideal for tests.
Consider using proper test assertions:
- if errors.len() != 1 {
- panic!("Expected 1 error, got {}", errors.len());
- }
-
- let error = errors.as_slice().first().unwrap();
- if error.is_err() {
- panic!("Error, got {:?}", error);
- }
+ assert_eq!(errors.len(), 1, "Expected exactly one error result");
+ let error = errors.first().expect("Error array is empty");
+ assert!(error.is_ok(), "Unexpected error: {:?}", error);Likely invalid or redundant comment.
71-98:
Implement cleanup for created group.
The test creates a group but doesn't clean it up after the test completes, which could leave resources hanging if the test fails.
Add cleanup using a defer pattern:
+ struct TestCleanup<'a> {
+ server: &'a Server::V2,
+ group_handle: Option<u32>,
+ }
+
+ impl<'a> Drop for TestCleanup<'a> {
+ fn drop(&mut self) {
+ if let Some(handle) = self.group_handle {
+ let _ = self.server.remove_group(handle, true);
+ }
+ }
+ }
+
let group = server
.add_group("test", true, 0, 0, 0, 0, 0.0)
.expect("Failed to add group");
+
+ let _cleanup = TestCleanup {
+ server: &server,
+ group_handle: Some(group.handle()),
+ };Likely invalid or redundant comment.
opc_da/src/client/traits/sync_io2.rs (2)
51-57:
Add bounds checking for array sizes.
The conversion from usize to u32 could fail for large arrays, and the RemoteArray allocation could fail.
Add explicit checks:
+ if server_handles.len() > u32::MAX as usize {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "array size exceeds maximum supported length",
+ ));
+ }
+
let len = server_handles.len().try_into()?;
- let mut values = RemoteArray::new(len);
- let mut qualities = RemoteArray::new(len);
- let mut timestamps = RemoteArray::new(len);
- let mut errors = RemoteArray::new(len);
+ let mut values = RemoteArray::new(len).map_err(|e| {
+ windows::core::Error::new(
+ windows::Win32::Foundation::E_OUTOFMEMORY,
+ format!("failed to allocate values array: {}", e),
+ )
+ })?;
+ // Similar error handling for other arrays...Likely invalid or redundant comment.
84-118: 🛠️ Refactor suggestion
Add max_age validation in write_vqt.
Similar to read_max_age, write_vqt should validate array bounds and allocation.
Apply similar checks as in read_max_age:
fn write_vqt(
&self,
server_handles: &[u32],
values: &[opc_da_bindings::tagOPCITEMVQT],
) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> {
+ if server_handles.len() > u32::MAX as usize {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "array size exceeds maximum supported length",
+ ));
+ }Likely invalid or redundant comment.
opc_da/src/client/traits/browse.rs (2)
108-110:
Add bounds check before setting RemoteArray length
The count should be validated against max_elements to prevent potential buffer overflow.
if count > 0 {
+ if count > max_elements {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "Server returned more elements than requested",
+ ));
+ }
elements.set_len(count);
}Likely invalid or redundant comment.
40-40: 🛠️ Refactor suggestion
Add bounds checking for array length conversion
The conversion from usize to u32 could fail for very large arrays.
-let mut results = RemoteArray::new(item_ids.len().try_into()?);
+let count: u32 = item_ids.len().try_into().map_err(|_| {
+ windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "item_ids length exceeds u32::MAX",
+ )
+})?;
+let mut results = RemoteArray::new(count);Likely invalid or redundant comment.
opc_da/src/client/traits/item_properties.rs (1)
88-93: Extract common validation logic
The error handling code for empty property_ids is duplicated between get_item_properties and lookup_item_ids.
Also applies to: 134-139
opc_da/src/client/traits/item_sampling_mgt.rs (1)
135-145:
Fix potential use-after-free in set_item_buffer_enable
The enable_bool vector is dropped while its pointer is still being used in the unsafe block.
opc_da/src/client/v3/mod.rs (3)
39-47: Add COM initialization check in Server::try_from
Before casting interfaces, verify that COM has been initialized to avoid undefined behavior.
#!/bin/bash
# Search for COM initialization calls in the codebase
rg -A 5 'CoInitializeEx|CoInitialize'112-125: Ensure COM is initialized in Group::try_from
Before casting interfaces, confirm that COM is initialized to prevent potential issues.
#!/bin/bash
# Search for COM initialization calls in the codebase
rg -A 5 'CoInitializeEx|CoInitialize'176-185: Well-implemented error handling for optional interfaces
The implementation correctly handles the case when IOPCItemSamplingMgt is not supported, returning an appropriate error with E_NOTIMPL.
opc_da/src/client/v2/mod.rs (1)
84-93: Well-implemented error handling for optional interfaces
The implementation correctly handles optional interfaces like IOPCServerPublicGroups, returning appropriate errors when not supported.
opc_da/src/client/memory.rs (2)
39-53: Well-implemented safety checks in as_slice
The implementation includes proper null pointer checks, length validation, and safe conversion from u32 to usize.
165-207: Well-designed safe memory management
The LocalPointer implementation provides a safe abstraction over boxed values with proper null handling and clear ownership semantics.
…dling in get_state method, validate deadband values in item management, and refactor group addition logic for clarity
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
opc_da/src/client/traits/connection_point_container.rs (3)
4-9: Add OPC DA version compatibility information to trait documentation.Consider enhancing the trait documentation by specifying which OPC DA versions this trait is compatible with, as this information would be valuable for implementers.
/// COM connection point container functionality. /// /// Provides methods to establish connections between event sources /// and event sinks in the OPC COM architecture. Used primarily for /// handling asynchronous callbacks. +/// +/// # Compatibility +/// Compatible with OPC DA versions: +/// - OPC DA 2.0 +/// - OPC DA 3.0
10-12: Document safety requirements and consider type aliasing.The
interfacemethod is used by unsafe methods, so it should document its safety requirements. Also consider using type aliases to improve readability.+use windows::Win32::System::Com::IConnectionPointContainer; + pub trait ConnectionPointContainerTrait { + /// Returns the underlying COM connection point container interface. + /// + /// # Safety + /// While this method is safe, the returned interface is used by unsafe methods. + /// Implementers must ensure: + /// - The COM object is properly initialized + /// - The interface pointer remains valid for the lifetime of self fn interface( &self, - ) -> windows::core::Result<&windows::Win32::System::Com::IConnectionPointContainer>; + ) -> windows::core::Result<&IConnectionPointContainer>;
41-59: Add usage example to documentation.Consider adding a usage example similar to
find_connection_pointto maintain consistency in documentation./// Enumerates all available connection points. /// /// # Returns /// Enumerator for iterating through available connection points /// +/// # Example +/// ```no_run +/// let enum_cp = container.enum_connection_points(); +/// ``` +/// /// # Safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
opc_da/src/client/traits/connection_point_container.rs(1 hunks)opc_da/src/client/traits/group_state_mgt.rs(1 hunks)opc_da/src/client/traits/item_deadband_mgt.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)
🔇 Additional comments (12)
opc_da/src/client/traits/item_deadband_mgt.rs (4)
1-10: LGTM! Well-documented trait definition.
The trait documentation clearly explains the purpose and functionality of deadband management.
11-56: LGTM! Robust implementation with proper validation.
The implementation includes:
- Input validation for array lengths
- Validation of deadband values within range (0.0 to 100.0)
- Safe memory management using RemoteArray
- Proper error handling for COM interface calls
67-86: 🛠️ Refactor suggestion
Add empty check for consistency.
Add validation to ensure server_handles is not empty, similar to set_item_deadband.
Apply this diff:
fn get_item_deadband(
&self,
server_handles: &[u32],
) -> windows::core::Result<(RemoteArray<f32>, RemoteArray<windows::core::HRESULT>)> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;95-112: 🛠️ Refactor suggestion
Add empty check for consistency.
Add validation to ensure server_handles is not empty, similar to set_item_deadband.
Apply this diff:
fn clear_item_deadband(
&self,
server_handles: &[u32],
) -> windows::core::Result<RemoteArray<windows::core::HRESULT>> {
+ if server_handles.is_empty() {
+ return Err(windows::core::Error::new(
+ windows::Win32::Foundation::E_INVALIDARG,
+ "server_handles must not be empty",
+ ));
+ }
let len = server_handles.len().try_into()?;opc_da/src/client/traits/group_state_mgt.rs (4)
1-19: LGTM! Clear and well-structured trait definition.
The trait is well-documented with comprehensive explanation of its functionality.
49-91: LGTM! Robust implementation with proper memory management.
The implementation correctly uses LocalPointer for parameter management and handles errors appropriately.
23-47: 🛠️ Refactor suggestion
Improve memory safety in name handling.
The current implementation could be improved to ensure proper cleanup of COM resources.
Consider using RAII pattern more effectively:
fn get_state(&self) -> windows::core::Result<def::GroupState> {
let mut state = def::GroupState::default();
let mut active = windows::Win32::Foundation::BOOL::default();
- let name = {
- let mut name = RemotePointer::new();
+ let mut name_buf = Vec::<u16>::new();
+ let mut name_ptr = windows::core::PWSTR::null();
+
+ unsafe {
+ self.interface()?.GetState(
+ &mut state.update_rate,
+ &mut active,
+ &mut name_ptr,
+ &mut state.time_bias,
+ &mut state.percent_deadband,
+ &mut state.locale_id,
+ &mut state.client_group_handle,
+ &mut state.server_group_handle,
+ )?;
+
+ if !name_ptr.is_null() {
+ name_buf = name_ptr.to_string()?.into_bytes();
+ }
+ }
+
+ state.active = active.as_bool();
+ state.name = String::from_utf8(name_buf)?;94-113:
Add error propagation for COM calls.
The unsafe calls to SetName and CloneGroup should propagate potential errors.
Apply these diffs:
fn set_name(&self, name: &str) -> windows::core::Result<()> {
let name = LocalPointer::from_str(name)?;
- unsafe { self.interface()?.SetName(name.as_pwstr()) }
+ unsafe { self.interface()?.SetName(name.as_pwstr())? };
+ Ok(())
}
fn clone_group(
&self,
name: &str,
id: &windows::core::GUID,
) -> windows::core::Result<windows::core::IUnknown> {
let name = LocalPointer::from_str(name)?;
- unsafe { self.interface()?.CloneGroup(name.as_pwstr(), id) }
+ unsafe { self.interface()?.CloneGroup(name.as_pwstr(), id)? }
}opc_da/src/client/traits/connection_point_container.rs (2)
14-39: Implementation looks good with proper safety documentation.
The method is well-documented with clear safety requirements, error conditions, and usage examples. The unsafe block is properly contained.
9-60: Verify trait implementation and usage patterns.
Let's verify how this trait is being implemented and used across the codebase to ensure consistent safety patterns.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Trait implementation and usage patterns are well-structured and safe
The verification shows:
- The trait is consistently implemented by both Server and Group types in v2 and v3 modules
- COM initialization is properly handled through a dedicated guard with COINIT_MULTITHREADED
- All implementations properly manage the connection_point_container field
- Usage patterns align with the documented safety requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of ConnectionPointContainerTrait
echo "Searching for trait implementations..."
rg -A 5 "impl\s+ConnectionPointContainerTrait\s+for"
# Search for usage of the unsafe methods
echo "Searching for method usage patterns..."
rg -A 3 "find_connection_point|enum_connection_points"
# Look for potential COM initialization patterns
echo "Checking COM initialization patterns..."
rg -A 5 "COINIT_"
Length of output: 7074
opc_da/src/client/traits/server.rs (2)
48-62: 🛠️ Refactor suggestion
Minimize the scope of the unsafe block and document safety
To enhance code safety and readability, consider reducing the unsafe block to encompass only the unsafe operations. Additionally, documenting the safety requirements helps others understand the rationale.
Refactored code:
let group_name = LocalPointer::from(name);
let group_name = group_name.as_pcwstr();
let mut revised_percent_deadband = 0;
// Obtain the interface outside the unsafe block
let interface = self.interface()?;
unsafe {
// SAFETY: `AddGroup` is unsafe because it involves raw pointers and COM interface calls.
interface.AddGroup(
group_name,
windows::Win32::Foundation::BOOL::from(active),
update_rate,
client_handle,
&time_bias,
&percent_deadband,
locale_id,
&mut group_server_handle,
&mut revised_percent_deadband,
&opc_da_bindings::IOPCItemMgt::IID,
&mut group,
)?;
}118-118: 🛠️ Refactor suggestion
Handle potential failure in enumerator.cast()
The cast() method may fail if the enumerator cannot be cast to the desired interface. Explicitly handling this potential failure improves robustness.
Updated code:
enumerator.cast().map_err(|e| windows::core::Error::new(
e.code(),
"Failed to cast enumerator to IEnumUnknown",
))
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
opc_da/src/client/memory.rs (5)
83-92: Consider adding safety documentation for Drop implementationWhile the Drop implementation is correct, it would be beneficial to document the safety guarantees around
CoTaskMemFree.Add documentation:
impl<T: Sized> Drop for RemoteArray<T> { + /// Frees the memory allocated by COM using CoTaskMemFree. + /// + /// # Safety + /// This implementation assumes the pointer was allocated using COM's task allocator. #[inline(always)] fn drop(&mut self) {
139-150: Consider handling potential string conversion errorsThe String conversion could fail for reasons other than null pointers (e.g., invalid UTF-16). Consider providing more specific error information.
fn try_from(value: RemotePointer<u16>) -> Result<Self, Self::Error> { if value.inner.is_null() { return Err(windows::Win32::Foundation::E_POINTER.into()); } - Ok(unsafe { PWSTR(value.inner).to_string() }?) + let pwstr = unsafe { PWSTR(value.inner) }; + pwstr.to_string().map_err(|e| { + windows::core::Error::new( + windows::Win32::Foundation::E_INVALIDARG, + format!("Invalid UTF-16 string: {}", e) + ) + }) }
223-228: Consider optimizing UTF-16 string conversionThe current implementation allocates a new Vec for each string conversion. For better performance with large strings, consider using a pre-allocated buffer.
impl From<&str> for LocalPointer<Vec<u16>> { #[inline(always)] fn from(s: &str) -> Self { + let mut vec = Vec::with_capacity(s.len() + 1); + vec.extend(s.encode_utf16()); + vec.push(0); - Self::new(Some(s.encode_utf16().chain(Some(0)).collect())) + Self::new(Some(vec)) } }
285-293: Consider returning an empty Vec instead of a Vec with null PWSTRWhen inner is None, returning an empty Vec might be more intuitive than a Vec containing a null PWSTR.
pub fn as_pwstr_array(&self) -> Vec<windows::core::PWSTR> { match &self.inner { Some(values) => values .iter() .map(|value| windows::core::PWSTR(value.as_ptr() as _)) .collect(), - None => vec![windows::core::PWSTR::null()], + None => Vec::new(), } }
1-323: Overall solid implementation with good memory safetyThe implementation demonstrates good practices:
- Proper use of null pointer checks
- Safe conversion between different pointer types
- Correct COM memory management
- Clear separation between remote and local pointer management
A few minor suggestions were made for optimization and documentation, but the core implementation is sound.
Consider adding unit tests to verify the memory safety guarantees, especially around COM memory management and string conversions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
opc_da/src/client/memory.rs(1 hunks)opc_da/src/client/traits/server.rs(1 hunks)
🔇 Additional comments (4)
opc_da/src/client/memory.rs (1)
39-53: LGTM! Safe implementation of as_slice
The implementation correctly:
- Handles null pointer and zero length cases
- Performs bounds checking with
try_from - Includes proper safety documentation
opc_da/src/client/traits/server.rs (3)
81-81:
Handle potential null pointer returned by GetStatus
The GetStatus method may return a null pointer. Before using the returned pointer, check if it's null to prevent undefined behavior.
Consider updating the code as follows:
let status = unsafe { self.interface()?.GetStatus()? };
if status.is_null() {
return Err(windows::core::Error::new(
windows::Win32::Foundation::E_POINTER,
"GetStatus returned a null pointer",
));
}
Ok(RemotePointer::from_raw(status))109-114:
Handle potential null pointer returned by CreateGroupEnumerator
The CreateGroupEnumerator method may return a null pointer. Check if enumerator is None before attempting to cast it to avoid potential errors.
Consider updating the code:
let enumerator = unsafe {
self.interface()?
.CreateGroupEnumerator(scope, &windows::Win32::System::Com::IEnumUnknown::IID)?
};
let enumerator = enumerator.ok_or_else(|| windows::core::Error::new(
windows::Win32::Foundation::E_POINTER,
"CreateGroupEnumerator returned a null pointer",
))?;
enumerator.cast()48-62: 🛠️ Refactor suggestion
Minimize the scope of the unsafe block and add safety comments
For better code safety and readability, consider minimizing the scope of the unsafe block to only the unsafe call and add safety comments explaining why the operation is safe.
Refactored code:
let mut group = None;
let mut group_server_handle = 0u32;
let group_name = LocalPointer::from(name);
let group_name = group_name.as_pcwstr();
let mut revised_percent_deadband = 0.0;
unsafe {
// SAFETY: The pointers are valid, and COM call conventions are correctly followed.
self.interface()?.AddGroup(
group_name,
windows::Win32::Foundation::BOOL::from(active),
update_rate,
client_handle,
&time_bias,
&percent_deadband,
locale_id,
&mut group_server_handle,
&mut revised_percent_deadband,
&opc_da_bindings::IOPCItemMgt::IID,
&mut group,
)?;
}
…UID iteration
Summary by CodeRabbit
New Features
AsyncIoTrait,ItemMgtTrait, andGroupStateMgtTrait.Client,Group, andServerthat encapsulate various OPC interfaces.Clientstruct using the Actix framework.RemoteArrayandLocalPointer.actor,client,group,iter, andserverwithin the unified client structure.ClientActorstruct for managing server GUID retrieval asynchronously.Guardstruct to manage COM initialization and uninitialization safely.Bug Fixes
Documentation
Refactor
windows_coretowindowsacross multiple files for consistency.Nullvariant from theVariantenum to streamline its usage.