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).
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.
- 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.
+{
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.
- 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).
- 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.
- 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 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.
- 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);