Re: [v2 2/3] ntdll: Implement FileIdInformation class support in NtQueryInformationFile.

[Sebastian Lackner]

On 26.01.2017 12:09, Jonathan Doron wrote:
> Signed-off-by: Jonathan Doron <jond(a)wizery.com>
> ---
>  configure           | 24 ++++++++++++++++++++++++
>  configure.ac        | 16 ++++++++++++++++
>  dlls/ntdll/file.c   | 22 +++++++++++++++++++++-
>  include/config.h.in |  3 +++
>  include/winbase.h   |  9 +++++++++
>  5 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 1a96117..a9d7878 100755
> --- a/configure
> +++ b/configure
> @@ -16407,6 +16407,30 @@ _ACEOF
>  fi
>
> +ac_fn_c_check_member "$LINENO" "struct statfs" "f_fsid.__val" "ac_cv_member_struct_statfs_f_fsid___val" "#include <sys/types.h>
> +#ifdef HAVE_SYS_PARAM_H
> +# include <sys/param.h>
> +#endif
> +#ifdef HAVE_SYS_MOUNT_H
> +# include <sys/mount.h>
> +#endif
> +#ifdef HAVE_SYS_VFS_H
> +# include <sys/vfs.h>
> +#endif
> +#ifdef HAVE_SYS_STATFS_H
> +# include <sys/statfs.h>
> +#endif
> +"
> +if test "x$ac_cv_member_struct_statfs_f_fsid___val" = xyes; then :
> +
> +cat >>confdefs.h <<_ACEOF
> +#define HAVE_STRUCT_STATFS_F_SID_VAL 1
> +_ACEOF
> +
> +
> +fi
> +
> +
>  ac_fn_c_check_member "$LINENO" "struct statvfs" "f_blocks" "ac_cv_member_struct_statvfs_f_blocks" "#ifdef HAVE_SYS_STATVFS_H
>  #include <sys/statvfs.h>
>  #endif

