Windows: Move temp files to Temp directory. #184

dawid-sabat · 2022-12-09T13:43:04Z

For Windows compilations of OpenDCDiag, temp files were created in directory where the .exe is placed. The application doesn't always have access rights to do that.

Changes here move all temp files to Windows designated area: C:\Users%username\AppData\Local\Temp

Signed-off-by: dawid-sabat dawid.sabat@intel.com

framework/sysdeps/windows/tmpfile.c

dawid-sabat · 2022-12-13T13:22:22Z

I noticed that this change breaks selftests in a way I don't completely understand. I suspect there might something wrong with wine configuration but I'm not sure.

not ok 1 Verify that the python yaml module can import regular output
# (from function `run_sandstone_yaml' in file bats/sanity-check/../testenv.bash, line 87,
#  in test file bats/sanity-check/[10](https://github.com/opendcdiag/opendcdiag/actions/runs/3657877805/jobs/6181966319#step:10:11)-yaml-validate.bats, line 9)
#   `run_sandstone_yaml --retest-on-failure=0 --selftest --timeout=60s --quick -e @positive' failed
# + wine builddir-windows-cross/opendcdiag.exe --on-crash=core --on-hang=kill -Y -o - --retest-on-failure=0 --selftest --timeout=60s --quick -e @positive
# internal error creating temporary file: File too large

Any ideas?

thiagomacieira

Hadn't seen this before I wrote #186

thiagomacieira · 2022-12-14T17:54:25Z

framework/sysdeps/windows/tmpfile.c

    for (int i = 0; hFile == INVALID_HANDLE_VALUE && i < 16; ++i) {
        wcscat(_ultow(next_random(), tmpname, 36), L".tmp");            // yes, base 36
        DWORD access = GENERIC_READ | GENERIC_WRITE;
        DWORD sharemode = FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE;
        DWORD creation = CREATE_NEW;
        DWORD flags = FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE;
-        hFile = CreateFileW(tmpname, access, sharemode, &sa, creation, flags, NULL);
+
+        if ((wcslen(tmppath) + wcslen(tmpname)) >= MAX_PATH)


That doesn't happen for all file names. The shortest file name we'll generate is "0.tmp" and the longest is "1z141z4.tmp". So there's a possibility that some paths will work.

Either way, either we handle GetTempPathW returning too long a string before the loop or we simply continue here. Or both.

Should be done now

dawid-sabat · 2022-12-15T14:22:58Z

@lukasz-tuz and I made some experiments about opendcdiag selftests failing:

tried to recreate the same conditions as CI (the compiller, libs etc) locally
from my end I noticed that selftests and opendcdiag.exe works with wine outside the docker container.
Lukasz has been debugging on his end and he's going to describe his effort.

thiagomacieira · 2022-12-15T19:02:41Z

@lukasz-tuz and I made some experiments about opendcdiag selftests failing:

tried to recreate the same conditions as CI (the compiller, libs etc) locally

from my end I noticed that selftests and opendcdiag.exe works with wine outside the docker container.

Lukasz has been debugging on his end and he's going to describe his effort.

I recreated the conditions on Wine by simply doing a cd / before launching it. For the BATS testing, we need to check that the output log file (if not stdout) has a full path). We should do that for Unix too.

lukasz-tuz · 2022-12-16T13:27:48Z

Lack of full path to output log file is only part of this (albeit needs to be fixed). However:

default Wine prefix doesn't seem to have %TEMP% or %TMP% variables; they need to be added manually, and need to be added to the registry file in the Wine prefix being used. Passing them as shell environment variables is not sufficient (see Wine User Guide).
Need to ensure that directory pointed to by %TEMP% has write permissions set
even with the steps above completed, opendcdiag.exe would still fail when trying to truncate the temp file with ftruncate. As @dawid-sabat observed, this does not happen with more up-to-date version of Wine (I used 7.22 in my tests). Older version (7.2) fails with EFBIG error, although length passed to ftruncate is just 65 kB.

Going to run a targeted test, where previously failing environment gets a Wine update (and just Wine, to rule out any other dependencies).

