[PATCH 0/1] MR942: ntdll: Unify get_file_info and fd_get_file_info.
In the ntdll file handling code, there are currently two duplicated file info setting functions: `get_file_info` and `fd_get_file_info`. One pasted on path strings, and the other based on file descriptors respectively. This patch set simplifies the code by unifying both functions into a single function powered by `fstatat` instead of `lstat` and `fstat`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/942
From: "Erich E. Hoover" <erich.e.hoover(a)gmail.com> Co-authored-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- dlls/ntdll/unix/file.c | 79 ++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 46 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 7eb8dbe7ad4..071484f4b02 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -1451,59 +1451,46 @@ static inline ULONG get_file_attributes( const struct stat *st ) } -static BOOL fd_is_mount_point( int fd, const struct stat *st ) +static BOOL is_mount_point( int fd, const char *path, const struct stat *st ) { + char *parent_path = NULL; struct stat parent; - return S_ISDIR( st->st_mode ) && !fstatat( fd, "..", &parent, 0 ) - && (parent.st_dev != st->st_dev || parent.st_ino == st->st_ino); -} + int ret; + if (!S_ISDIR( st->st_mode )) return FALSE; -/* get the stat info and file attributes for a file (by file descriptor) */ -static int fd_get_file_info( int fd, unsigned int options, struct stat *st, ULONG *attr ) -{ - int ret; + if (fd == AT_FDCWD) + { + if (!(parent_path = malloc( strlen(path) + 4 ))) return FALSE; + strcpy( parent_path, path ); + strcat( parent_path, "/.." ); + } - *attr = 0; - ret = fstat( fd, st ); - if (ret == -1) return ret; - *attr |= get_file_attributes( st ); - /* consider mount points to be reparse points (IO_REPARSE_TAG_MOUNT_POINT) */ - if ((options & FILE_OPEN_REPARSE_POINT) && fd_is_mount_point( fd, st )) - *attr |= FILE_ATTRIBUTE_REPARSE_POINT; - return ret; + ret = fstatat( fd, parent_path ? parent_path : "..", &parent, 0 ); + if (parent_path) free( parent_path ); + if (ret) return FALSE; + return (parent.st_dev != st->st_dev || parent.st_ino == st->st_ino); } -/* get the stat info and file attributes for a file (by name) */ -static int get_file_info( const char *path, struct stat *st, ULONG *attr ) +/* get the stat info and file attributes for a file (by name or file descriptor) */ +static int get_file_info( int fd, const char *path, unsigned int options, struct stat *st, ULONG *attr ) { - char *parent_path; int ret; *attr = 0; - ret = lstat( path, st ); + ret = fstatat( fd, path, st, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW); if (ret == -1) return ret; if (S_ISLNK( st->st_mode )) { - ret = stat( path, st ); + ret = fstatat( fd, path, st, AT_EMPTY_PATH ); if (ret == -1) return ret; /* is a symbolic link and a directory, consider these "reparse points" */ if (S_ISDIR( st->st_mode )) *attr |= FILE_ATTRIBUTE_REPARSE_POINT; } - else if (S_ISDIR( st->st_mode ) && (parent_path = malloc( strlen(path) + 4 ))) - { - struct stat parent_st; - - /* consider mount points to be reparse points (IO_REPARSE_TAG_MOUNT_POINT) */ - strcpy( parent_path, path ); - strcat( parent_path, "/.." ); - if (!stat( parent_path, &parent_st ) - && (st->st_dev != parent_st.st_dev || st->st_ino == parent_st.st_ino)) - *attr |= FILE_ATTRIBUTE_REPARSE_POINT; - - free( parent_path ); - } + /* consider mount points to be reparse points (IO_REPARSE_TAG_MOUNT_POINT) */ + else if ((options & FILE_OPEN_REPARSE_POINT) && is_mount_point( fd, path, st )) + *attr |= FILE_ATTRIBUTE_REPARSE_POINT; *attr |= get_file_attributes( st ); return ret; } @@ -2041,7 +2028,7 @@ static NTSTATUS get_dir_data_entry( struct dir_data *dir_data, void *info_ptr, I struct stat st; ULONG name_len, start, dir_size, attributes; - if (get_file_info( names->unix_name, &st, &attributes ) == -1) + if (get_file_info( AT_FDCWD, names->unix_name, FILE_OPEN_REPARSE_POINT, &st, &attributes ) == -1) { TRACE( "file no longer exists %s\n", names->unix_name ); return STATUS_SUCCESS; @@ -3996,7 +3983,7 @@ NTSTATUS WINAPI NtQueryFullAttributesFile( const OBJECT_ATTRIBUTES *attr, ULONG attributes; struct stat st; - if (get_file_info( unix_name, &st, &attributes ) == -1) + if (get_file_info( AT_FDCWD, unix_name, FILE_OPEN_REPARSE_POINT, &st, &attributes ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) status = STATUS_INVALID_INFO_CLASS; @@ -4041,7 +4028,7 @@ NTSTATUS WINAPI NtQueryAttributesFile( const OBJECT_ATTRIBUTES *attr, FILE_BASIC ULONG attributes; struct stat st; - if (get_file_info( unix_name, &st, &attributes ) == -1) + if (get_file_info( AT_FDCWD, unix_name, FILE_OPEN_REPARSE_POINT, &st, &attributes ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) status = STATUS_INVALID_INFO_CLASS; @@ -4158,7 +4145,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, switch (class) { case FileBasicInformation: - if (fd_get_file_info( fd, options, &st, &attr ) == -1) + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) status = STATUS_INVALID_INFO_CLASS; @@ -4169,7 +4156,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_STANDARD_INFORMATION *info = ptr; - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else { fill_file_info( &st, attr, info, class ); @@ -4186,7 +4173,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, } break; case FileInternalInformation: - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else fill_file_info( &st, attr, ptr, class ); break; case FileEaInformation: @@ -4196,7 +4183,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, } break; case FileEndOfFileInformation: - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else fill_file_info( &st, attr, ptr, class ); break; case FileAllInformation: @@ -4204,7 +4191,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, FILE_ALL_INFORMATION *info = ptr; char *unix_name; - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) status = STATUS_INVALID_INFO_CLASS; else if (!(status = server_get_unix_name( handle, &unix_name ))) @@ -4287,7 +4274,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, ULONG attributes; struct stat st; - if (get_file_info( unix_name, &st, &attributes ) == -1) + if (get_file_info( AT_FDCWD, unix_name, FILE_OPEN_REPARSE_POINT, &st, &attributes ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) status = STATUS_INVALID_INFO_CLASS; @@ -4312,7 +4299,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, } break; case FileIdInformation: - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else { struct mountmgr_unix_drive drive; @@ -4326,13 +4313,13 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, } break; case FileAttributeTagInformation: - if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); + if (get_file_info( fd, "", options, &st, &attr ) == -1) status = errno_to_status( errno ); else { FILE_ATTRIBUTE_TAG_INFORMATION *info = ptr; info->FileAttributes = attr; info->ReparseTag = 0; /* FIXME */ - if ((options & FILE_OPEN_REPARSE_POINT) && fd_is_mount_point( fd, &st )) + if ((options & FILE_OPEN_REPARSE_POINT) && is_mount_point( fd, "", &st )) info->ReparseTag = IO_REPARSE_TAG_MOUNT_POINT; } break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/942
This merge request was approved by Joel Holdsworth. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/942
The reason that I originally posted this as two separate commits was that I didn't know if `AT_EMPTY_PATH` was available on other platforms. Clearly it's not available on at least Mac OS, so I would suggest that instead of using `fstatat` here that we do something like the following: ``` ... if (fd == AT_FDCWD) ret = lstat( path, st ); else ret = fstat( fd, st ); if (ret == -1) return ret; if (S_ISLNK( st->st_mode )) { if (fd == AT_FDCWD) ret = stat( path, st ); ... ``` We could also introduce an internal fstatat-like routine, but that may not be worth the trouble given that this is the only place (that I'm aware of) that would benefit from this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/942#note_9402
I don't think it's worth the trouble, both functions are only a few lines. It's definitely not worth adding potentially non-portable constructs for this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/942#note_9414
participants (4)
-
Alexandre Julliard (＠julliard) -
Erich E. Hoover
-
Erich Hoover (＠ehoover)
-
Joel Holdsworth (＠jhol)