There is usually no need to include changes for configure. It will automatically be updated when
your patch gets accepted. The same also applies to config.h.in.
> diff --git a/configure.ac b/configure.ac
> index 353f271..3790a39 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2277,6 +2277,22 @@ AC_CHECK_MEMBERS([struct statfs.f_bfree, struct statfs.f_bavail, struct statfs.f
>  # include <sys/statfs.h>
>  #endif])
>
> +dnl Check for f_fsid.__val
> +AC_CHECK_MEMBERS([struct f_fsid.f_fsid.__val],,,

Shouldn't this be "struct statfs.[...]" ? And is there any reason why this isn't merged in the
block above, which checks other statfs fields?
> +[#include <sys/types.h>
> +#ifdef HAVE_SYS_PARAM_H
> +# include <sys/param.h>
> +#endif
> +#ifdef HAVE_SYS_MOUNT_H
> +# include <sys/mount.h>
> +#endif
> +#ifdef HAVE_SYS_VFS_H
> +# include <sys/vfs.h>
> +#endif
> +#ifdef HAVE_SYS_STATFS_H
> +# include <sys/statfs.h>
> +#endif])
> +
>  AC_CHECK_MEMBERS([struct statvfs.f_blocks],,,
>  [#ifdef HAVE_SYS_STATVFS_H
>  #include <sys/statvfs.h>
> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c
> index 6eddd19..38ca0c5 100644
> --- a/dlls/ntdll/file.c
> +++ b/dlls/ntdll/file.c
> @@ -2379,7 +2379,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io,
>          0,                                             /* FileRenameInformationBypassAccessCheck */
>          0,                                             /* FileLinkInformationBypassAccessCheck */
>          0,                                             /* FileVolumeNameInformation */
> -        0,                                             /* FileIdInformation */
> +        sizeof(FILE_ID_INFORMATION),                   /* FileIdInformation */
>          0,                                             /* FileIdExtdDirectoryInformation */
>          0,                                             /* FileReplaceCompletionInformation */
>          0,                                             /* FileHardLinkFullIdInformation */
> @@ -2621,6 +2621,26 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io,
>              }
>          }
>          break;
> +    case FileIdInformation:
> +        {
> +            struct statfs volstats;
> +            FILE_ID_INFORMATION *info = ptr;
> +            if ((fstat(fd, &st) == -1) || (fstatfs(fd, &volstats) == -1))

Other blocks using fstatfs are protected with an #if defined(HAVE_FSTATFS) check.
You might need that here, too.
> +            {
> +                io->u.Status = FILE_GetNtStatus();
> +            }
> +            else
> +            {
> +                memset(info, 0, sizeof(*info));
> +#ifdef HAVE_STRUCT_STATFS_F_SID_VAL

With my version of autoconf, the define is called HAVE_STRUCT_F_FSID_F_FSID___VAL (with two
additional underscores). Are there differences between the versions of autoconf or have you
guessed those names? Please note that you can use "autoreconf -f" to update various files
automatically after changing configure.ac.
> +                memcpy(&info->VolumeSerialNumber,
> +                       &volstats.f_fsid.__val,
> +                       min(sizeof(info->VolumeSerialNumber), sizeof(volstats.f_fsid.__val)));
> +#endif
> +                ((PLARGE_INTEGER)&info->FileId)->QuadPart = st.st_ino;

Not really an issue, but it is usually preferred to avoid P* types. You could also write:
((LARGE_INTEGER *)&info->FileId)->QuadPart = st.st_ino;

To make your changes even more convincing, I would also recommend to take a look at the existing
tests and try to add one for this information class. This could for example be used to verify
that FileId is indeed identical to the (smaller) FileId returned by some other info classes.
> +            }
> +        }
> +        break;
>      default:
>          FIXME("Unsupported class (%d)\n", class);
>          io->u.Status = STATUS_NOT_IMPLEMENTED;
> diff --git a/include/config.h.in b/include/config.h.in
> index 5dcd90b..6bba9df 100644
> --- a/include/config.h.in
> +++ b/include/config.h.in
> @@ -963,6 +963,9 @@
>  /* Define to 1 if `f_namelen' is a member of `struct statfs'. */
>  #undef HAVE_STRUCT_STATFS_F_NAMELEN
>
> +/* Define to 1 if `f_fsid.__val' is a member of `struct statfs'. */
> +#undef HAVE_STRUCT_STATFS_F_SID_VAL
> +
>  /* Define to 1 if `f_blocks' is a member of `struct statvfs'. */
>  #undef HAVE_STRUCT_STATVFS_F_BLOCKS
>
> diff --git a/include/winbase.h b/include/winbase.h
> index eff5972..83c27f8 100644
> --- a/include/winbase.h
> +++ b/include/winbase.h
> @@ -905,6 +905,15 @@ typedef struct _FILE_REMOTE_PROTOCOL_INFO {
>      } ProtocolSpecificReserved;
>  } FILE_REMOTE_PROTOCOL_INFO, *PFILE_REMOTE_PROTOCOL_INFO;

Those changes should probably go to include/winternl.h. On Windows they
are in include/ntifs.h, but we don't have that in Wine yet, and all other
FILE_*_INFORMATION structs are also in winternl.h.
> +typedef struct _FILE_ID_128 {
> +    BYTE Identifier[16];

My header files define this as UCHAR array. If there are multiple versions
out there it shouldn't matter what you use, otherwise I would suggest to
stick to what is used in official header files.
> +} FILE_ID_128, *PFILE_ID_128;
> +
> +typedef struct _FILE_ID_INFORMATION {
> +    ULONGLONG VolumeSerialNumber;
> +    FILE_ID_128 FileId;
> +} FILE_ID_INFORMATION, *PFILE_ID_INFORMATION;
> +
>  #define PIPE_ACCESS_INBOUND  1
>  #define PIPE_ACCESS_OUTBOUND 2
>  #define PIPE_ACCESS_DUPLEX   3

Feel free to ask if you have any questions.

Regards,
Sebastian