thiagomacieira · 2022-12-16T13:56:06Z

I don't know where the environment came from in my ~/.wine/user.reg, but it does show:

[Environment] 1632151948
#time=1d7ae34b87983e8
"PATH"="C:\\windows\\system32;C:\\windows;C:\\windows\\system32\\wbem;C:\\windows\\system32\\WindowsPowershell\\v1.0;C:\\MinGW\\bin;C:\\Qt\\qtbase\\bin"
"QT_FORCE_STDERR_LOGGING"="1"
"TEMP"="C:\\users\\tjmaciei\\Temp"
"TMP"="C:\\users\\tjmaciei\\Temp"

I don't remember setting them and I probably wouldn't have set them to the path that is shown there (I'd choose "Z:\tmp"), but the other two variables are definitely me.

Another thing is that tracing its execution shows just how Wine executes GetTempPathW:

1515859.354:00dc:00e0:Call KERNEL32.GetTempPathW(000000f9,0081e6a0) ret=140020632
1515859.354:00dc:00e0:trace:process:GetEnvironmentVariableW (L"TMP" 000000000081E350 260)
1515859.354:00dc:00e0:Call ntdll.RtlInitUnicodeString(0081e2d0,7b08bd14 L"TMP") ret=7b05c2d7
1515859.354:00dc:00e0:Ret  ntdll.RtlInitUnicodeString() retval=00000008 ret=7b05c2d7
1515859.354:00dc:00e0:Call ntdll.RtlQueryEnvironmentVariable_U(00000000,0081e2d0,0081e2e0) ret=7b05c305
1515859.354:00dc:00e0:trace:environ:RtlQueryEnvironmentVariable_U 0000000000000000 L"TMP" 000000000081E2E0
1515859.354:00dc:00e0:Ret  ntdll.RtlQueryEnvironmentVariable_U() retval=00000000 ret=7b05c305
1515859.354:00dc:00e0:Call ntdll.RtlGetFullPathName_U(0081e350 L"C:\\users\\tjmaciei\\Temp",00000208,0081e350,00000000) ret=7b01d1e4
1515859.354:00dc:00e0:trace:file:RtlGetFullPathName_U (L"C:\\users\\tjmaciei\\Temp" 520 000000000081E350 0000000000000000)
1515859.354:00dc:00e0:Ret  ntdll.RtlGetFullPathName_U() retval=0000002c ret=7b01d1e4
1515859.354:00dc:00e0:Call ntdll.memset(0081e6d0,00000000,000001c2) ret=7b01d390
1515859.354:00dc:00e0:Ret  ntdll.memset() retval=0081e6d0 ret=7b01d390
1515859.354:00dc:00e0:trace:file:GetTempPathW returning 23, L"C:\\users\\tjmaciei\\Temp\\"

That is, the darned thing does a GetEnvironmentVariableW! I don't know what real Windows does, but we can assume it's maybe smarter than this.

thiagomacieira · 2022-12-16T14:11:29Z

I've just tested this patch with Wine 7.22 on Arch Linux, with and without WINE_ROOT set. The selftests launch and (mostly) pass. I get a BATS failure on selftest_fastfail but that seems unrelated (it's expecting code 0xC0000005 but getting 0xc0000409).

lukasz-tuz · 2022-12-16T14:45:10Z

the darned thing does a GetEnvironmentVariableW! I don't know what real Windows does, but we can assume it's maybe smarter than this.

Well, it probably isn't. Quoting the documentation for GetTempPathW:

The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:

The path specified by the TMP environment variable.

The path specified by the TEMP environment variable.

The path specified by the USERPROFILE environment variable.

The Windows directory.

Note that the function does not verify that the path exists, nor does it test to see if the current process has any kind of access rights to the path.

The last one probably just looks for %WINDIR%.

