Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration from winapi to windows-rs crate #68

Merged
merged 7 commits into from
Feb 28, 2023

Conversation

vthib
Copy link
Contributor

@vthib vthib commented Feb 27, 2023

Replace winapi, com and widestring with the windows-rs crate:

What does it changes internally (should not be visible from the public API):

  • windows-rs comes with a BSTR module helper, so the wmi-rs one is no longer needed.
  • COM objects all properly implement Clone (with AddRef) and Drop (with Release), so no need to do it by hand, and no risk of memory issues when forgotten.
  • The implement feature gives access to easy COM implementation: a rust struct can implement a COM interface, and the interface comes in the form of a clean Rust trait.
  • The APIs are more rust-idiomatic to use: None can be passed for many optional parameters, and the return result is very often already a Result: the check_hres helper is no longer needed.

What does it changes externally (breaking changes):

  • The widestring dependency is no longer needed, and its errors are no longer exposed in the WMIError object. Instead, the BSTR helper from windows-rs is used, and this one generates the standard std::string::FromUtf16Error error. This gives less information on the conversion issue.
  • The BSTR helper from windows-rs also panics on allocation failure, making the WMIError::ConvertAllocateError obsolete. We could probably change this with an internal conversion if needed, but "panic on allocation failure" is probably baked in other windows-rs helpers...

Another point to take into account is that windows-rs is still getting updated regularly, and so it may be needed to release new wmi-rs versions more regularly to keep up with those updates. The windows-sys crate is a bit lower level and less often updated, and is supposed to be designed for libs like this. However, we need the COM helpers and the implement macro, which only exists in the higher-level windows crate.

Fixes #67

Copy link
Owner

@ohadravid ohadravid left a comment

Choose a reason for hiding this comment

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

Wow! 🚀
I really really like this.
Makes the code safer, easier to read the It's feels "more Rusty" - the windows-rs crate is super nice to use.

Thanks for working on this 🙌

// https://docs.microsoft.com/en-us/windows/win32/api/wbemcli/nf-wbemcli-iwbemobjectsink-indicate
// For error codes, see https://docs.microsoft.com/en-us/windows/win32/learnwin32/error-handling-in-com
let objs = unsafe {
std::slice::from_raw_parts(
Copy link
Owner

Choose a reason for hiding this comment

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

The reason the old code used ptr.add was that from_raw_parts has a bunch of safety reqs which I didn't want to risk violating.
If you think that the apObjArray is OK to use in this context, that's fine by me 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the safety comment, imho it's "safe" to do so, as those safety requirements are supposed to be guaranteed by the API for Indicate (and if they are not upheld, we would be UB anyway as any read from apObjArray + i would be OOB)

Copy link
Owner

Choose a reason for hiding this comment

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

I think the note that The array memory itself is read-only from MSDN is the one thing I wasn't sure about 👍


check_hres(VariantClear(&mut vt_prop))?;
VariantClear(&mut vt_prop)?;
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this now? Maybe VARIANT does this by itself?
No, there's no Drop impl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, windows-rs does not improve the ease of use for VARIANT ^^ It's quite a pain to deal with, because it can be of any type and filled in many different ways. there's no clone/drop or any helpers for it yet in this crate.

// https://docs.microsoft.com/en-us/windows/win32/api/wbemcli/nf-wbemcli-iwbemobjectsink-indicate
// For error codes, see https://docs.microsoft.com/en-us/windows/win32/learnwin32/error-handling-in-com
let objs = unsafe {
std::slice::from_raw_parts(
Copy link
Owner

Choose a reason for hiding this comment

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

I think the note that The array memory itself is read-only from MSDN is the one thing I wasn't sure about 👍

@ohadravid
Copy link
Owner

I merged #66 and released 0.11.5.
Ping me when you fix the conflicts and I'll merge this 🥳

@vthib
Copy link
Contributor Author

vthib commented Feb 28, 2023

Thanks for the reactivity and positivity :)

I solved the conflicts, it should be good to go.

@ohadravid ohadravid merged commit eb960e4 into ohadravid:main Feb 28, 2023
@vthib vthib deleted the 67-windows-rs-migration branch February 28, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from winapi to windows-rs
2 participants