[PATCH] add: SetEnvironmentStringsW
Frank Uhlig
uhlig.frank at gmail.com
Wed Mar 11 16:50:42 CDT 2020
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 at 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 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.
>
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?
> >
> >>
> >>> +
> >>> + 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.
> >
> >> * 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.
> >
> >>
> >> * 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);
> >>>
> >>
> >
>
>
More information about the wine-devel
mailing list