From: Joel Holdsworth joel@airwebreathe.org.uk
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53544 Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- dlls/ntdll/tests/file.c | 8 +-- dlls/ntdll/unix/file.c | 112 +++++++++++++++++++++++----------------- 2 files changed, 66 insertions(+), 54 deletions(-)
diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index 065a9251c2a..dd0061b13d8 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -1287,7 +1287,7 @@ static void test_file_full_size_information(void)
/* Assume No Quota Settings configured on Wine Testbot */ res = pNtQueryVolumeInformationFile(h, &io, &ffsi, sizeof ffsi, FileFsFullSizeInformation); - todo_wine ok(res == STATUS_SUCCESS, "cannot get attributes, res %lx\n", res); + ok(res == STATUS_SUCCESS, "cannot get attributes, res %lx\n", res); res = pNtQueryVolumeInformationFile(h, &io, &fsi, sizeof fsi, FileFsSizeInformation); ok(res == STATUS_SUCCESS, "cannot get attributes, res %lx\n", res);
@@ -1303,8 +1303,6 @@ static void test_file_full_size_information(void) ok(fsi.BytesPerSector == 512, "[fsi] BytesPerSector expected 512, got %ld\n",fsi.BytesPerSector); ok(fsi.SectorsPerAllocationUnit == 8, "[fsi] SectorsPerAllocationUnit expected 8, got %ld\n",fsi.SectorsPerAllocationUnit);
- todo_wine - { ok(ffsi.TotalAllocationUnits.QuadPart > 0, "[ffsi] TotalAllocationUnits expected positive, got negative value 0x%s\n", wine_dbgstr_longlong(ffsi.TotalAllocationUnits.QuadPart)); @@ -1322,14 +1320,10 @@ static void test_file_full_size_information(void) "[ffsi] CallerAvailableAllocationUnits error fsi:0x%s, ffsi: 0x%s\n", wine_dbgstr_longlong(fsi.AvailableAllocationUnits.QuadPart), wine_dbgstr_longlong(ffsi.CallerAvailableAllocationUnits.QuadPart)); - }
/* Assume file system is NTFS */ - todo_wine - { ok(ffsi.BytesPerSector == 512, "[ffsi] BytesPerSector expected 512, got %ld\n",ffsi.BytesPerSector); ok(ffsi.SectorsPerAllocationUnit == 8, "[ffsi] SectorsPerAllocationUnit expected 8, got %ld\n",ffsi.SectorsPerAllocationUnit); - }
CloseHandle( h ); } diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 738367dbb57..159591aff4c 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -1810,6 +1810,56 @@ static NTSTATUS fill_name_info( const char *unix_name, FILE_NAME_INFORMATION *in }
+static NTSTATUS get_full_size_info(int fd, FILE_FS_FULL_SIZE_INFORMATION *info) { + struct stat st; + ULONGLONG bsize; + +#if !defined(linux) || !defined(HAVE_FSTATFS) + struct statvfs stfs; +#else + struct statfs stfs; +#endif + + if (fstat( fd, &st ) < 0) + { + return errno_to_status( errno ); + } + if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) + { + return STATUS_INVALID_DEVICE_REQUEST; + } + + /* Linux's fstatvfs is buggy */ +#if !defined(linux) || !defined(HAVE_FSTATFS) + if (fstatvfs( fd, &stfs ) < 0) + { + return errno_to_status( errno ); + } + bsize = stfs.f_frsize; +#else + if (fstatfs( fd, &stfs ) < 0) + { + return errno_to_status( errno ); + } + bsize = stfs.f_bsize; +#endif + if (bsize == 2048) /* assume CD-ROM */ + { + info->BytesPerSector = 2048; + info->SectorsPerAllocationUnit = 1; + } + else + { + info->BytesPerSector = 512; + info->SectorsPerAllocationUnit = 8; + } + info->TotalAllocationUnits.QuadPart = bsize * stfs.f_blocks / (info->BytesPerSector * info->SectorsPerAllocationUnit); + info->CallerAvailableAllocationUnits.QuadPart = bsize * stfs.f_bavail / (info->BytesPerSector * info->SectorsPerAllocationUnit); + info->ActualAvailableAllocationUnits.QuadPart = bsize * stfs.f_bfree / (info->BytesPerSector * info->SectorsPerAllocationUnit); + return STATUS_SUCCESS; +} + + static NTSTATUS server_get_file_info( HANDLE handle, IO_STATUS_BLOCK *io, void *buffer, ULONG length, FILE_INFORMATION_CLASS info_class ) { @@ -6406,7 +6456,6 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, IO_STATUS_BLOCK *io FS_INFORMATION_CLASS info_class ) { int fd, needs_close; - struct stat st; NTSTATUS status;
status = server_get_unix_fd( handle, 0, &fd, &needs_close, NULL, NULL ); @@ -6456,52 +6505,14 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, IO_STATUS_BLOCK *io else { FILE_FS_SIZE_INFORMATION *info = buffer; + FILE_FS_FULL_SIZE_INFORMATION full_info;
- if (fstat( fd, &st ) < 0) - { - status = errno_to_status( errno ); - break; - } - if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) - { - status = STATUS_INVALID_DEVICE_REQUEST; - } - else - { - ULONGLONG bsize; - /* Linux's fstatvfs is buggy */ -#if !defined(linux) || !defined(HAVE_FSTATFS) - struct statvfs stfs; - - if (fstatvfs( fd, &stfs ) < 0) - { - status = errno_to_status( errno ); - break; - } - bsize = stfs.f_frsize; -#else - struct statfs stfs; - if (fstatfs( fd, &stfs ) < 0) - { - status = errno_to_status( errno ); - break; - } - bsize = stfs.f_bsize; -#endif - if (bsize == 2048) /* assume CD-ROM */ - { - info->BytesPerSector = 2048; - info->SectorsPerAllocationUnit = 1; - } - else - { - info->BytesPerSector = 512; - info->SectorsPerAllocationUnit = 8; - } - info->TotalAllocationUnits.QuadPart = bsize * stfs.f_blocks / (info->BytesPerSector * info->SectorsPerAllocationUnit); - info->AvailableAllocationUnits.QuadPart = bsize * stfs.f_bavail / (info->BytesPerSector * info->SectorsPerAllocationUnit); + if ((status = get_full_size_info(fd, &full_info)) == STATUS_SUCCESS) { + info->TotalAllocationUnits = full_info.TotalAllocationUnits; + info->AvailableAllocationUnits = full_info.CallerAvailableAllocationUnits; + info->SectorsPerAllocationUnit = full_info.SectorsPerAllocationUnit; + info->BytesPerSector = full_info.BytesPerSector; io->Information = sizeof(*info); - status = STATUS_SUCCESS; } } break; @@ -6645,8 +6656,15 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, IO_STATUS_BLOCK *io break;
case FileFsFullSizeInformation: - FIXME( "%p: full size info not supported\n", handle ); - status = STATUS_NOT_IMPLEMENTED; + if (length < sizeof(FILE_FS_FULL_SIZE_INFORMATION)) + status = STATUS_BUFFER_TOO_SMALL; + else + { + FILE_FS_FULL_SIZE_INFORMATION *info = buffer; + if ((status = get_full_size_info(fd, info)) == STATUS_SUCCESS) { + io->Information = sizeof(*info); + } + } break;
case FileFsObjectIdInformation:
From: Joel Holdsworth joel@airwebreathe.org.uk
This patch fixes GetDiskFreeSpaceA/W when a NT-style GUID volume path is provided e.g. "\?\Volume{00000000-0000-0000-0000-000000000043}" as might be retrieved by FindFirstVolumeA/W and FindNextVolumeA/W.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53547 Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- dlls/mountmgr.sys/device.c | 50 ++++++++++++++ dlls/mountmgr.sys/unixlib.c | 134 ++++++++++++++++++++++++++++++++++++ dlls/mountmgr.sys/unixlib.h | 16 +++++ 3 files changed, 200 insertions(+)
diff --git a/dlls/mountmgr.sys/device.c b/dlls/mountmgr.sys/device.c index 50b2f38bbd8..328b0b2f344 100644 --- a/dlls/mountmgr.sys/device.c +++ b/dlls/mountmgr.sys/device.c @@ -1656,6 +1656,30 @@ static NTSTATUS WINAPI harddisk_query_volume( DEVICE_OBJECT *device, IRP *irp ) status = STATUS_SUCCESS; break; } + case FileFsSizeInformation: + { + FILE_FS_SIZE_INFORMATION *info = irp->AssociatedIrp.SystemBuffer; + struct size_info size_info = { 0, 0, 0, 0, 0 }; + struct get_volume_size_info_params params = { dev->unix_mount, &size_info }; + + if (length < sizeof(FILE_FS_SIZE_INFORMATION)) + { + status = STATUS_BUFFER_TOO_SMALL; + break; + } + + if ((status = MOUNTMGR_CALL( get_volume_size_info, ¶ms )) == STATUS_SUCCESS) + { + info->TotalAllocationUnits.QuadPart = size_info.total_allocation_units; + info->AvailableAllocationUnits.QuadPart = size_info.caller_available_allocation_units; + info->SectorsPerAllocationUnit = size_info.sectors_per_allocation_unit; + info->BytesPerSector = size_info.bytes_per_sector; + io->Information = sizeof(*info); + status = STATUS_SUCCESS; + } + + break; + } case FileFsAttributeInformation: { FILE_FS_ATTRIBUTE_INFORMATION *info = irp->AssociatedIrp.SystemBuffer; @@ -1702,6 +1726,32 @@ static NTSTATUS WINAPI harddisk_query_volume( DEVICE_OBJECT *device, IRP *irp ) status = STATUS_SUCCESS; break; } + case FileFsFullSizeInformation: + { + FILE_FS_FULL_SIZE_INFORMATION *info = irp->AssociatedIrp.SystemBuffer; + struct size_info size_info = { 0, 0, 0, 0, 0 }; + struct get_volume_size_info_params params = { dev->unix_mount, &size_info }; + + if (length < sizeof(FILE_FS_FULL_SIZE_INFORMATION)) + { + status = STATUS_BUFFER_TOO_SMALL; + break; + } + + if ((status = MOUNTMGR_CALL( get_volume_size_info, ¶ms )) == STATUS_SUCCESS) + { + info->TotalAllocationUnits.QuadPart = size_info.total_allocation_units; + info->CallerAvailableAllocationUnits.QuadPart = size_info.caller_available_allocation_units; + info->ActualAvailableAllocationUnits.QuadPart = size_info.actual_available_allocation_units; + info->SectorsPerAllocationUnit = size_info.sectors_per_allocation_unit; + info->BytesPerSector = size_info.bytes_per_sector; + io->Information = sizeof(*info); + status = STATUS_SUCCESS; + } + + break; + } + default: FIXME("Unsupported volume query %x\n", irpsp->Parameters.QueryVolume.FsInformationClass); status = STATUS_NOT_SUPPORTED; diff --git a/dlls/mountmgr.sys/unixlib.c b/dlls/mountmgr.sys/unixlib.c index 52a3fce66d6..52a4099d1fb 100644 --- a/dlls/mountmgr.sys/unixlib.c +++ b/dlls/mountmgr.sys/unixlib.c @@ -30,12 +30,63 @@ #include <stdio.h> #include <stdlib.h> #include <sys/stat.h> +#ifdef HAVE_SYS_STATFS_H +#include <sys/statfs.h> +#endif +#ifdef HAVE_SYS_STATVFS_H +# include <sys/statvfs.h> +#endif #include <unistd.h>
#include "unixlib.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(mountmgr);
static struct run_loop_params run_loop_params;
+static NTSTATUS errno_to_status( int err ) +{ + TRACE( "errno = %d\n", err ); + switch (err) + { + case EAGAIN: return STATUS_SHARING_VIOLATION; + case EBADF: return STATUS_INVALID_HANDLE; + case EBUSY: return STATUS_DEVICE_BUSY; + case ENOSPC: return STATUS_DISK_FULL; + case EPERM: + case EROFS: + case EACCES: return STATUS_ACCESS_DENIED; + case ENOTDIR: return STATUS_OBJECT_PATH_NOT_FOUND; + case ENOENT: return STATUS_OBJECT_NAME_NOT_FOUND; + case EISDIR: return STATUS_INVALID_DEVICE_REQUEST; + case EMFILE: + case ENFILE: return STATUS_TOO_MANY_OPENED_FILES; + case EINVAL: return STATUS_INVALID_PARAMETER; + case ENOTEMPTY: return STATUS_DIRECTORY_NOT_EMPTY; + case EPIPE: return STATUS_PIPE_DISCONNECTED; + case EIO: return STATUS_DEVICE_NOT_READY; +#ifdef ENOMEDIUM + case ENOMEDIUM: return STATUS_NO_MEDIA_IN_DEVICE; +#endif + case ENXIO: return STATUS_NO_SUCH_DEVICE; + case ENOTTY: + case EOPNOTSUPP:return STATUS_NOT_SUPPORTED; + case ECONNRESET:return STATUS_PIPE_DISCONNECTED; + case EFAULT: return STATUS_ACCESS_VIOLATION; + case ESPIPE: return STATUS_ILLEGAL_FUNCTION; + case ELOOP: return STATUS_REPARSE_POINT_NOT_RESOLVED; +#ifdef ETIME /* Missing on FreeBSD */ + case ETIME: return STATUS_IO_TIMEOUT; +#endif + case ENOEXEC: /* ?? */ + case EEXIST: /* ?? */ + default: + FIXME( "Converting errno %d to STATUS_UNSUCCESSFUL\n", err ); + return STATUS_UNSUCCESSFUL; + } +} + static char *get_dosdevices_path( const char *dev ) { const char *home = getenv( "HOME" ); @@ -266,6 +317,88 @@ static NTSTATUS set_dosdev_symlink( void *args ) return status; }
+static NTSTATUS get_volume_size_info( void *args ) +{ + const struct get_volume_size_info_params *params = args; + const char *unix_mount = params->unix_mount; + struct size_info *info = params->info; + + struct stat st; + ULONGLONG bsize; + NTSTATUS status; + int fd = -1; + +#if !defined(linux) || !defined(HAVE_FSTATFS) + struct statvfs stfs; +#else + struct statfs stfs; +#endif + + if (!unix_mount) + { + return STATUS_NO_SUCH_DEVICE; + } + + if (unix_mount[0] != '/') + { + char *path = get_dosdevices_path( unix_mount ); + if (path) fd = open( path, O_RDONLY ); + free( path ); + } + else fd = open( unix_mount, O_RDONLY ); + + if (fstat( fd, &st ) < 0) + { + status = errno_to_status( errno ); + goto done; + } + if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) + { + status = STATUS_INVALID_DEVICE_REQUEST; + goto done; + } + + /* Linux's fstatvfs is buggy */ +#if !defined(linux) || !defined(HAVE_FSTATFS) + if (fstatvfs( fd, &stfs ) < 0) + { + status = errno_to_status( errno ); + goto done; + } + bsize = stfs.f_frsize; +#else + if (fstatfs( fd, &stfs ) < 0) + { + status = errno_to_status( errno ); + goto done; + } + bsize = stfs.f_bsize; +#endif + if (bsize == 2048) /* assume CD-ROM */ + { + info->bytes_per_sector = 2048; + info->sectors_per_allocation_unit = 1; + } + else + { + info->bytes_per_sector = 512; + info->sectors_per_allocation_unit = 8; + } + + info->total_allocation_units = + bsize * stfs.f_blocks / (info->bytes_per_sector * info->sectors_per_allocation_unit); + info->caller_available_allocation_units = + bsize * stfs.f_bavail / (info->bytes_per_sector * info->sectors_per_allocation_unit); + info->actual_available_allocation_units = + bsize * stfs.f_bfree / (info->bytes_per_sector * info->sectors_per_allocation_unit); + + status = STATUS_SUCCESS; + +done: + close( fd ); + return status; +} + static NTSTATUS get_volume_dos_devices( void *args ) { const struct get_volume_dos_devices_params *params = args; @@ -440,6 +573,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = add_drive, get_dosdev_symlink, set_dosdev_symlink, + get_volume_size_info, get_volume_dos_devices, read_volume_file, match_unixdev, diff --git a/dlls/mountmgr.sys/unixlib.h b/dlls/mountmgr.sys/unixlib.h index e7846a764da..d70371876fa 100644 --- a/dlls/mountmgr.sys/unixlib.h +++ b/dlls/mountmgr.sys/unixlib.h @@ -64,6 +64,21 @@ struct add_drive_params int *letter; };
+struct size_info +{ + LONGLONG total_allocation_units; + LONGLONG caller_available_allocation_units; + LONGLONG actual_available_allocation_units; + ULONG sectors_per_allocation_unit; + ULONG bytes_per_sector; +}; + +struct get_volume_size_info_params +{ + const char *unix_mount; + struct size_info *info; +}; + struct get_dosdev_symlink_params { const char *dev; @@ -142,6 +157,7 @@ enum mountmgr_funcs unix_add_drive, unix_get_dosdev_symlink, unix_set_dosdev_symlink, + unix_get_volume_size_info, unix_get_volume_dos_devices, unix_read_volume_file, unix_match_unixdev,
From: Joel Holdsworth joel@airwebreathe.org.uk
Signed-off-by: Joel Holdsworth joel@airwebreathe.org.uk --- dlls/kernel32/tests/drive.c | 40 +++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/dlls/kernel32/tests/drive.c b/dlls/kernel32/tests/drive.c index e03c44c1947..b9b7562a3df 100644 --- a/dlls/kernel32/tests/drive.c +++ b/dlls/kernel32/tests/drive.c @@ -26,6 +26,11 @@ #include "winerror.h"
static DWORD (WINAPI *pGetDiskFreeSpaceExA)(LPCSTR, PULARGE_INTEGER, PULARGE_INTEGER, PULARGE_INTEGER); +static HANDLE (WINAPI *pFindFirstVolumeA)(LPSTR,DWORD); +static HANDLE (WINAPI *pFindFirstVolumeW)(LPWSTR,DWORD); +static BOOL (WINAPI *pFindNextVolumeA)(HANDLE,LPSTR,DWORD); +static BOOL (WINAPI *pFindNextVolumeW)(HANDLE,LPWSTR,DWORD); +static BOOL (WINAPI *pFindVolumeClose)(HANDLE);
static void test_GetDriveTypeA(void) { @@ -140,6 +145,8 @@ static void test_GetDiskFreeSpaceA(void) DWORD sectors_per_cluster, bytes_per_sector, free_clusters, total_clusters; char drive[] = "?:\"; DWORD logical_drives; + char volume_guid[MAX_PATH + 1] = { 0 }; + HANDLE handle;
ret = GetDiskFreeSpaceA(NULL, §ors_per_cluster, &bytes_per_sector, &free_clusters, &total_clusters); ok(ret, "GetDiskFreeSpaceA error %ld\n", GetLastError()); @@ -204,6 +211,19 @@ static void test_GetDiskFreeSpaceA(void) } logical_drives >>= 1; } + + if (pFindFirstVolumeA) { + handle = pFindFirstVolumeA(volume_guid, MAX_PATH); + ok(handle != INVALID_HANDLE_VALUE, "FindFirstVolumeA error\n"); + do { + ret = GetDiskFreeSpaceA(volume_guid, §ors_per_cluster, &bytes_per_sector, &free_clusters, &total_clusters); + if (!ret) + /* GetDiskFreeSpaceA() should succeed, but it can fail with too many + different GetLastError() results to be usable for an ok() */ + trace("GetDiskFreeSpaceA(%s) failed with %ld\n", volume_guid, GetLastError()); + } while (pFindNextVolumeA(handle, volume_guid, MAX_PATH)); + pFindVolumeClose(handle); + } }
static void test_GetDiskFreeSpaceW(void) @@ -212,6 +232,8 @@ static void test_GetDiskFreeSpaceW(void) DWORD sectors_per_cluster, bytes_per_sector, free_clusters, total_clusters; WCHAR drive[] = {'?',':','\',0}; DWORD logical_drives; + WCHAR volume_guid[MAX_PATH + 1] = { 0 }; + HANDLE handle; static const WCHAR empty_pathW[] = { 0 }; static const WCHAR root_pathW[] = { '\', 0 }; static const WCHAR unix_style_root_pathW[] = { '/', 0 }; @@ -256,12 +278,30 @@ static void test_GetDiskFreeSpaceW(void) } logical_drives >>= 1; } + + if (pFindFirstVolumeW) { + handle = pFindFirstVolumeW(volume_guid, MAX_PATH); + ok(handle != INVALID_HANDLE_VALUE, "FindFirstVolumeW error\n"); + do { + ret = GetDiskFreeSpaceW(volume_guid, §ors_per_cluster, &bytes_per_sector, &free_clusters, &total_clusters); + if (!ret) + /* GetDiskFreeSpaceW() should succeed, but it can fail with too many + different GetLastError() results to be usable for an ok() */ + trace("GetDiskFreeSpaceW(%ls) failed with %ld\n", volume_guid, GetLastError()); + } while (pFindNextVolumeW(handle, volume_guid, MAX_PATH)); + pFindVolumeClose(handle); + } }
START_TEST(drive) { HANDLE hkernel32 = GetModuleHandleA("kernel32"); pGetDiskFreeSpaceExA = (void *) GetProcAddress(hkernel32, "GetDiskFreeSpaceExA"); + pFindFirstVolumeA = (void *) GetProcAddress(hkernel32, "FindFirstVolumeA"); + pFindFirstVolumeW = (void *) GetProcAddress(hkernel32, "FindFirstVolumeW"); + pFindNextVolumeA = (void *) GetProcAddress(hkernel32, "FindNextVolumeA"); + pFindNextVolumeW = (void *) GetProcAddress(hkernel32, "FindNextVolumeW"); + pFindVolumeClose = (void *) GetProcAddress(hkernel32, "FindVolumeClose");
test_GetDriveTypeA(); test_GetDriveTypeW();
On 8/25/22 08:07, Joel Holdsworth wrote:
- if (pFindFirstVolumeA) {
handle = pFindFirstVolumeA(volume_guid, MAX_PATH);ok(handle != INVALID_HANDLE_VALUE, "FindFirstVolumeA error\n");do {ret = GetDiskFreeSpaceA(volume_guid, §ors_per_cluster, &bytes_per_sector, &free_clusters, &total_clusters);if (!ret)/* GetDiskFreeSpaceA() should succeed, but it can fail with too manydifferent GetLastError() results to be usable for an ok() */trace("GetDiskFreeSpaceA(%s) failed with %ld\n", volume_guid, GetLastError());
If it should succeed, don't we want
ok(ret == TRUE, ...);
or under what circumstances can this actually fail?
} while (pFindNextVolumeA(handle, volume_guid, MAX_PATH));pFindVolumeClose(handle);- } }
...
START_TEST(drive) { HANDLE hkernel32 = GetModuleHandleA("kernel32"); pGetDiskFreeSpaceExA = (void *) GetProcAddress(hkernel32, "GetDiskFreeSpaceExA");
pFindFirstVolumeA = (void *) GetProcAddress(hkernel32, "FindFirstVolumeA");
pFindFirstVolumeW = (void *) GetProcAddress(hkernel32, "FindFirstVolumeW");
pFindNextVolumeA = (void *) GetProcAddress(hkernel32, "FindNextVolumeA");
pFindNextVolumeW = (void *) GetProcAddress(hkernel32, "FindNextVolumeW");
pFindVolumeClose = (void *) GetProcAddress(hkernel32, "FindVolumeClose");
test_GetDriveTypeA(); test_GetDriveTypeW();
These are available since at least XP; I think we can link to them directly.
On Fri Aug 26 17:15:03 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 8/25/22 08:07, Joel Holdsworth wrote: > + > + if (pFindFirstVolumeA) { > + handle = pFindFirstVolumeA(volume_guid, MAX_PATH); > + ok(handle != INVALID_HANDLE_VALUE, "FindFirstVolumeA error\n"); > + do { > + ret = GetDiskFreeSpaceA(volume_guid, §ors_per_cluster, &bytes_per_sector, &free_clusters, &total_clusters); > + if (!ret) > + /* GetDiskFreeSpaceA() should succeed, but it can fail with too many > + different GetLastError() results to be usable for an ok() */ > + trace("GetDiskFreeSpaceA(%s) failed with %ld\n", volume_guid, GetLastError()); If it should succeed, don't we want ok(ret == TRUE, ...); or under what circumstances can this actually fail? > + } while (pFindNextVolumeA(handle, volume_guid, MAX_PATH)); > + pFindVolumeClose(handle); > + } > } ... > START_TEST(drive) > { > HANDLE hkernel32 = GetModuleHandleA("kernel32"); > pGetDiskFreeSpaceExA = (void *) GetProcAddress(hkernel32, "GetDiskFreeSpaceExA"); > + pFindFirstVolumeA = (void *) GetProcAddress(hkernel32, "FindFirstVolumeA"); > + pFindFirstVolumeW = (void *) GetProcAddress(hkernel32, "FindFirstVolumeW"); > + pFindNextVolumeA = (void *) GetProcAddress(hkernel32, "FindNextVolumeA"); > + pFindNextVolumeW = (void *) GetProcAddress(hkernel32, "FindNextVolumeW"); > + pFindVolumeClose = (void *) GetProcAddress(hkernel32, "FindVolumeClose"); > > test_GetDriveTypeA(); > test_GetDriveTypeW(); These are available since at least XP; I think we can link to them directly.
The tracing of `GetDiskFreeSpaceA` is copy-pasted from the code further up in the test, where the function is tested with DOS paths.
Stepping back... what do we want this test to look like? `GetDiskFreeSpaceA` can legitimately fail for a variety of reasons, which is why other parts of the unit test stop short of doing `ok(ret == TRUE, ...)`. In similar fashion, this test code simply gets all the drive NT GUID paths (which come from `mountmgr.sys`), and then verifies that `mountmgr.sys` can cope with `FileFsSizeInfo` queries on the paths it listed out.
So it seemed to me, the best I can do is exercise the API, without checking for errors.
Thanks for the feedback on `GetProcAddress`
On 8/26/22 13:59, jhol (@jhol) wrote:
The tracing of `GetDiskFreeSpaceA` is copy-pasted from the code further up in the test, where the function is tested with DOS paths.
Stepping back... what do we want this test to look like? `GetDiskFreeSpaceA` can legitimately fail for a variety of reasons, which is why other parts of the unit test stop short of doing `ok(ret == TRUE, ...)`. In similar fashion, this test code simply gets all the drive NT GUID paths (which come from `mountmgr.sys`), and then verifies that `mountmgr.sys` can cope with `FileFsSizeInfo` queries on the paths it listed out.
Okay, I didn't look at the rest of the test.
It strikes me as more than a little unfortunate that we're saying "can fail" without explaining what those failures are or why they're legitimate. I wouldn't immediately think that GetDiskFreeSpaceA() should ever fail on Windows.
`git blame` shows some of the prior error codes, though unfortunately almost none of the commits are explained and few of the error codes make sense. Based on 3826193f33fb I guess at least some are due to broken virtual filesystem drivers, and I guess some others would be consistent with a network drive that was taken down...
Maybe we could at least test that GetDiskFreeSpaceA() of the C drive works, since I imagine that should never fail. As it is the test seems pretty much useless...
Of course this is all pretty much orthogonal to this patch set.
Maybe we could at least test that GetDiskFreeSpaceA() of the C drive works, since I imagine that should never fail. As it is the test seems pretty much useless...
Ok, so we can have
``` ok(GetDiskFreeSpaceA("C:", ...) == TRUE); ``` and
``` ok(GetDiskFreeSpaceA("\\?\Volume{00000000-0000-0000-0000-000000000043}\", ...) == TRUE); ```
Wine implements a pseudo-volume-GUID scheme whereby the last two digits are the ASCII code of the DOS drive letter in hexadecimal. It uses real GUIDs for other volumes on my machine, though I'm not sure what these devices are.
Should we be hard-coding these pseudo-GUIDs into the test cases?, or do we need to treat it as an opaque token which must first be retrieved from `FindFirstVolume` and friends? I guess we could take the DOS device letter and convert it into a volume GUID somehow? - need to figure out how.
My other question is, can we really always guaruantee that the C-drive is a conventional disk on Windows machines? I've never seen it in the wild, but is it theoretically possible for the whole system to be running with a different drive letter as the main system drive, and C be one of these weird virtual/network drive examples you found? Or is it possible for someone to have a system running on a weird drive?
I guess we could enumerate the drives and find the first one that is not weird, but I'm not sure what APIs are available to make such a determination.
I'd like to have a good test case, I'm just unsure how we can test an API which works great - until it doesn't.
On 8/27/22 04:29, jhol (@jhol) wrote:
Maybe we could at least test that GetDiskFreeSpaceA() of the C drive works, since I imagine that should never fail. As it is the test seems pretty much useless...
Ok, so we can have
ok(GetDiskFreeSpaceA("C:", ...) == TRUE);and
ok(GetDiskFreeSpaceA("\\\\?\\Volume{00000000-0000-0000-0000-000000000043}\\", ...) == TRUE);Wine implements a pseudo-volume-GUID scheme whereby the last two digits are the ASCII code of the DOS drive letter in hexadecimal. It uses real GUIDs for other volumes on my machine, though I'm not sure what these devices are.
Should we be hard-coding these pseudo-GUIDs into the test cases?, or do we need to treat it as an opaque token which must first be retrieved from `FindFirstVolume` and friends? I guess we could take the DOS device letter and convert it into a volume GUID somehow? - need to figure out how.
It can be converted with GetVolumeNameForVolumeMountPointW(). That would probably be better than using a Wine-specific scheme (which may not remain stable anyway.)
My other question is, can we really always guaruantee that the C-drive is a conventional disk on Windows machines? I've never seen it in the wild, but is it theoretically possible for the whole system to be running with a different drive letter as the main system drive, and C be one of these weird virtual/network drive examples you found? Or is it possible for someone to have a system running on a weird drive?
I guess we could enumerate the drives and find the first one that is not weird, but I'm not sure what APIs are available to make such a determination.
I'd like to have a good test case, I'm just unsure how we can test an API which works great - until it doesn't.
I don't know if it's possible to have a misbehaving C drive, but on the other hand, I expect that if programs try to call GetDiskFreeSpaceA("C:") and it fails, they will not handle that well. Which is to say, anyone with a misbehaving C drive is already asking for trouble.
It's a lot more reasonable to expect there to be misbehaving network drives on the machine (especially since it's often the easiest way to get a winetest executable onto a machine.)
So I think (and this is my personal opinion) it's reasonable to assume that querying the C drive always works. Or, at least, it's better to have a test case that is useful and works almost-always, than no test case at all. (Depending in general on what's being tested, of course.)
I don't think any of that should block this patch, since it's a preëxisting problem with the test, but it'd certainly be appreciated if you have the time for it.
-
jhol (＠jhol) -
Joel Holdsworth
-
Zebediah Figura -
Zebediah Figura (she/her)