[PATCH 0/1] MR942: ntdll: Unify get_file_info and fd_get_file_info.

[Joel Holdsworth (＠jhol)]

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`.

[PATCH 1/1] ntdll: Unify get_file_info and fd_get_file_info.

[Erich E. Hoover]

From: "Erich E. Hoover"

Co-authored-by: Joel Holdsworth 
Signed-off-by: Joel Holdsworth 
---
 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;

[PATCH 0/1] MR942: ntdll: Unify get_file_info and fd_get_file_info. - approved

[Joel Holdsworth (＠jhol)]

This merge request was approved by Joel Holdsworth.

[Erich Hoover (＠ehoover)]

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.

[Alexandre Julliard (＠julliard)]

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.