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.

Sign up for GitHub

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

Update to windows 0.58 #1338

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update to windows 0.58 #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ serde = { version = "^1.0.190", optional = true }

[target.'cfg(windows)'.dependencies]
ntapi = { version = "0.4", optional = true }
# Support a range of versions in order to avoid duplication of this crate. Make sure to test all
# versions when bumping to a new release, and only increase the minimum when absolutely necessary.
windows = { version = ">=0.54, <=0.57", optional = true }
windows = { version = "0.58", optional = true }

[target.'cfg(not(any(target_os = "unknown", target_arch = "wasm32")))'.dependencies]
libc = "^0.2.153"
Expand Down
20 changes: 10 additions & 10 deletions src/windows/cpu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ unsafe fn init_load_avg() -> Mutex<Option<LoadAvg>> {
}

struct InternalQuery {
query: HANDLE,
query: isize,
event: HANDLE,
data: HashMap<String, HANDLE>,
data: HashMap<String, isize>,
}

unsafe impl Send for InternalQuery {}
Expand All @@ -147,15 +147,15 @@ impl Drop for InternalQuery {
fn drop(&mut self) {
unsafe {
for (_, counter) in self.data.iter() {
PdhRemoveCounter(counter.0);
PdhRemoveCounter(*counter);
}

if !self.event.is_invalid() {
let _err = CloseHandle(self.event);
}

if !self.query.is_invalid() {
PdhCloseQuery(self.query.0);
if !(self.query == -1 || self.query == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

That code in particular is pretty bad. :-/

Do you know if there is a wrapper type instead of just handling an isize directly? That's very error-prone... The previous HANDLE wasn't great, but at least it was obvious what we were manipulating.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it's not great, it just duplicates the is_invalid code but I wasn't sure even /that/ was actually correct in this case. Usually these windows functions are already hardened against invalid handles so it might just be OK to always call PdhCloseQuery instead of checking on our side.

Ideally there'd be a PDH_HQUERY as used by the Pdh* functions but no such type exists in windows-rs.

Copy link
Owner

Choose a reason for hiding this comment

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

I opened https://github.com/microsoft/windows-rs/issues/3200 for this. I'm really not sure what to do here... Is there any internals improvements in windows crate worth merging this PR as is even with the new code?

If not, would you be open to create a newtype which would wrap this value so we at least know what we're manipulating?

Copy link

Choose a reason for hiding this comment

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

I opened https://github.com/microsoft/windows-rs/issues/3200 for this. I'm really not sure what to do here... Is there any internals improvements in windows crate worth merging this PR as is even with the new code?

If not, would you be open to create a newtype which would wrap this value so we at least know what we're manipulating?

It would be nice if this were merged so that my dependency tree doesn't use 4 or 5 different versions of windows-rs. :)

Copy link
Owner

Choose a reason for hiding this comment

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

It was merged. Just need to wait for windows-rs 0.59 version to be released now I suppose. :)

Copy link
Author

Choose a reason for hiding this comment

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

Wouldn't it be a better idea to merge this and make the code look like 0.59 in the meantime. windows-rs upgrades are infrequent and (by us) kind of unwanted too since they're a massive ecosystem wide upgrade that needs to happen.

Copy link
Owner

Choose a reason for hiding this comment

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

Not really. The code is much less good with the 0.58 version and I'd prefer to avoid having "bad code" until next windows-rs release.

Copy link

@BenjaminBrienen BenjaminBrienen Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was merged. Just need to wait for windows-rs 0.59 version to be released now I suppose. :)

I'm confused. You say it was merged, but the PR is still open.

in master (why is it not called "main"?) you have windows = { version = ">=0.54, <=0.57", optional = true }

Copy link
Owner

Choose a reason for hiding this comment

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

The issue I opened was merged in microsoft/win32metadata@ff991cf. Now I'm waiting for windows-rs to update their dependency.

(why is it not called "main"?)

It was created with the branch named master so don't see a reason to change it.

PdhCloseQuery(self.query);
}
}
}
Expand All @@ -171,7 +171,7 @@ impl Query {
unsafe {
if PdhOpenQueryA(PCSTR::null(), 0, &mut query) == ERROR_SUCCESS.0 {
let q = InternalQuery {
query: HANDLE(query),
query,
event: HANDLE::default(),
data: HashMap::new(),
};
Expand All @@ -190,7 +190,7 @@ impl Query {
let mut display_value = mem::MaybeUninit::<PDH_FMT_COUNTERVALUE>::uninit();

return if PdhGetFormattedCounterValue(
counter.0,
*counter,
PDH_FMT_DOUBLE,
None,
display_value.as_mut_ptr(),
Expand All @@ -216,13 +216,13 @@ impl Query {
unsafe {
let mut counter = 0;
let ret = PdhAddEnglishCounterW(
self.internal.query.0,
self.internal.query,
PCWSTR::from_raw(getter.as_ptr()),
0,
&mut counter,
);
if ret == ERROR_SUCCESS.0 {
self.internal.data.insert(name.clone(), HANDLE(counter));
self.internal.data.insert(name.clone(), counter);
} else {
sysinfo_debug!(
"Query::add_english_counter: failed to add counter '{}': {:x}...",
Expand All @@ -237,7 +237,7 @@ impl Query {

pub fn refresh(&self) {
unsafe {
if PdhCollectQueryData(self.internal.query.0) != ERROR_SUCCESS.0 {
if PdhCollectQueryData(self.internal.query) != ERROR_SUCCESS.0 {
sysinfo_debug!("failed to refresh CPU data");
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/windows/sid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
use std::{fmt::Display, str::FromStr};

use windows::core::{PCWSTR, PWSTR};
use windows::Win32::Foundation::{LocalFree, ERROR_INSUFFICIENT_BUFFER, HLOCAL, PSID};
use windows::Win32::Foundation::{LocalFree, ERROR_INSUFFICIENT_BUFFER, HLOCAL};
use windows::Win32::Security::Authorization::{ConvertSidToStringSidW, ConvertStringSidToSidW};
use windows::Win32::Security::{
CopySid, GetLengthSid, IsValidSid, LookupAccountSidW, SidTypeUnknown,
CopySid, GetLengthSid, IsValidSid, LookupAccountSidW, SidTypeUnknown, PSID
};

use crate::sys::utils::to_utf8_str;
Expand Down