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

Use ComPtr<> from the MS WRL library #8014

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dreamer-dead
Copy link
Contributor

Here I'm trying to use MS WRL library/component.
Relates to #8003

Will see how it builds/links

@dreamer-dead dreamer-dead requested review from a team as code owners May 6, 2023 07:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 7, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dreamer-dead dreamer-dead changed the title WIP: Add header for ComPtr usage Use ComPtr<> ftom the MS WRL library May 8, 2023
@dreamer-dead dreamer-dead changed the title Use ComPtr<> ftom the MS WRL library Use ComPtr<> from the MS WRL library May 8, 2023
@marcosd4h
Copy link
Contributor

marcosd4h commented May 9, 2023

The PR showcases ComPtr<> usage, making it a good candidate for merge

Can you confirm that osquery operates without crashing and functions as expected upon exiting? You can test this by running osqueryi.exe, querying a table that utilizes the WMI functionality, and exiting the application. The ComPtr<> destructor will be invoked upon exit, so it would be good to observe its behavior in this context.

Additionally, it would be great if you could explore the possibility of adding some tests to further illustrate the use of ComPtr<>. Thank you!

@mike-myers-tob mike-myers-tob changed the title Use ComPtr<> from the MS WRL library Use ComPtr<> from the MS WRL library May 9, 2023
@mike-myers-tob mike-myers-tob added Windows refactor Related to osquery code refactoring ready for review Pull requests that are ready to be reviewed by a maintainer labels May 9, 2023
@dreamer-dead
Copy link
Contributor Author

@marcosd4h thanks for the review!
Will try to run the binary as soon as can get a Windows machine.

Do you want to add tests into this particular PR or make a new one?

@marcosd4h
Copy link
Contributor

to add tests into this particular PR or make a new one?

I'll let you decide on this one. Having tests will help on showcasing more usage examples

@directionless directionless added this to the 5.9.0 milestone Jun 3, 2023
@directionless
Copy link
Member

What's the status on this one?

@dreamer-dead
Copy link
Contributor Author

What's the status on this one?

@directionless I should do some hand checks and I can try to add few unit-tests to show ComPtr usage, but I'm quite busy at the moment =(
This PR do not address any bugs and I believe it can wait or even be closed until I have more time to complete.

@directionless
Copy link
Member

It's no problem leaving PRs open, but I'll push this to the next release target.

@directionless directionless modified the milestones: 5.9.0, 5.10.0 Jun 6, 2023
@Smjert
Copy link
Member

Smjert commented Oct 3, 2023

@marcosd4h I would say that we have tests that run a query on each table on Windows, so this code should be already hit (for the concern of crashing), but more tests are welcome.

That been said @dreamer-dead , I also wonder what about the notice reported here: https://learn.microsoft.com/en-us/cpp/cppcx/wrl/windows-runtime-cpp-template-library-wrl?view=msvc-160, which says that the WRL library is being deprecated in favor of the C++/WinRT one.
We are compiling with C++17, so it should be accessible.

I would move this to the next milestone though.

@Smjert Smjert modified the milestones: 5.10.0, 5.11.0 Oct 6, 2023
@directionless directionless modified the milestones: 5.11.0, 5.12.0 Dec 27, 2023
@directionless directionless modified the milestones: 5.12.0, 5.13 Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull requests that are ready to be reviewed by a maintainer refactor Related to osquery code refactoring Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants