On 3/11/20 4:50 PM, Frank Uhlig wrote:
Sorry for the late reply, but have finally gotten back to this patch.
On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura z.figura12@gmail.com wrote:
On 2/20/20 3:12 PM, Frank Uhlig wrote:
Hi Zebediah, thanks for looking into this. Please find my responses inline below. I've already incorporated many things, but still have some remaining questions and comments.
I would re-submit it soon, and include a "versioning" in the mail tag as I've seen on other submissions (i.e., [PATCH v2]).
On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura z.figura12@gmail.com wrote:
Hello Frank, thanks for the patch.
With regard to the title, convention is to put the name of the component first, followed by a colon, then a verb describing what the patch does, so e.g. "kernelbase: Add SetEnvironmentStringsW()."
done.
I have a few more comments inlined below which might help improve the patch:
On 1/22/20 3:33 PM, Frank Uhlig wrote:
From: Frank Uhlig fuulish@users.noreply.github.com
Signed-off-by: Frank Uhlig uhlig.frank@gmail.com
...ms-win-core-processenvironment-l1-1-0.spec | 2 +- ...ms-win-core-processenvironment-l1-2-0.spec | 2 +- dlls/kernel32/kernel32.spec | 2 +- dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++ dlls/kernelbase/kernelbase.spec | 2 +- dlls/kernelbase/process.c | 29 +++++++++ include/winbase.h | 1 + 7 files changed, 95 insertions(+), 4 deletions(-)
diff --git a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec index e3698d6efd..7a62b74390 100644 --- a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec +++ b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec @@ -15,7 +15,7 @@ @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW -@ stub SetEnvironmentStringsW +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW @ stdcall SetStdHandle(long long) kernel32.SetStdHandle diff --git a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec index 2c25ee1a07..c93d221c5e 100644 --- a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec +++ b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec @@ -17,7 +17,7 @@ @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW -@ stub SetEnvironmentStringsW +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW @ stdcall SetStdHandle(long long) kernel32.SetStdHandle diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec index be48ef1694..2b74a4182e 100644 --- a/dlls/kernel32/kernel32.spec +++ b/dlls/kernel32/kernel32.spec @@ -1387,7 +1387,7 @@ # @ stub SetDynamicTimeZoneInformation @ stdcall -import SetEndOfFile(long) # @ stub SetEnvironmentStringsA -# @ stub SetEnvironmentStringsW +@ stdcall -import SetEnvironmentStringsW (ptr) @ stdcall -import SetEnvironmentVariableA(str str) @ stdcall -import SetEnvironmentVariableW(wstr wstr) @ stdcall -import SetErrorMode(long) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 44a6a0cff0..53b2803e77 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void) FreeEnvironmentStringsW(env2); }
+static void test_SetEnvironmentStringsW(void) +{
- DWORD buf_len;
- BOOL ret;
- DWORD ret_size;
- static WCHAR buf[256];
- static WCHAR name[] = {'N','a','m','e',0};
- static WCHAR value[] = {'V','a','l','u','e',0};
- static WCHAR env[] = {'N','a','m','e','=','V','a','l','u','e',0};
- static WCHAR eman[] = {'e','m','a','N',0};
- static WCHAR eulav[] = {'e','u','l','a','V',0};
- static WCHAR vne[] = {'e','m','a','N','=','e','u','l','a','V',0};
- static WCHAR var[] = {'V','a','r',0};
- static WCHAR val[] = {'V','a','l',0};
- static WCHAR rav[] = {'r','a','V',0};
- static WCHAR lav[] = {'l','a','V',0};
- static WCHAR mul[] = {'V','a','r','=','V','a','l',' ','r','a','V','=','l','a','V',0};
- static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also allow you to get rid of some of these declarations).
Didn't change this, b/c I was trying to keep the same style as already there and described on the website (https://wiki.winehq.org/Developer_Hints#Using_only_C89-compliant_code).
The wiki is out of date on this particular topic. Someone™ should probably update it.
I suspect that you also want either local variables or (when applicable) "static const". There's no point declaring non-constant variables as static.
Agreed, but kept as is to be consistent; and my test code is now not changing the input anymore.
To be consistent with what? There may be existing code that uses bad style, but that's not really a good reason to emulate it.
consistent style is less prone to errors. but further, I prefer the array initialization b/c of the visible double-0 termination of the strings. and yes, the static was a remnant of a previous static const, and got removed.
- buf_len = sizeof(buf) / 2;
I personally think using an extra variable is redundant here. I also think "ARRAY_SIZE(buf)" would be more expressive.
Changed to ARRAY_SIZE(buf)
- ret = SetEnvironmentStringsW(env);
- ok( ret, "Setting environment strings failed\n" );
- ret_size = GetEnvironmentVariableW(name, buf, buf_len);
- ok( ((0 != ret_size) && (0 == wcscmp(buf, value))),
"Environment String settings resulted in different value\n");With all of these tests I think it would be helpful to print the actual value of "ret_size" and contents of "buf" in the case the test fails. I would also recommend testing "ret_size" against the exact expected length, and perhaps splitting the two tests into two separate ok() calls.
Made the checks more specific, and included more info in the output
- ret = SetEnvironmentStringsW(vne);
- ok( ret, "Setting environment strings failed\n" );
- ret_size = GetEnvironmentVariableW(eman, buf, buf_len);
- ok( ((0 != ret_size) && (0 == wcscmp(buf, eulav))),
"Environment String settings resulted in different value\n");- ret = SetEnvironmentStringsW(mul);
- ok( ret, "Setting environment strings failed\n" );
- ret_size = GetEnvironmentVariableW(var, buf, buf_len);
- ok( ((0 != ret_size) && (0 == wcscmp(buf, val))),
"Environment String settings resulted in different value\n");- ret_size = GetEnvironmentVariableW(rav, buf, buf_len);
- ok( ((0 != ret_size) && (0 == wcscmp(buf, lav))),
"Environment String settings resulted in different value\n");- ret = SetEnvironmentStringsW(empty);
- ok( ret, "Setting environment strings failed\n" );
- ret_size = GetEnvironmentVariableW(var, buf, buf_len);
- ok( (0 == ret_size),
"Environment String settings resulted in different value\n");+}
- START_TEST(environ) { init_functionpointers();
@@ -591,4 +651,5 @@ START_TEST(environ) test_GetComputerNameExA(); test_GetComputerNameExW(); test_GetEnvironmentStringsW();
- test_SetEnvironmentStringsW(); }
diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec index f36d4d525c..a5e30b938f 100644 --- a/dlls/kernelbase/kernelbase.spec +++ b/dlls/kernelbase/kernelbase.spec @@ -1422,7 +1422,7 @@ @ stdcall SetDefaultDllDirectories(long) # @ stub SetDynamicTimeZoneInformation @ stdcall SetEndOfFile(long) -@ stub SetEnvironmentStringsW +@ stdcall SetEnvironmentStringsW(ptr) @ stdcall SetEnvironmentVariableA(str str) @ stdcall SetEnvironmentVariableW(wstr wstr) @ stdcall SetErrorMode(long) diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index a07dddb1fc..97b59b9548 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1345,6 +1345,35 @@ BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentVariableW( LPCWSTR name, LPCWSTR val }
+/***********************************************************************
SetEnvironmentStringsW (kernelbase.@)- */
+BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentStringsW( LPWCH NewEnvironment )
"WCHAR *" would probably be preferred.
Any particular reason for this? That's exactly what LPWCH is defined as.
Broadly, use of pointer typedefs tends to be dispreferred in new code. I also suspect that "env" would be nicer than "NewEnvironment".
OK, fine by me.
+{
Stray newline.
Done.
- BOOL rc = FALSE;
- WCHAR *var, *val;
- const WCHAR delim[] = {' ','=','\n',0};
- TRACE( "(%s)\n", debugstr_w(NewEnvironment));
- if (NULL == NewEnvironment)
return rc;This probably deserves a test.
I tried including a simple test by just calling SetEnvironmentStrings(NULL) and that lead to an unhandled exception on my windows machine (cross-compiled from 64 bit Linux to 64 bit Windows using mingw). No clue how and why, but would welcome any suggestions on why this simple thing might fail.
It's possible that Windows doesn't check the pointer against NULL either, and in that case there's not much point in doing it in Wine.
You're right, but this is such an elementary check that I would not remove it.
- var = wcstok(NewEnvironment, delim);
- val = wcstok(NULL, delim);
This isn't thread-safe. [It's not stated anywhere that SetEnvironmentStrings() itself is thread-safe, but it costs little to assume it is, and moreover this will race with anything else that calls wcstok().]
True, however I had trouble using the reentrant wcstok_s on my system (i.e., 'twas unavailable :). Is there an alternative, specific include? (I tride the expected #include <wchar.h> but that didn't work).
It should probably be added to the headers. I think it goes in string.h and wchar.h, but Jacek might correct me on this.
Switched to wcschr which is available and working. Was trying to include wcstok_s from msvcrt, but without success. What would be the proper way to go about this?
I think that Jacek added it in the interim ;-)
That said, you probably shouldn't use it, if tests show that SetEnvironmentStrings() doesn't modify the argument. (If it does, I guess that gives you free license.)
- while (var != NULL) {
if (FALSE == (rc = SetEnvironmentVariableW(var, val)))break;var = wcstok(NULL, delim);val = wcstok(NULL, delim);- }
- return rc;
+}
Broadly, this seems surprising; is this really how the function is supposed to work?
I think your patch could use more interesting tests. For example:
- Immediately I'd expect GetEnvironmentStrings() to return the exact
same pointer that SetEnvironmentStrings() set. Is this in fact the case? If so, how do A and W variants (of both Get/Set) interact?
Confusing. GetEnvironmentStringsW doesn't return a pointer. Haven't seen any info on the GetEnvironmentStringsA variant.
Er, what? GetEnvironmentStringsA() and GetEnvironmentStringsW() return pointers to char and WCHAR respectively.
Sorry, misread. However, GetEnvironmentStringsW returns a pointer to a new copy of the environment strings with each call. Probably from the same source that gets modified by SetEnvironmentStringsW. Not really checkable from the outside, I gather.
Okay, so part of that comment was based on the mistaken impression that GetEnvironmentStrings() returned a pointer to a constant string instead of allocating a new set every time. Apologies for the confusion that may have caused.
- Are values not present in the call to SetEnvironmentStrings() deleted
from the environment?
doc doesn't specify anything about this. I'd expect no.
The documentation is often less specific than desired, and also often incorrect; that's why we write tests, and code our implementations against them. It's not obvious whether SetEnvironmentStrings() means "add these several strings to the environment" or "replace the entire environment with these strings", hence the question.
Yes, on a Windows machine a fresh environment is set up and all the previous variables are discarded. In the wine patch now as well.
One thing you'll need to take into consideration, then, is that there are several environment variables Wine needs to function correctly. In practice I think this means pretty much anything prefixed with WINE, though you may want to maintain a manual list.
Are there any others I'm not thinking of?
(Host libraries would presumably need them—and if I'm using e.g. GST_DEBUG I'd prefer not to have to jump through hoops to make that work, although if SetEnvironmentStrings() is uncommon I can't complain too much, and really all I'd need to do is hack RtlSetCurrentEnvironment() to set that variable afterward. On the one hand, host libraries would use getenv() anyway, but on the other hand, if we move to PE files for some of them, this becomes a potential problem again. All in all, I'm not sure this is a salient enough concern to warrant doing anything about it...)
- The PSDK header suggests that SetEnvironmentStrings() should receive a
doubly null-terminated argument. I'd advise testing with an environment containing a null separator (e.g. L"one=two\0three=four\0".)
Done.
- You treat '\n' as a separator, but have no tests for it. Also, if both
space and newline are a separator, what about other whitespace characters (tab, carriage return)?
I have reduced the number of possible separators to just include '=' and conform with the doubly-null terminated string expectation. This also conforms with the few examples I've found.
- What about strings that violate the assumptions this implementation
makes? For example, "one=", "one", "=two", "one=two=three", "one=two three=four"...
Included a few more tests.
Just to note, "one=two=three" is valid and will assign 'two=three' to the variable 'one' (on windows). one= is also not illegal, AFAICT.
- Is this function really supposed to modify its argument? (Yes, it's
not declared constant, but that doesn't always mean that it will.)
Done, included a buffer that's modified instead.
- /***********************************************************************
***********************************************************************/
- Process/thread attribute lists
diff --git a/include/winbase.h b/include/winbase.h index 655eb48f0f..0c1aa19b42 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -2621,6 +2621,7 @@ WINBASEAPI BOOL WINAPI SetEndOfFile(HANDLE); WINBASEAPI BOOL WINAPI SetEnvironmentVariableA(LPCSTR,LPCSTR); WINBASEAPI BOOL WINAPI SetEnvironmentVariableW(LPCWSTR,LPCWSTR); #define SetEnvironmentVariable WINELIB_NAME_AW(SetEnvironmentVariable) +WINBASEAPI BOOL WINAPI SetEnvironmentStringsW(LPWCH); WINBASEAPI UINT WINAPI SetErrorMode(UINT); WINBASEAPI BOOL WINAPI SetEvent(HANDLE); WINBASEAPI VOID WINAPI SetFileApisToANSI(void);