[PATCH 0/1] MR1665: kernelbase: Let GetFileInformationByHandle succeed on unix redirected handle.
As these handles to a file don't have an unix-name attached, the FileAllInformation query fails. So only implement GetFileInformationByHandle() on top of queries which don't fail. I tested if the three ntdll calls instead of one did impact performance. On cached handles they don't: a simple x100000 loop even shows a ~10% improvement, likely because we don't grab the filename from wineserver. Note: that NtQueryInformationFile(FileAllInformation) (as other queries returning file's name) will still fail on these unix-redirected handles. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51813 Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665
From: Eric Pouech <eric.pouech(a)gmail.com> As these handles to a file don't have an unix-name attached, the FileAllInformation query fails. So only implement GetFileInformationByHandle() on top of queries which don't fail. I tested if the three ntdll calls instead of one did impact performance. On cached handles they don't: a simple x100000 loop even shows a ~10% improvement, likely because we don't grab the filename from wineserver. Note: that NtQueryInformationFile(FileAllInformation) (as other queries returning file's name) will still fail on these unix-redirected handles. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51813 Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/kernelbase/file.c | 44 +++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index ac04388acde..cf8215d020b 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -2923,27 +2923,39 @@ BOOL WINAPI DECLSPEC_HOTPATCH FlushFileBuffers( HANDLE file ) BOOL WINAPI DECLSPEC_HOTPATCH GetFileInformationByHandle( HANDLE file, BY_HANDLE_FILE_INFORMATION *info ) { FILE_FS_VOLUME_INFORMATION volume_info; - FILE_ALL_INFORMATION all_info; + FILE_BASIC_INFORMATION basic_info; + FILE_STANDARD_INFORMATION standard_info; + FILE_INTERNAL_INFORMATION internal_info; IO_STATUS_BLOCK io; NTSTATUS status; - status = NtQueryInformationFile( file, &io, &all_info, sizeof(all_info), FileAllInformation ); - if (status == STATUS_BUFFER_OVERFLOW) status = STATUS_SUCCESS; - if (!set_ntstatus( status )) return FALSE; + /* We're not using FileAllInformation as it fails on file handles without unix_name + * (like file from unix redirection). + */ + if ((status = NtQueryInformationFile( file, &io, &basic_info, + sizeof(basic_info), FileBasicInformation )) || + (status = NtQueryInformationFile( file, &io, &standard_info, + sizeof(standard_info), FileStandardInformation )) || + (status = NtQueryInformationFile( file, &io, &internal_info, + sizeof(internal_info), FileInternalInformation ))) + { + set_ntstatus( status ); + return FALSE; + } - info->dwFileAttributes = all_info.BasicInformation.FileAttributes; - info->ftCreationTime.dwHighDateTime = all_info.BasicInformation.CreationTime.u.HighPart; - info->ftCreationTime.dwLowDateTime = all_info.BasicInformation.CreationTime.u.LowPart; - info->ftLastAccessTime.dwHighDateTime = all_info.BasicInformation.LastAccessTime.u.HighPart; - info->ftLastAccessTime.dwLowDateTime = all_info.BasicInformation.LastAccessTime.u.LowPart; - info->ftLastWriteTime.dwHighDateTime = all_info.BasicInformation.LastWriteTime.u.HighPart; - info->ftLastWriteTime.dwLowDateTime = all_info.BasicInformation.LastWriteTime.u.LowPart; + info->dwFileAttributes = basic_info.FileAttributes; + info->ftCreationTime.dwHighDateTime = basic_info.CreationTime.u.HighPart; + info->ftCreationTime.dwLowDateTime = basic_info.CreationTime.u.LowPart; + info->ftLastAccessTime.dwHighDateTime = basic_info.LastAccessTime.u.HighPart; + info->ftLastAccessTime.dwLowDateTime = basic_info.LastAccessTime.u.LowPart; + info->ftLastWriteTime.dwHighDateTime = basic_info.LastWriteTime.u.HighPart; + info->ftLastWriteTime.dwLowDateTime = basic_info.LastWriteTime.u.LowPart; info->dwVolumeSerialNumber = 0; - info->nFileSizeHigh = all_info.StandardInformation.EndOfFile.u.HighPart; - info->nFileSizeLow = all_info.StandardInformation.EndOfFile.u.LowPart; - info->nNumberOfLinks = all_info.StandardInformation.NumberOfLinks; - info->nFileIndexHigh = all_info.InternalInformation.IndexNumber.u.HighPart; - info->nFileIndexLow = all_info.InternalInformation.IndexNumber.u.LowPart; + info->nFileSizeHigh = standard_info.EndOfFile.u.HighPart; + info->nFileSizeLow = standard_info.EndOfFile.u.LowPart; + info->nNumberOfLinks = standard_info.NumberOfLinks; + info->nFileIndexHigh = internal_info.IndexNumber.u.HighPart; + info->nFileIndexLow = internal_info.IndexNumber.u.LowPart; status = NtQueryVolumeInformationFile( file, &io, &volume_info, sizeof(volume_info), FileFsVolumeInformation ); if (status == STATUS_SUCCESS || status == STATUS_BUFFER_OVERFLOW) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1665
This is doing 3 ntdll calls where before we had one. Is this fixable in some in ntdll instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_18561
I'm reluctant fixing in ntdll as: - it would require giving a name (even empty) to these handles to file being constructed on unix fd and expose it to kernel* while we currently fail. There's no clean and portable way to get back to file name (in the case where fd is an opened file) - there's no need in GetFileInformationByHandle to get the name - the sum of the 3 ntdll calls is faster than the existing ntdll query -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19550
I'm reluctant fixing in ntdll as:
* it would require giving a name (even empty) to these handles to file being constructed on unix fd and expose it to kernel\* while we currently fail. There's no clean and portable way to get back to file name (in the case where fd is an opened file) * there's no need in GetFileInformationByHandle to get the name
Shouldn't it be possible to query the name from such files on Windows anyway, though? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19585
On Wed Dec 14 07:48:00 2022 +0000, Zebediah Figura wrote:
I'm reluctant fixing in ntdll as:
* it would require giving a name (even empty) to these handles to file being constructed on unix fd and expose it to kernel\* while we currently fail. There's no clean and portable way to get back to file name (in the case where fd is an opened file) * there's no need in GetFileInformationByHandle to get the name Shouldn't it be possible to query the name from such files on Windows anyway, though? These handles are created by Wine when inheriting unix's fd 0,1,2. So they don't really exist per se on Windows. Currently, all NtQueryInformation returning a name fail on such handles. Please advise which name should be exposed?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19613
On Wed Dec 14 07:48:00 2022 +0000, eric pouech wrote:
These handles are created by Wine when inheriting unix's fd 0,1,2. So they don't really exist per se on Windows. Currently, all NtQueryInformation returning a name fail on such handles. Please advise which name should be exposed? Well, I mean it's always possible to query the name of an open file, right?
It's unfortunate that POSIX doesn't provide an API for this, but in the absence of that, it still seems to me like using platform-specific APIs is the right thing to do. Or, failing that, perhaps we should provide a fake name, like an empty string. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19639
On 12/14/22 12:04, Zebediah Figura (@zfigura) wrote:
On Wed Dec 14 07:48:00 2022 +0000, eric pouech wrote:
These handles are created by Wine when inheriting unix's fd 0,1,2. So they don't really exist per se on Windows. Currently, all NtQueryInformation returning a name fail on such handles. Please advise which name should be exposed? Well, I mean it's always possible to query the name of an open file, right?
It's unfortunate that POSIX doesn't provide an API for this, but in the absence of that, it still seems to me like using platform-specific APIs is the right thing to do. Or, failing that, perhaps we should provide a fake name, like an empty string.
This is probably the third approach to this issue. Just FYI, there is Gabriel's patch which is now in Proton: https://github.com/ValveSoftware/wine/commit/e9565a4160533f5236e635849495d42.... That was sent upstream back then but didn't go in, there could be some conversation about that in wine-devel.
On Wed Dec 14 18:41:55 2022 +0000, **** wrote:
Paul Gofman replied on the mailing list: ``` On 12/14/22 12:04, Zebediah Figura (@zfigura) wrote:
On Wed Dec 14 07:48:00 2022 +0000, eric pouech wrote:
These handles are created by Wine when inheriting unix's fd 0,1,2. So they don't really exist per se on Windows. Currently, all NtQueryInformation returning a name fail on such handles. Please advise which name should be exposed? Well, I mean it's always possible to query the name of an open file, right?
It's unfortunate that POSIX doesn't provide an API for this, but in the absence of that, it still seems to me like using platform-specific APIs is the right thing to do. Or, failing that, perhaps we should provide a fake name, like an empty string.
This is probably the third approach to this issue. Just FYI, there is Gabriel's patch which is now in Proton: https://github.com/ValveSoftware/wine/commit/e9565a4160533f5236e635849495d42.... That was sent upstream back then but didn't go in, there could be some conversation about that in wine-devel. ``` My patch is also in wine-staging, but I just checked, apparently it's disabled now (didn't realize it before, maybe I can try to rebase it but that depends how much the server code changed): [server-unix_name](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-uni...)
If someone wants to try to upstream it please feel free to, same as the other wine-staging patches from me. But obviously after code freeze though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19650
My patch is also in wine-staging, but I just checked, apparently it's disabled now (didn't realize it before, maybe I can try to rebase it but that depends how much the server code changed): [server-unix_name](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-uni...)
I actually meant to reënable it in 533337e25, but evidently I forgot to actually reënable it :-/ -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19651
On Wed Dec 14 20:27:56 2022 +0000, Zebediah Figura wrote:
My patch is also in wine-staging, but I just checked, apparently it's disabled now (didn't realize it before, maybe I can try to rebase it but that depends how much the server code changed): [server-unix_name](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-uni...) I actually meant to reënable it in 533337e25, but evidently I forgot to actually reënable it :-/ Nice. At least my comment indirectly worked as a reminder :)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19652
On Wed Dec 14 20:42:06 2022 +0000, Gabriel Ivăncescu wrote: > Nice. At least my comment indirectly worked as a reminder :) Trying to get a name for a fd won't cover all the use cases: - AFAICT not all the unices provide a reliable interface (current code in wine staging covers at least linux, netbsd & macos) - fd:s not mapped to a file won't get a name. so cases like 'wine foo | dosomething' or 'wine foo > /dev/null' won't be supported. Getting a name for a fd bound to a file would be an improvement in Wine, but it can cover all the cases considered in the bug [](https://bugs.winehq.org/show_bug.cgi?id=51813). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19672
On Thu Dec 15 09:22:18 2022 +0000, eric pouech wrote:
Trying to get a name for a fd won't cover all the use cases: - AFAICT not all the unices provide a reliable interface (current code in wine staging covers at least linux, netbsd & macos) - fd:s not mapped to a file won't get a name. so cases like 'wine foo | dosomething' or 'wine foo > /dev/null' won't be supported. Getting a name for a fd bound to a file would be an improvement in Wine, but it can cover all the cases considered in the bug [](https://bugs.winehq.org/show_bug.cgi?id=51813). and I forgot in the list of not supported case the fd:s gotten from current (p)tty (so the handles mapped to fd 0,1 & 2)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_19673
On Thu Dec 15 09:28:04 2022 +0000, eric pouech wrote:
and I forgot in the list of not supported case the fd:s gotten from current (p)tty (so the handles mapped to fd 0,1 & 2) seems @epo has a point here
any other remark preventing this from being merged ? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_22586
no agreement on proposal, no clear & valid alternate proposal. hence closing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665#note_23207
This merge request was closed by eric pouech. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1665
participants (7)
-
Eric Pouech -
eric pouech (＠epo)
-
Gabriel Ivăncescu
-
Michel Zou (＠xantares)
-
Nikolay Sivov (＠nsivov)
-
Paul Gofman -
Zebediah Figura (＠zfigura)