feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801
Conversation
| Ok(unix_time_msec as u64) | ||
| } | ||
|
|
||
| const WINDOWS_TICK: i64 = 10_000; // 100-nanosecond intervals in a millisecond |
There was a problem hiding this comment.
I don't think that this is a good place for consts, but had no idea where to put them, so I will be happy for some suggestions
ext/fs/std_fs.rs
Outdated
|
|
||
| let file_info = { | ||
| let mut file_info = std::mem::MaybeUninit::<FILE_BASIC_INFO>::zeroed(); | ||
| if GetFileInformationByHandleEx( |
There was a problem hiding this comment.
Before creating this method, I tried to take the value for ChangeTime from the NtQueryInformationFile used below, but it returns 0.
There was a problem hiding this comment.
Are you sure about this? Libuv (which is what Node uses under the hood) gets this information from the NtQueryInformationFile syscall: https://github.com/libuv/libuv/blob/4e310d0f90af29e699e2dedad5fa0dcee181a7cc/src/win/fs.c#L1751-L1793
I think it's quite unfortunate that we would have to do another syscall just to obtain the ctime value.
There was a problem hiding this comment.
I'm pretty sure that was the case, but let me check again to make sure.
There was a problem hiding this comment.
So I did some digging, and here are my findings, it would be great if someone could check it on their machine, too.
In my case, at least the NtQueryInformationFile doesn't work completely!
I checked the status code for this call. It always returns some big (which might not matter) negative number meaning that it failed to get the data, but on the other hand std::io::Error::last_os_error() returns an object with a status code of 0 and information that it was successful.
So I found that we close a file handle in code before actually using it in the query_file_information method.
but even moving it below the call doesn't change anything at all and NtQueryInformationFile still fails the same way as before.
Now I tried to use the NtQueryInformationFile method in just plain rust program and even tried to get only basic info instead of the full one, but it also failed. I tried that on two different machines with Windows 11 64bit and ARM on board, and it doesn't work in both.
If someone could check on their end whether this is only my machine or if this doesn't seem to work at all, that would be great.
There was a problem hiding this comment.
NtQueryInformationFile doesn't set last_os_error() upon failure. Instead, it returns an NTSTATUS code and you have to convert it to a win32 error (using RtlNtStatusToDosError) manually to know what the error was.
There was a problem hiding this comment.
Good to know, thanks, I assumed this cause that's how handling is done in deno code right now. I will have a look at this then, what about the CloseHandle placement, shouldn't it be after each call that uses the handle?
There was a problem hiding this comment.
It looks like the call to RtlNtStatusToDosError is entirely missing from the current code base, which means that when NtQueryInformationFile fails we are not currently reporting an appropriate error code.
There was a problem hiding this comment.
Thank you so much for your help! I'm not an expert on a Windows API topic (it is my first time touching it), but it was a lot of fun to learn about it! 😄. I managed to fix the NtQueryInformationFile call and get all the data from a single call, and I have added some proper error handling. Let me know if there is something else that can be improved.
| @@ -862,10 +883,11 @@ fn stat_extra( | |||
| } | |||
|
|
|||
| let result = get_dev(file_handle); | |||
| CloseHandle(file_handle); | |||
There was a problem hiding this comment.
This one needs to be moved after we use the handle in NtQueryInformationFile otherwise ERROR_NOACCESS(998) is returned
| let status = NtQueryInformationFile( | ||
| handle as _, | ||
| std::ptr::null_mut(), | ||
| io_status_block.as_mut_ptr(), |
There was a problem hiding this comment.
With null ptr it will crash resulting in ERROR_NOACCESS (998) code
| // since struct is populated with other data anyway. | ||
| /// https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntqueryinformationfile#remarks | ||
|
|
||
| if converted_status != ERROR_MORE_DATA { |
There was a problem hiding this comment.
According to Microsoft docs (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntqueryinformationfile#remarks) it is fine to ignore it as long as we don't care about full file name "If NtQueryInformationFile fails because of a buffer overflow, drivers that implement FileNameInformation should return as many WCHAR characters of the file name as will fit in the buffer and specify the full length that is required in the FileNameLength parameter of the FILE_NAME_INFORMATION structure.". To make it work without an error, we would have to query the sys call again with size buffer of struct + length of the file name, as the FILE_ALL_INFORMATION has variable size in FILE_NAME_INFORMATION struct (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/ns-ntddk-_file_name_information)
lczerniawski
changed the title
| ino: unix ? response.ino : null, | ||
| mode: unix ? response.mode : null, |
There was a problem hiding this comment.
To explain why I removed that, I don't think this is true anymore; after a fix in this PR which now results in `NtQueryInformationFile' working correctly, we indeed set the mode in std_fs.rs lines 880/883/891/894.
There was a problem hiding this comment.
This part looks fairly independent of the main change of this PR. Can you split this to another PR?
There was a problem hiding this comment.
Like the whole .js file change? Yeah, I can do that if you think it would be better. For me, it is more of a whole as by changing the underlying Rust structure, we should also update the JS API, but if you wish otherwise, sure, I can :)
There was a problem hiding this comment.
Like the whole .js file change?
I meant only split mode value change. I think the change of mode value can be done independently from ctime addition, or am I missing something?
There was a problem hiding this comment.
Well, I had to double-check, but as I suspected I done it cause the tests start to fail
https://github.com/denoland/deno/actions/runs/11594876322/job/32282184345?pr=24801
It assumes that for Windows mode should be empty, but after my fix to NtQueryInformationFile call, it is no longer true (and the whole code for setting mode was already there I didn't change that in any way).
So it is either changing the tests, but then tests will accommodate incorrect behavior or changing what we expose in API, honestly I can do both, but for me personally it makes more sense to just do it once and fix as it should be, but of course please let me know how you view this thing after my explanation, for now I left the code with rollbacked version of changes that fails the tests
kt3k
changed the title
|
Looks like |
No problem! That's what I meant, but I guess I didn't express myself particularly well 😄 |
This PR fixes #24453, by introducing a ctime (using ctime for UNIX and ChangeTime for Windows) to Deno.stats.
The screenshots below show a working example of the code in action.
vs SetMacu tool outputs