re: kernel32: Implement GetVolumePathName.
Fails test here? ../../../tools/runtest -q -P wine -M ntdll.dll -T ../../.. -p ntdll_test.exe.so file.c && touch file.ok Unhandled exception: page fault on read access to 0x00212b18 in 32-bit code (0x7bc47995). Backtrace: =>0 HEAP_CreateFreeBlock+0xbb(subheap=0x110014, ptr=0x122b48, size=0xeffd0) [dlls/ntdll/heap.c:583] 1 HEAP_ShrinkBlock+0x42(subheap=0x110014, pArena=0x122b08, size=0x38) [dlls/ntdll/heap.c:685] 2 RtlAllocateHeap+0x2a8(heap=0x110000, flags=0x2, size=0x38) [dlls/ntdll/heap.c:1699] 3 RtlDosPathNameToNtPathName_U+0x35c(dos_path="C:\windows\system32", ntpath=0x32fbb4, file_part=(nil), cd=(nil)) [dlls/ntdll/path.c:394] 4 GetVolumePathNameW+0x107(filename="C:\windows\system32", volumepathname="?Ir\sandbox\slave\(", buflen=0x14) [dlls/kernel32/volume.c:1767] 5 test_file_name_information+0x179() [dlls/ntdll/tests/file.c:1304] 6 func_file+0x374() [dlls/ntdll/tests/file.c:1726] 7 run_test+0x9e(name="file.c") [include/wine/test.h:556] 8 main+0x23a(argc=<?>, argv=0x1103c8) [include/wine/test.h:624] 0x7bc47995 HEAP_CreateFreeBlock+0xbb [dlls/ntdll/heap.c:583] in ntdll: movl 0x0(%eax),%eax 583 (*(DWORD *)((char *)ptr + size) & ARENA_FLAG_FREE))
On Tue, Oct 4, 2011 at 9:46 AM, Dan Kegel <dank(a)kegel.com> wrote:
Fails test here? ...
3 RtlDosPathNameToNtPathName_U+0x35c(dos_path="C:\windows\system32",
ntpath=0x32fbb4, file_part=(nil), cd=(nil)) [dlls/ntdll/path.c:394] 4 GetVolumePathNameW+0x107(filename="C:\windows\system32", volumepathname="?Ir\sandbox\slave\(", buflen=0x14) [dlls/kernel32/volume.c:1767]
...
That test succeeds on my box... That is also a particularly odd place for it to segfault, I really don't see how RtlDosPathNameToNtPathName_U could possibly screw up with that pathname. Erich Hoover ehoover(a)mines.edu
On Tue, Oct 4, 2011 at 8:58 AM, Erich Hoover <compholio(a)gmail.com> wrote:
Fails test here? ...
3 RtlDosPathNameToNtPathName_U+0x35c(dos_path="C:\windows\system32", ntpath=0x32fbb4, file_part=(nil), cd=(nil)) [dlls/ntdll/path.c:394] 4 GetVolumePathNameW+0x107(filename="C:\windows\system32", volumepathname="?Ir\sandbox\slave\(", buflen=0x14) [dlls/kernel32/volume.c:1767]
That test succeeds on my box... That is also a particularly odd place for it to segfault, I really don't see how RtlDosPathNameToNtPathName_U could possibly screw up with that pathname.
Might be an unrelated flakiness. I'll run it in a loop here and see if I can get it to happen again. - Dan
On Oct 4, 2011, at 9:32 AM, Erich Hoover wrote:
This patch implements GetVolumePathName by using the full folder path and working backward until stat() returns a different device. Surely there's a better way. Can't you do a statfs(2)/statvfs(2) (for example) to find out the FS root? (I know that works on the BSDs and Mac OS, because their statfs/statvfs structures all have a field for that--f_mntonname.) Then you can map that path back to a Wine path, and return that. Of course, that won't work for fake drives (see below).
At the very least, you should be caching this information, like I did when I added case-insensitive lookup support to Wine (see commit 4e44e153c5c78339f17baeabe22f2015cfc8737c). Because you call stat(2) on every single directory in a pathname (especially in the common case of the file being on the root device), calculating this from scratch is extremely expensive. And what about drive C? On Wine, that's not a device at all. It's a folder usually somewhere on the root device. In general, I don't see anything in your patch that accounts for fake drives like C:--the common case, I might add! Chip
On Tue, Oct 4, 2011 at 11:36 AM, Charles Davis <cdavis(a)mymail.mines.edu> wrote:
On Oct 4, 2011, at 9:32 AM, Erich Hoover wrote:
This patch implements GetVolumePathName by using the full folder path and working backward until stat() returns a different device. Surely there's a better way. Can't you do a statfs(2)/statvfs(2) (for example) to find out the FS root? (I know that works on the BSDs and Mac OS, because their statfs/statvfs structures all have a field for that--f_mntonname.) Then you can map that path back to a Wine path, and return that. Of course, that won't work for fake drives (see below).
I doubt it, the function needs to return the mount point along the given path (see MSDN). In the case of cross-device symbolic links your suggestion will not return the current path.
At the very least, you should be caching this information, like I did when I added case-insensitive lookup support to Wine (see commit 4e44e153c5c78339f17baeabe22f2015cfc8737c). Because you call stat(2) on every single directory in a pathname (especially in the common case of the file being on the root device), calculating this from scratch is extremely expensive.
I'm not sure this function is used enough to justify caching the results, as this is the first time I've noticed the FIXME on the console. If someone thinks that is the case then I can always make that a "part 2" patch.
And what about drive C? On Wine, that's not a device at all. It's a folder usually somewhere on the root device. In general, I don't see anything in your patch that accounts for fake drives like C:--the common case, I might add!
If you pass it just the path to "C:\" then it will spit back "C:\", since it only encounters the one filesystem. If you pass something like "C:\windows\system32\" then it will spit back "C:\" unless the "windows" or "system32" folders are symbolic links to a folder on another drive. Erich Hoover ehoover(a)mines.edu
On Oct 4, 2011, at 11:56 AM, Erich Hoover wrote:
On Tue, Oct 4, 2011 at 11:36 AM, Charles Davis <cdavis(a)mymail.mines.edu> wrote:
On Oct 4, 2011, at 9:32 AM, Erich Hoover wrote:
This patch implements GetVolumePathName by using the full folder path and working backward until stat() returns a different device. Surely there's a better way. Can't you do a statfs(2)/statvfs(2) (for example) to find out the FS root? (I know that works on the BSDs and Mac OS, because their statfs/statvfs structures all have a field for that--f_mntonname.) Then you can map that path back to a Wine path, and return that. Of course, that won't work for fake drives (see below).
I doubt it, the function needs to return the mount point along the given path (see MSDN). In the case of cross-device symbolic links your suggestion will not return the current path. Ah, you're right. MSDN says that GetVolumePathName() returns the volume containing the target of the symlink... unless of course, the path is a remote one. (Same for mounted volumes.) But that isn't a common case, nor is it one that Wine even supports yet :).
At the very least, you should be caching this information, like I did when I added case-insensitive lookup support to Wine (see commit 4e44e153c5c78339f17baeabe22f2015cfc8737c). Because you call stat(2) on every single directory in a pathname (especially in the common case of the file being on the root device), calculating this from scratch is extremely expensive.
I'm not sure this function is used enough to justify caching the results, as this is the first time I've noticed the FIXME on the console. If someone thinks that is the case then I can always make that a "part 2" patch. OK.
And what about drive C? On Wine, that's not a device at all. It's a folder usually somewhere on the root device. In general, I don't see anything in your patch that accounts for fake drives like C:--the common case, I might add!
If you pass it just the path to "C:\" then it will spit back "C:\", since it only encounters the one filesystem. If you pass something like "C:\windows\system32\" then it will spit back "C:\" unless the "windows" or "system32" folders are symbolic links to a folder on another drive. Ah, I read your patch wrong. Never mind.
Chip
participants (4)
-
Charles Davis -
Dan Kegel -
Erich Hoover -
Erich Hoover