Support for Event notifications #60

MineBartekSA · 2022-09-19T10:05:56Z

In this pull request, I'm adding support for WMI Event notifications and deserialization of the Unknown Variant.
This PR closes #46.

Significant changes were made to notifiaction.rs, variant.rs, result_enumerator.rs, query.rs, variant_de.rs, and wbem_class_de.rs.
Other files only received some minor changes.

Major changes

I've added a new file called notifiaction.rs, which contains new WMIConnection methods used to subscribe to WMI Event notifications. They utilize the ExecNoticicationQuery (and for async the Async equivalent) method.

Since intrinsic events hold event data inside their properties, I've added appropriate Variant variants and their processing.
The new Variant::Unknown(IUnknownWrapper) is a temporary variant used to pass the IUknown to the CIM conversion stage. From there it is converted into the Variant::Object(IWbemClassWrapper) using the IUknown::QueryInterface() method.

Also, because of how intrinsic events are queried, I've added new FilterValue variants.
New variants add support for the WITHIN clause and the ISA operator.

I've attempted to add deserialization of the Variant::Object and somewhat succeeded.
But it resulted in a quite major change to the wbem_class_de::Deserializer.
Since I don't know how to handle lifetimes and borrows that well, I needed to remove its lifetimes and make it own the IWbemClassWrapper copy.
I don't think this should break or change anything major, besides the need to clone the IWbemClassWrapper, which now implements the Clone trait.

I also wanted to add serialization to IWbemClassWrapper, since Variant implements Serialize, ~~but for the same reason as above, I failed~~ and I've somewhat succeeded.

Minor changes

Most minor changes are attributed to documentation or Clippy suggestions.
I've also unified returns to all use the WMIResult alias instead of Result<_, WMIError>.
Lastly, I also fixed the wbem_class_de::tests::it_desr_into_map test. It was failing for me since I don't have the en-US lang.

Comments, suggestions, and explanations would be very welcome.

~~Also, please note that doc tests might not ever complete, or they will only complete after a while because they will wait for a new process to start.~~

ohadravid · 2022-09-19T15:07:51Z

Hi!

Thanks for looking into this 😄

I'll have more time next week to review this properly but overall this sounds great 🚀

About the waiting doc tests: I think the best thing would be to start a new thread which starts a short lived process (like ping -n 1 127.0.0.1) in a loop, just to make sure tests always finish in a timely manner.

MineBartekSA · 2022-09-20T11:36:44Z

I've fixed tests.
Now doc tests will finish in ~3 seconds max.
Also, I've added the missing dev dependency for the test I've fixed.
For some reason, I forgot to add Cargo.toml to the original commit.

Also, as much as I wanted to put the two new functions (at the bottom of the lib.rs file) for the doc tests in the wmi::tests module, I just couldn't because that module isn't being imported.
From what I can see, doc tests are run like normal programs, not tests.
And I have tried to add doctest to the #[cfg()]

MineBartekSA · 2022-09-22T07:47:07Z

I've again tried to implement Serialize for IWbemClassWrapper and I've somewhat succeeded.
I added a RefCell<Option<WbemClassSerializationData>> as a private filed to IWbemClassWrapper.
I'm using a RefCell because I need to write to that field under &self.
And for borrowing the data, I'm leaking a raw pointer to that struct, in order to get a borrow with a static lifetime.
I haven't found a better way and from my tests, this actually doesn't leak any memory.

Then again, I'm pretty much experimenting and seeing what sticks.
If there is a better way of doing this, I'd like to learn it.

ohadravid

I did an initial review for most of the code except the desr stuff.
Awesome work 🚀! Thanks again for implementing this

ohadravid · 2022-09-23T06:24:08Z

src/async_query.rs

