[PATCH] add: SetEnvironmentStringsW
Zebediah Figura
z.figura12 at gmail.com
Sun Feb 9 21:37:48 CST 2020
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()."
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 at users.noreply.github.com>
>
> Signed-off-by: Frank Uhlig <uhlig.frank at 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).
I suspect that you also want either local variables or (when applicable)
"static const". There's no point declaring non-constant variables as static.
> +
> + 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.
> +
> + 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.
> +
> + 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.
> +{
> +
Stray newline.
> + 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.
> +
> + 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().]
> +
> + 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?
* Are values not present in the call to SetEnvironmentStrings() deleted
from the environment?
* 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".)
* 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)?
* What about strings that violate the assumptions this implementation
makes? For example, "one=", "one", "=two", "one=two=three", "one=two
three=four"...
* Is this function really supposed to modify its argument? (Yes, it's
not declared constant, but that doesn't always mean that it will.)
> +
> +
> /***********************************************************************
> * 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);
>
More information about the wine-devel
mailing list