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

Better support for os::windows::fs::MetadataExt on uwp #86075

Closed
wants to merge 3 commits into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jun 6, 2021

Currently to get metadata we use GetFileInformationByHandle by default on Windows, and GetFileInformationByHandleEx on UWP. This PR makes two changes to os::windows::fs::MetadataExt:

  • Extend the documentation to not only mention the GetFileInformationByHandle fields, but also the equivalent GetFileInformationByHandleEx fields.
  • Rename the unstable file_index method (tracking issue: Tracking issue for fs::Metadata extensions on Windows based on handle information #63010) to file_identifier and change the return type from Option<u64> to Option<u128>. This allows file_identifier and volume_serial_number to be implemented on UWP using FILE_ID_INFO, previously they would always return None.

@rustbot rustbot added A-FFI Area: Foreign function interface (FFI) O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 6, 2021
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Jun 6, 2021

Is there a reason we don't always use GetFileInformationByHandleEx? As far as I can tell almost all values would be the same, except for file_index/file_identifier which would be a 128-bit value instead of a 64-bit value. The 64-bit value returned by GetFileInformationByHandle is not guaranteed to be unique on some filesystems, which makes it not possible to determining if two files are the same; from the FILE_ID_INFO docs: "To determine whether two open handles represent the same file, combine the identifier and the volume serial number for each file and compare them."

@ChrisDenton
Copy link
Member

Is the FileIdInfo class available in all supported OS versions (e.g. 7, 10, UWP)?

@CDirkx
Copy link
Contributor Author

CDirkx commented Jun 6, 2021

Not exactly sure, GetFileInformationByHandleEx itself is supported on Vista, 7, 8, 10 and UWP, however the FILE_ID_INFO docs have:

Requirements

Minimum supported client None supported
Minimum supported server Windows Server 2012 [desktop apps only]
Header winbase.h (include Windows.h)

Which I initially interpreted as not yet being supported anywhere, but in a test I did (see below) it was working fine, at least on Windows 10.

#![feature(with_options)]

use std::path::Path;

mod c {
    #[repr(C)]
    pub enum FILE_INFO_BY_HANDLE_CLASS {
        FileBasicInfo = 0,
        FileStandardInfo = 1,
        FileNameInfo = 2,
        FileRenameInfo = 3,
        FileDispositionInfo = 4,
        FileAllocationInfo = 5,
        FileEndOfFileInfo = 6,
        FileStreamInfo = 7,
        FileCompressionInfo = 8,
        FileAttributeTagInfo = 9,
        FileIdBothDirectoryInfo = 10,        // 0xA
        FileIdBothDirectoryRestartInfo = 11, // 0xB
        FileIoPriorityHintInfo = 12,         // 0xC
        FileRemoteProtocolInfo = 13,         // 0xD
        FileFullDirectoryInfo = 14,          // 0xE
        FileFullDirectoryRestartInfo = 15,   // 0xF
        FileStorageInfo = 16,                // 0x10
        FileAlignmentInfo = 17,              // 0x11
        FileIdInfo = 18,                     // 0x12
        FileIdExtdDirectoryInfo = 19,        // 0x13
        FileIdExtdDirectoryRestartInfo = 20, // 0x14
        MaximumFileInfoByHandlesClass,
    }

    #[repr(C)]
    #[derive(Debug)]
    pub struct FILE_ID_INFO {
        pub VolumeSerialNumber: u64,
        pub FileId: u128
    }

    extern "system" {
        pub fn GetFileInformationByHandleEx(
            hFile: *mut std::ffi::c_void,
            fileInfoClass: FILE_INFO_BY_HANDLE_CLASS,
            lpFileInformation: *mut std::ffi::c_void,
            dwBufferSize: u32,
        ) -> i32;
    }
}

fn main() {
    unsafe {
        use std::os::windows::prelude::*;

        let handle = std::fs::File::with_options().access_mode(0).open("a/real.file").unwrap().into_raw_handle();

        let mut info: c::FILE_ID_INFO = std::mem::zeroed();
        let result = c::GetFileInformationByHandleEx(
            handle,
            c::FILE_INFO_BY_HANDLE_CLASS::FileIdInfo,
            &mut info as *mut _ as *mut std::ffi::c_void,
            std::mem::size_of_val(&info) as u32
        );

        if result == 0 {
            panic!("{}", std::io::Error::last_os_error())
        } else {
            // prints "FILE_ID_INFO { VolumeSerialNumber: xxxxxxxxxxxxxxxxx, FileId: xxxxxxxxxxxxxxxxx }" on Windows 10
            println!("{:?}", info);
        }
    }
}

@CDirkx
Copy link
Contributor Author

CDirkx commented Jun 6, 2021

Based on https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_descriptor it seems that 128 bit file identifiers are not supported before Windows 8, so I think FILE_ID_INFO is not available on Windows 7 (but it should be on UWP?).

This means always using GetFileInformationByHandleEx is not possible, however we could still use GetFileInformationByHandleEx on all platforms except 7, at least to get a 128-bit file identifier. Using the 64-bit identifier (casted to an u128) on Windows 7 should be fine, as it doesn't support any file systems that actually use 128-bit identifiers, so no collisions are possible.

@ChrisDenton
Copy link
Member

Yeah, that table looks suspiciously like it was auto-generated without the relevant data. Peeking at the Windows header files, it says that it was introduced in Windows 8:

#if (NTDDI_VERSION >= NTDDI_WIN8)
    FileStorageInfo,
    FileAlignmentInfo,
    FileIdInfo,
    FileIdExtdDirectoryInfo,
    FileIdExtdDirectoryRestartInfo,
#endif

I guess if you want to use FileIdInfo on all supported versions you could test for ERROR_INVALID_PARAMETER and use a fallback?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

The fix itself shouldn't have a compatibility issue and looks good to me. On file_index, renaming and extending the return type to u128 would need a t-libs-api member's review (but it looks reasonable to me).

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
let mut info: c::FILE_ID_INFO = mem::zeroed();
let size = mem::size_of_val(&info);
cvt(c::GetFileInformationByHandleEx(
self.handle.raw(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Given I/O safety has been merged, as_raw_handle should be used instead of raw.

@bors
Copy link
Contributor

bors commented Oct 15, 2021

☔ The latest upstream changes (presumably #84096) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Ping from triage:
@CDirkx Can you please address the comment and merge conflict issues?

Please adjust the label with @rustbot ready when you're ready for another review.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@CDirkx
I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thank you.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Feb 13, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) O-windows Operating system: Windows S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.