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`~~.
-- v2: ntdll: Unify get_file_info and fd_get_file_info.
From: "Erich E. Hoover" erich.e.hoover@gmail.com
Co-authored-by: Joel Holdsworth joel@airwebreathe.org.uk Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- dlls/ntdll/unix/file.c | 80 ++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 45 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 7eb8dbe7ad4..406ea176842 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -1451,39 +1451,39 @@ 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 ); + + if (fd == AT_FDCWD) ret = lstat( path, st ); + else ret = fstat( fd, st ); if (ret == -1) return ret; + if (S_ISLNK( st->st_mode )) { ret = stat( path, st ); @@ -1491,19 +1491,9 @@ static int get_file_info( const char *path, struct stat *st, ULONG *attr ) /* 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 +2031,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 +3986,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 +4031,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 +4148,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 +4159,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 +4176,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 +4186,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 +4194,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 +4277,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 +4302,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 +4316,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;
On Tue Sep 27 16:32:20 2022 +0000, Erich Hoover wrote:
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.
Just applied the change.
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.
@julliard Are you talking about the unification as a whole, or the `AT_EMPTY_PATH` discussion above (that just got fixed)? I recommended to @jhol to unify these routines in order to simplify the DOS Attributes (!924) implementation, since that also needs to be called from both paths.
@jhol You also need the `stat` inside `if (S_ISLNK( st->st_mode ))` to be behind `if (fd == AT_FDCWD)`, otherwise the fd path will `stat` an empty pathname.
@julliard Are you talking about the unification as a whole, or the `AT_EMPTY_PATH` discussion above (that just got fixed)?
The whole thing. If there's something else that needs to be done by both functions it can be done in a common helper.
On Tue Sep 27 17:03:14 2022 +0000, Alexandre Julliard wrote:
@julliard Are you talking about the unification as a whole, or the
`AT_EMPTY_PATH` discussion above (that just got fixed)? The whole thing. If there's something else that needs to be done by both functions it can be done in a common helper.
@jhol Then I would recommend closing this merge request.
This merge request was closed by Joel Holdsworth.
-
Alexandre Julliard (＠julliard) -
Erich E. Hoover
-
Erich Hoover (＠ehoover)
-
Joel Holdsworth (＠jhol)