get a BATS failure on selftest_fastfail but that seems unrelated (it's expecting code 0xC0000005 but getting 0xc0000409)

Same result here.

lukasz-tuz · 2022-12-20T14:13:19Z

Ah, the problem was right there, staring me in the face:

// MinGW's ftruncate64 tries to check free disk space and that fails on Wine,
// so use the the 32-bit offset version (which calls _chsize)

And the following typo:

#    undef fruncate

After changing to

#    undef ftruncate

opendcdiag stops failing when trying to truncate the temp file with ftruncate.

thiagomacieira

Approving to unblock, but my comment on tmpfile.c is unaddressed.

For Windows compilations of OpenDCDiag, temp files were created in directory where the .exe is placed. The application doesn't always have access rights to do that. Changes here move all temp files to Windows designated area: C:\Users\%username\AppData\Local\Temp Signed-off-by: dawid-sabat <dawid.sabat@intel.com>

Signed-off-by: Lukasz Tuz <lukasz.tuz@intel.com>

busykai

LGTM.

The concern was addressed.

pwlazlyn reviewed Dec 9, 2022

View reviewed changes

framework/sysdeps/windows/tmpfile.c Outdated Show resolved Hide resolved

lukasz-tuz previously requested changes Dec 13, 2022

View reviewed changes

framework/sysdeps/windows/tmpfile.c Outdated Show resolved Hide resolved

dawid-sabat force-pushed the temp-files-on-windows branch from e8f7c01 to 2b396ed Compare December 13, 2022 13:19

thiagomacieira reviewed Dec 13, 2022

View reviewed changes

dawid-sabat mentioned this pull request Dec 14, 2022

FW/Win32: find a directory for our temporary files other than "." #186

Closed

thiagomacieira requested changes Dec 14, 2022

View reviewed changes

lukasz-tuz force-pushed the temp-files-on-windows branch 3 times, most recently from a5f7942 to 0eca7aa Compare December 20, 2022 15:21

lukasz-tuz requested review from lukasz-tuz and thiagomacieira December 20, 2022 17:00

thiagomacieira previously approved these changes Jan 7, 2023

View reviewed changes

dawid-sabat dismissed thiagomacieira’s stale review via 2b396ed January 9, 2023 10:55

dawid-sabat force-pushed the temp-files-on-windows branch 2 times, most recently from 2b396ed to a830f07 Compare January 9, 2023 10:58

dawid-sabat and others added 2 commits January 9, 2023 12:04

Fixing typo that broke self tests on wine

abd15dc

Signed-off-by: Lukasz Tuz <lukasz.tuz@intel.com>

dawid-sabat force-pushed the temp-files-on-windows branch from a830f07 to abd15dc Compare January 9, 2023 11:05

busykai approved these changes Jan 9, 2023

View reviewed changes

busykai merged commit fc9cd16 into opendcdiag:main Jan 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows: Move temp files to Temp directory. #184

Windows: Move temp files to Temp directory. #184

dawid-sabat commented Dec 9, 2022

dawid-sabat commented Dec 13, 2022

thiagomacieira left a comment

thiagomacieira Dec 14, 2022

dawid-sabat Jan 9, 2023

dawid-sabat commented Dec 15, 2022

thiagomacieira commented Dec 15, 2022

lukasz-tuz commented Dec 16, 2022

thiagomacieira commented Dec 16, 2022

thiagomacieira commented Dec 16, 2022

lukasz-tuz commented Dec 16, 2022

lukasz-tuz commented Dec 20, 2022

thiagomacieira left a comment

busykai left a comment

Windows: Move temp files to Temp directory. #184

Windows: Move temp files to Temp directory. #184

Conversation

dawid-sabat commented Dec 9, 2022

dawid-sabat commented Dec 13, 2022

thiagomacieira left a comment

Choose a reason for hiding this comment

thiagomacieira Dec 14, 2022

Choose a reason for hiding this comment

dawid-sabat Jan 9, 2023

Choose a reason for hiding this comment

dawid-sabat commented Dec 15, 2022

thiagomacieira commented Dec 15, 2022

lukasz-tuz commented Dec 16, 2022

thiagomacieira commented Dec 16, 2022

thiagomacieira commented Dec 16, 2022

lukasz-tuz commented Dec 16, 2022

lukasz-tuz commented Dec 20, 2022

thiagomacieira left a comment

Choose a reason for hiding this comment

busykai left a comment

Choose a reason for hiding this comment