-
Notifications
You must be signed in to change notification settings - Fork 28
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
Upgrade windows crate #51
Conversation
@@ -44,54 +41,10 @@ const FOF_NO_UI: u32 = FOF_SILENT | FOF_NOCONFIRMATION | FOF_NOERRORUI | FOF_NOC | |||
const FOFX_EARLYFAILURE: u32 = 0x00100000; | |||
/////////////////////////////////////////////////////////////////////////// | |||
|
|||
macro_rules! check_res_and_get_ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we can get rid of these macros and just do idiomatic ?
Rust error handling now.
src/windows.rs
Outdated
}); | ||
|
||
// TODO: take care of freeing this? | ||
let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed from MaybeUninit::<*mut ITEMIDLIST>
to MaybeUninit::<ITEMIDLIST>
to get things to compile and that was probably a bad idea. I need to spend some more time thinking about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I remember this must be a slice of sorts now, but I wasn't able to produce a pointer to it. It's probably an unsafe-Rust question, but if we should all be stuck I am sure we will get help over at the windows
repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost certain that is incorrect. I think the correct way to do it is the following.
Note that item
is a pointer, as it should be.
Please also add the call to CoTaskMemFree
.
let next_items = &mut [std::ptr::null_mut()];
while let Ok(_) = peidl.Next(next_items, std::ptr::null_mut()) {
let item = next_items[0];
defer! {{ CoTaskMemFree(item as *mut c_void); }}
@ArturKovacs FYI this is as far as I was able to get yesterday. It's a bit rough and I need to spend some more time to formulate useful questions for you. Feel free to take a look now if you're bored or curious, otherwise I'll come back to this in a few days and ping you with some specific questions. |
That was my experience as well, it's like using a |
Made good progress on this today. This Raymond Chen article was really helpful, I've been porting the C++ from that to Rust. Will update the PR in a day or 2. |
src/windows.rs
Outdated
}); | ||
|
||
// TODO: take care of freeing this? | ||
let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost certain that is incorrect. I think the correct way to do it is the following.
Note that item
is a pointer, as it should be.
Please also add the call to CoTaskMemFree
.
let next_items = &mut [std::ptr::null_mut()];
while let Ok(_) = peidl.Next(next_items, std::ptr::null_mut()) {
let item = next_items[0];
defer! {{ CoTaskMemFree(item as *mut c_void); }}
Tests are passing! I'm probably leaking memory though, will take a closer look at that soon (please jump in if you notice any obvious problems). I managed to simplify some things with Mr. Chen's help:
|
I am very impressed by your ability to not only dig in and make it work but to also find simpler ways of doing the same thing in an API as big as the Mount Everest! Thank you! |
This might be ready for review. I spent some time trying to understand whether I need to explicitly free memory, and... I'm not always sure 😞. In some situations, freeing is handled for us. For example, let item2: IShellItem2 = item.cast()?;
let original_location_variant: PROPVARIANT = item2.GetProperty(&SCID_ORIGINAL_LOCATION)?;
let original_location_bstr: BSTR = PropVariantToBSTR(&original_location_variant)?; Looking into TakeawayIf someone more experience in this area (@ArturKovacs?) could look over the current PR with an eye toward memory usage, that would be greatly appreciated. If not, I can try to find some time next week to understand this better; maybe there are tools I can use or maybe I should just try running the code repeatedly to see if memory usage goes up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your work and the summary! It's pure insanity to see how incompletely documented an OS API seems to be in terms of ownership and how much of a minefield even simple tasks or even calls seem to be. Maybe I am just spoiled by Rust but I do firmly believe that the windows
crates have to get to a point where all that is automatic and safe.
That said, I am approving the PR, but since my bar is so very low (compilation + tests are green) I think @ArturKovacs should be the one driving the merge :D.
Agreed that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny remark, but this PR looks good to me
src/windows.rs
Outdated
original_parent: PathBuf::from(orig_loc), | ||
time_deleted: date_deleted, | ||
}); | ||
let recycle_bin = recycle_bin.assume_init().expect("not initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expect
function panics if recycle_bin
is None
. I understand that this entire crate is useless if we fail to get the recycle bin, but in my opinion a lib-type crate should avoid panicking as much as possible.
I think this should instead be something like this:
let recycle_bin = recycle_bin.assume_init().ok_or(Error::Unknown {
description: "SHGetKnownFolderItem gave NULL for FOLDERID_RecycleBinFolder".into()
})?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, will make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Some(item) => { | ||
let id = get_display_name(item, SIGDN_DESKTOPABSOLUTEPARSING)?; | ||
let name = get_display_name(item, SIGDN_PARENTRELATIVE)?; | ||
let item2: IShellItem2 = item.cast()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow I wouldn't have considered that it's possible to cast these IShellItem
s into IShellItem2
. Well spotted!
When you call The Dropping the
|
@ArturKovacs Thanks! IShellItem
impl Drop for IUnknown {
fn drop(&mut self) {
unsafe {
(self.vtable().Release)(core::mem::transmute_copy(self));
}
}
} So I think that's taken care of for us. PROPVARIANT
Under the hood pub struct PROPVARIANT {
pub Anonymous: PROPVARIANT_0,
}
...
pub union PROPVARIANT_0 {
pub Anonymous: ::core::mem::ManuallyDrop<PROPVARIANT_0_0>,
pub decVal: super::super::super::Foundation::DECIMAL,
} BSTR
Thankfully it looks like impl ::core::ops::Drop for BSTR {
fn drop(&mut self) {
if !self.0.is_null() {
unsafe { SysFreeString(self as &Self) }
}
}
} |
Yeah I also think it's all good as it is. |
@ArturKovacs Please feel free to merge. GitHub still thinks a change is requested, but in any case when merged I can cut a new release. Thanks. |
Thanks a lot for merging! Here is the new release 🎉. |
This is not quite done but I'm putting it up as a draft in case people want to provide feedback.
Building on #49, I'm able to get everything compiling but the tests crash in the
list()
function:I suspect I'm doing something wrong with the way I create an
ITEMIDLIST
:let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit();
I'll revisit this in a few days; my brain is fried from working at the intersection of unsafe Rust and Win32.