[PATCH] add: SetEnvironmentStringsW
Zebediah Figura
z.figura12 at gmail.com
Thu Feb 20 15:29:09 CST 2020
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 at 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 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).
>>
>
> 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.
>
>>> +
>>> + 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".
>>> +{
>>> +
>>
>> 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.
>
>>> +
>>> + 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.
>
>>
>>> +
>>> + 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.
>
>> * 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.
>
>>
>> * 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);
>>>
>>
>
More information about the wine-devel
mailing list