@@ -56,13 +57,13 @@ impl WMIConnection {
    /// # use wmi::*;
    /// # use std::collections::HashMap;
    /// # use futures::executor::block_on;
-    /// # fn main() -> Result<(), wmi::WMIError> {
+    /// # fn main() -> wmi::WMIResult<()> {


If you have the time, splitting this PR into 2 (one with this kind of changes, the other with your new logic) would be great

src/lib.rs

src/notification.rs

src/query.rs

src/variant.rs

src/result_enumerator.rs

ohadravid · 2022-09-23T07:17:11Z

src/result_enumerator.rs

+    where
+        S: serde::Serializer
+    {
+        let data = self.serialization_data().map_err(Error::custom)?;


What you actually want here, which I think avoids all the lifetime problems, is to generate a "map" and not a "struct"

The distinction Serde makes [between structs and maps] is that structs have keys that are compile-time constant strings and will be known at deserialization time without looking at the serialized data.

https://serde.rs/impl-serialize.html#serializing-a-struct

impl Serialize for IWbemClassWrapper { fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer { let props = self.list_properties().map_err(Error::custom)?; let mut map = serializer.serialize_map(Some(props.len()))?; for property in props.iter() { let value = self.get_property(&property).unwrap(); map.serialize_entry(property, &value)?; } map.end() } }

Could you also add to the src\query.rs::it_works test to check that it serializes correctly?

(But you should keep the Deserializer vs Deserializer<'a> change: the new version is needed for the new code to work, and is better anyway)

I'm kinda mad this works without any problems. But it is a better way of handling this.

src/variant.rs

ohadravid

🚀🚀
just a small nit-pick about naming conventions, and we're ready to merge this

Also: merge from main, I fixed the CI workflow so the tests should run now

ohadravid · 2022-09-29T18:27:35Z

src/query.rs


 pub enum FilterValue {
    Bool(bool),
    Number(i64),
    Str(&'static str),
    String(String),
+    Is_A(&'static str),


I think this should be IsA, and the function is_a() (just the expected Rust conventions)

I should have done that from the beginning, but for some reason, I focused on IsA.
Anyway, I've changed this now.

ohadravid · 2022-09-29T18:29:29Z

src/query.rs

@@ -51,7 +53,17 @@ impl From<bool> for FilterValue {
    }
 }

-/// Build an SQL query for the given filters, over the given type (using it's name and fields).
+impl FilterValue {
+    pub fn IsA<'de, T>() -> WMIResult<Self>


Maybe copy-paste a part of the example in lib.rs as a docstring for this? (No need to actually query anything, just a simplified example)

ohadravid · 2022-09-29T18:32:50Z

src/utils.rs


 #[derive(Debug, Error)]
 #[non_exhaustive]
 pub enum WMIError {
+    /// You can find a useful resource for decoding error codes [here](https://docs.microsoft.com/en-us/windows/win32/wmisdk/wmi-error-constants)


MineBartekSA · 2022-09-29T19:15:25Z

Glad to hear it! 😄

Also, you need to approve the workflow run

ohadravid · 2022-09-29T19:30:42Z

Ah sorry you were bitten by the chrono/time features. Maybe add #[cfg(feature = "chrono")] above the relevant asserts & use?

MineBartekSA · 2022-09-29T19:33:57Z

Ok, fixed

MineBartekSA · 2022-09-29T19:39:38Z

I forgot about usings, my bad

ohadravid · 2022-09-29T20:03:21Z

Released as version 0.11.3, thanks a lot @MineBartekSA 🎉

MineBartekSA added 2 commits September 19, 2022 11:19

Add notifications

12ee6e8

Fix some spelling mistakes

35b55c9

Fix tests

5a6ef8a

IWbemClassWrapper serialization

db0c366

ohadravid requested changes Sep 23, 2022

View reviewed changes

MineBartekSA added 4 commits September 23, 2022 09:38

Move to serializaing a map instead of a strut from IWbemClassWrapper

dad2881

Move some doctests under the test feature

6a204e7

FilterValue and documentation improvements

adacab9

Add build_notification_query and some docs

77cd288

ohadravid approved these changes Sep 29, 2022

View reviewed changes

MineBartekSA added 2 commits September 29, 2022 21:10

Rename FilterValue ISA clause related symbols and some documentation

63b6af1

Merge branch 'ohadravid:main' into main

47f1994

MineBartekSA added 2 commits September 29, 2022 21:32

Add chrono/time crate specific notification tests

b5579e0

Rename time create specific notification tests

972525c

Fix tests

a5b2e0f

ohadravid merged commit 99b4029 into ohadravid:main Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Event notifications #60

Support for Event notifications #60

MineBartekSA commented Sep 19, 2022 •

edited

Loading

ohadravid commented Sep 19, 2022

MineBartekSA commented Sep 20, 2022 •

edited

Loading

MineBartekSA commented Sep 22, 2022

ohadravid left a comment

ohadravid Sep 23, 2022 •

edited

Loading

ohadravid Sep 23, 2022 •

edited

Loading

MineBartekSA Sep 23, 2022

ohadravid Sep 23, 2022

ohadravid left a comment •

edited

Loading

ohadravid Sep 29, 2022

MineBartekSA Sep 29, 2022

ohadravid Sep 29, 2022

ohadravid Sep 29, 2022

MineBartekSA commented Sep 29, 2022

ohadravid commented Sep 29, 2022

MineBartekSA commented Sep 29, 2022

MineBartekSA commented Sep 29, 2022

ohadravid commented Sep 29, 2022

Support for Event notifications #60

Support for Event notifications #60

Conversation

MineBartekSA commented Sep 19, 2022 • edited Loading

Major changes

Minor changes

ohadravid commented Sep 19, 2022

MineBartekSA commented Sep 20, 2022 • edited Loading

MineBartekSA commented Sep 22, 2022

ohadravid left a comment

Choose a reason for hiding this comment

ohadravid Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

ohadravid Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

MineBartekSA Sep 23, 2022

Choose a reason for hiding this comment

ohadravid Sep 23, 2022

Choose a reason for hiding this comment

ohadravid left a comment • edited Loading

Choose a reason for hiding this comment

ohadravid Sep 29, 2022

Choose a reason for hiding this comment

MineBartekSA Sep 29, 2022

Choose a reason for hiding this comment

ohadravid Sep 29, 2022

Choose a reason for hiding this comment

ohadravid Sep 29, 2022

Choose a reason for hiding this comment

MineBartekSA commented Sep 29, 2022

ohadravid commented Sep 29, 2022

MineBartekSA commented Sep 29, 2022

MineBartekSA commented Sep 29, 2022

ohadravid commented Sep 29, 2022

MineBartekSA commented Sep 19, 2022 •

edited

Loading

MineBartekSA commented Sep 20, 2022 •

edited

Loading

ohadravid Sep 23, 2022 •

edited

Loading

ohadravid Sep 23, 2022 •

edited

Loading

ohadravid left a comment •

edited

Loading