[PATCH 1/2] ntdll/tests: Refactor RtlSetEnvironmentVariable() tests.
Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/ntdll/tests/env.c | 169 +++++++++++++++++++---------------------- 1 file changed, 80 insertions(+), 89 deletions(-) diff --git a/dlls/ntdll/tests/env.c b/dlls/ntdll/tests/env.c index 63a9a0e463c..eed8ae38c14 100644 --- a/dlls/ntdll/tests/env.c +++ b/dlls/ntdll/tests/env.c @@ -24,12 +24,8 @@ static NTSTATUS (WINAPI *pRtlMultiByteToUnicodeN)( LPWSTR dst, DWORD dstlen, LPDWORD reslen, LPCSTR src, DWORD srclen ); -static NTSTATUS (WINAPI *pRtlCreateEnvironment)(BOOLEAN, PWSTR*); -static NTSTATUS (WINAPI *pRtlDestroyEnvironment)(PWSTR); static NTSTATUS (WINAPI *pRtlQueryEnvironmentVariable_U)(PWSTR, PUNICODE_STRING, PUNICODE_STRING); static NTSTATUS (WINAPI* pRtlQueryEnvironmentVariable)(WCHAR*, WCHAR*, SIZE_T, WCHAR*, SIZE_T, SIZE_T*); -static void (WINAPI *pRtlSetCurrentEnvironment)(PWSTR, PWSTR*); -static NTSTATUS (WINAPI *pRtlSetEnvironmentVariable)(PWSTR*, PUNICODE_STRING, PUNICODE_STRING); static NTSTATUS (WINAPI *pRtlExpandEnvironmentStrings_U)(LPWSTR, PUNICODE_STRING, PUNICODE_STRING, PULONG); static NTSTATUS (WINAPI *pRtlCreateProcessParameters)(RTL_USER_PROCESS_PARAMETERS**, const UNICODE_STRING*, const UNICODE_STRING*, @@ -169,84 +165,6 @@ static void testQuery(void) else win_skip("RtlQueryEnvironmentVariable not available, skipping tests\n"); } -static void testSetHelper(LPWSTR* env, const char* var, const char* val, NTSTATUS ret, NTSTATUS alt) -{ - WCHAR bvar[256], bval1[256], bval2[256]; - UNICODE_STRING uvar; - UNICODE_STRING uval; - NTSTATUS nts; - - uvar.Length = strlen(var) * sizeof(WCHAR); - uvar.MaximumLength = uvar.Length + sizeof(WCHAR); - uvar.Buffer = bvar; - pRtlMultiByteToUnicodeN( bvar, sizeof(bvar), NULL, var, strlen(var)+1 ); - if (val) - { - uval.Length = strlen(val) * sizeof(WCHAR); - uval.MaximumLength = uval.Length + sizeof(WCHAR); - uval.Buffer = bval1; - pRtlMultiByteToUnicodeN( bval1, sizeof(bval1), NULL, val, strlen(val)+1 ); - } - nts = pRtlSetEnvironmentVariable(env, &uvar, val ? &uval : NULL); - ok(nts == ret || (alt && nts == alt), "Setting var %s=%s (%x/%x)\n", var, val, nts, ret); - if (nts == STATUS_SUCCESS) - { - uval.Length = 0; - uval.MaximumLength = sizeof(bval2); - uval.Buffer = bval2; - nts = pRtlQueryEnvironmentVariable_U(*env, &uvar, &uval); - switch (nts) - { - case STATUS_SUCCESS: - ok(lstrcmpW(bval1, bval2) == 0, "Cannot get value written to environment\n"); - break; - case STATUS_VARIABLE_NOT_FOUND: - ok(val == NULL || - broken(strchr(var,'=') != NULL), /* variable containing '=' may be set but not found again on NT4 */ - "Couldn't find variable, but didn't delete it. val = %s\n", val); - break; - default: - ok(0, "Wrong ret %u for %s\n", nts, var); - break; - } - } -} - -static void testSet(void) -{ - LPWSTR env; - char tmp[16]; - int i; - - ok(pRtlCreateEnvironment(FALSE, &env) == STATUS_SUCCESS, "Creating environment\n"); - - testSetHelper(&env, "cat", "dog", STATUS_SUCCESS, 0); - testSetHelper(&env, "cat", "horse", STATUS_SUCCESS, 0); - testSetHelper(&env, "cat", "zz", STATUS_SUCCESS, 0); - testSetHelper(&env, "cat", NULL, STATUS_SUCCESS, 0); - testSetHelper(&env, "cat", NULL, STATUS_SUCCESS, STATUS_VARIABLE_NOT_FOUND); - testSetHelper(&env, "foo", "meouw", STATUS_SUCCESS, 0); - testSetHelper(&env, "me=too", "also", STATUS_SUCCESS, STATUS_INVALID_PARAMETER); - testSetHelper(&env, "me", "too=also", STATUS_SUCCESS, 0); - testSetHelper(&env, "=too", "also", STATUS_SUCCESS, 0); - testSetHelper(&env, "=", "also", STATUS_SUCCESS, 0); - - for (i = 0; i < 128; i++) - { - sprintf(tmp, "zork%03d", i); - testSetHelper(&env, tmp, "is alive", STATUS_SUCCESS, 0); - } - - for (i = 0; i < 128; i++) - { - sprintf(tmp, "zork%03d", i); - testSetHelper(&env, tmp, NULL, STATUS_SUCCESS, 0); - } - testSetHelper(&env, "fOo", NULL, STATUS_SUCCESS, 0); - - ok(pRtlDestroyEnvironment(env) == STATUS_SUCCESS, "Destroying environment\n"); -} - static void testExpand(void) { static const struct test @@ -565,8 +483,8 @@ static NTSTATUS set_env_var(WCHAR **env, const WCHAR *var, const WCHAR *value) { UNICODE_STRING var_string, value_string; RtlInitUnicodeString(&var_string, var); - RtlInitUnicodeString(&value_string, value); - return RtlSetEnvironmentVariable(env, &var_string, &value_string); + if (value) RtlInitUnicodeString(&value_string, value); + return RtlSetEnvironmentVariable(env, &var_string, value ? &value_string : NULL); } static void check_env_var_(int line, const char *var, const char *value) @@ -659,6 +577,83 @@ static void test_RtlSetCurrentEnvironment(void) ok( size > get_env_length(env) * sizeof(WCHAR), "got wrong size %lu\n", size ); } +static void query_env_var_(int line, WCHAR *env, const WCHAR *var, const WCHAR *value) +{ + UNICODE_STRING var_string, value_string; + WCHAR value_buffer[9]; + NTSTATUS status; + + RtlInitUnicodeString(&var_string, var); + value_string.Buffer = value_buffer; + value_string.MaximumLength = sizeof(value_buffer); + + status = RtlQueryEnvironmentVariable_U(env, &var_string, &value_string); + if (value) + { + ok_(__FILE__, line)(!status, "got %#x\n", status); + ok_(__FILE__, line)(value_string.Length/sizeof(WCHAR) == wcslen(value), + "wrong size %u\n", value_string.Length/sizeof(WCHAR)); + ok_(__FILE__, line)(!wcscmp(value_string.Buffer, value), "wrong value %s\n", debugstr_w(value_string.Buffer)); + } + else + ok_(__FILE__, line)(status == STATUS_VARIABLE_NOT_FOUND, "got %#x\n", status); +} +#define query_env_var(a, b, c) query_env_var_(__LINE__, a, b, c) + +static void test_RtlSetEnvironmentVariable(void) +{ + NTSTATUS status; + WCHAR *env; + + status = RtlCreateEnvironment(FALSE, &env); + ok(!status, "got %#x\n", status); + + status = set_env_var(&env, L"cat", L"dog"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"cat", L"dog"); + + status = set_env_var(&env, L"cat", L"horse"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"cat", L"horse"); + + status = set_env_var(&env, L"cat", NULL); + ok(!status, "got %#x\n", status); + query_env_var(env, L"cat", NULL); + + status = set_env_var(&env, L"cat", NULL); + todo_wine ok(!status, "got %#x\n", status); + + status = set_env_var(&env, L"foo", L"meouw"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"foo", L"meouw"); + + status = set_env_var(&env, L"fOo", NULL); + ok(!status, "got %#x\n", status); + query_env_var(env, L"foo", NULL); + + status = set_env_var(&env, L"horse", NULL); + todo_wine ok(!status, "got %#x\n", status); + query_env_var(env, L"horse", NULL); + + status = set_env_var(&env, L"me=too", L"also"); + ok(status == STATUS_INVALID_PARAMETER, "got %#x\n", status); + + status = set_env_var(&env, L"me", L"too=also"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"me", L"too=also"); + + status = set_env_var(&env, L"=too", L"also"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"=too", L"also"); + + status = set_env_var(&env, L"=", L"also"); + ok(!status, "got %#x\n", status); + query_env_var(env, L"=", L"also"); + + status = RtlDestroyEnvironment(env); + ok(!status, "got %#x\n", status); +} + START_TEST(env) { HMODULE mod = GetModuleHandleA("ntdll.dll"); @@ -666,19 +661,15 @@ START_TEST(env) initial_env = NtCurrentTeb()->Peb->ProcessParameters->Environment; pRtlMultiByteToUnicodeN = (void *)GetProcAddress(mod,"RtlMultiByteToUnicodeN"); - pRtlCreateEnvironment = (void*)GetProcAddress(mod, "RtlCreateEnvironment"); - pRtlDestroyEnvironment = (void*)GetProcAddress(mod, "RtlDestroyEnvironment"); pRtlQueryEnvironmentVariable_U = (void*)GetProcAddress(mod, "RtlQueryEnvironmentVariable_U"); pRtlQueryEnvironmentVariable = (void*)GetProcAddress(mod, "RtlQueryEnvironmentVariable"); - pRtlSetCurrentEnvironment = (void*)GetProcAddress(mod, "RtlSetCurrentEnvironment"); - pRtlSetEnvironmentVariable = (void*)GetProcAddress(mod, "RtlSetEnvironmentVariable"); pRtlExpandEnvironmentStrings_U = (void*)GetProcAddress(mod, "RtlExpandEnvironmentStrings_U"); pRtlCreateProcessParameters = (void*)GetProcAddress(mod, "RtlCreateProcessParameters"); pRtlDestroyProcessParameters = (void*)GetProcAddress(mod, "RtlDestroyProcessParameters"); testQuery(); - testSet(); testExpand(); test_process_params(); test_RtlSetCurrentEnvironment(); + test_RtlSetEnvironmentVariable(); } -- 2.31.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51035 Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/ntdll/env.c | 3 +-- dlls/ntdll/tests/env.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/env.c b/dlls/ntdll/env.c index 1ed52b9728f..c11960db165 100644 --- a/dlls/ntdll/env.c +++ b/dlls/ntdll/env.c @@ -276,7 +276,7 @@ NTSTATUS WINAPI RtlSetEnvironmentVariable(PWSTR* penv, PUNICODE_STRING name, { INT varlen, len, old_size; LPWSTR p, env; - NTSTATUS nts = STATUS_VARIABLE_NOT_FOUND; + NTSTATUS nts = STATUS_SUCCESS; TRACE("(%p, %s, %s)\n", penv, debugstr_us(name), debugstr_us(value)); @@ -352,7 +352,6 @@ NTSTATUS WINAPI RtlSetEnvironmentVariable(PWSTR* penv, PUNICODE_STRING name, memcpy( p, value->Buffer, value->Length ); p[value->Length / sizeof(WCHAR)] = 0; } - nts = STATUS_SUCCESS; done: if (!penv) RtlReleasePebLock(); diff --git a/dlls/ntdll/tests/env.c b/dlls/ntdll/tests/env.c index eed8ae38c14..da61d11c1f2 100644 --- a/dlls/ntdll/tests/env.c +++ b/dlls/ntdll/tests/env.c @@ -621,7 +621,7 @@ static void test_RtlSetEnvironmentVariable(void) query_env_var(env, L"cat", NULL); status = set_env_var(&env, L"cat", NULL); - todo_wine ok(!status, "got %#x\n", status); + ok(!status, "got %#x\n", status); status = set_env_var(&env, L"foo", L"meouw"); ok(!status, "got %#x\n", status); @@ -632,7 +632,7 @@ static void test_RtlSetEnvironmentVariable(void) query_env_var(env, L"foo", NULL); status = set_env_var(&env, L"horse", NULL); - todo_wine ok(!status, "got %#x\n", status); + ok(!status, "got %#x\n", status); query_env_var(env, L"horse", NULL); status = set_env_var(&env, L"me=too", L"also"); -- 2.31.1
Gijs Vermeulen <gijsvrm(a)gmail.com> wrote:
diff --git a/dlls/ntdll/tests/env.c b/dlls/ntdll/tests/env.c index 63a9a0e463c..eed8ae38c14 100644 --- a/dlls/ntdll/tests/env.c +++ b/dlls/ntdll/tests/env.c @@ -24,12 +24,8 @@
static NTSTATUS (WINAPI *pRtlMultiByteToUnicodeN)( LPWSTR dst, DWORD dstlen, LPDWORD reslen, LPCSTR src, DWORD srclen ); -static NTSTATUS (WINAPI *pRtlCreateEnvironment)(BOOLEAN, PWSTR*); -static NTSTATUS (WINAPI *pRtlDestroyEnvironment)(PWSTR); static NTSTATUS (WINAPI *pRtlQueryEnvironmentVariable_U)(PWSTR, PUNICODE_STRING, PUNICODE_STRING); static NTSTATUS (WINAPI* pRtlQueryEnvironmentVariable)(WCHAR*, WCHAR*, SIZE_T, WCHAR*, SIZE_T, SIZE_T*); -static void (WINAPI *pRtlSetCurrentEnvironment)(PWSTR, PWSTR*); -static NTSTATUS (WINAPI *pRtlSetEnvironmentVariable)(PWSTR*, PUNICODE_STRING, PUNICODE_STRING); static NTSTATUS (WINAPI *pRtlExpandEnvironmentStrings_U)(LPWSTR, PUNICODE_STRING, PUNICODE_STRING, PULONG); static NTSTATUS (WINAPI *pRtlCreateProcessParameters)(RTL_USER_PROCESS_PARAMETERS**, const UNICODE_STRING*, const UNICODE_STRING*,
Removing dynamic loading of ntdll APIs will unnecessarily break building the tests with older PSDK versions, in particular Windows 7/8 ones. I'm occasionally use Windows 7 to test things, I'd appreciate if building under that platform keeps working. -- Dmitry.
Op wo 21 apr. 2021 om 21:14 schreef Dmitry Timoshkov <dmitry(a)baikal.ru>:
Removing dynamic loading of ntdll APIs will unnecessarily break building the tests with older PSDK versions, in particular Windows 7/8 ones. I'm occasionally use Windows 7 to test things, I'd appreciate if building under that platform keeps working.
Isn't this already the case? (i.e. RtlInitUnicodeString in set_env_var() and RtlSetCurrentEnvironment in test_RtlSetCurrentEnvironment())
Gijs Vermeulen <gijsvrm(a)gmail.com> wrote:
Op wo 21 apr. 2021 om 21:14 schreef Dmitry Timoshkov <dmitry(a)baikal.ru>:
Removing dynamic loading of ntdll APIs will unnecessarily break building the tests with older PSDK versions, in particular Windows 7/8 ones. I'm occasionally use Windows 7 to test things, I'd appreciate if building under that platform keeps working.
Isn't this already the case? (i.e. RtlInitUnicodeString in set_env_var() and RtlSetCurrentEnvironment in test_RtlSetCurrentEnvironment())
I haven't built the ntdll tests for some time, so probably something went in and broke things, however fixing a couple APIs is much less trouble than dealing with completely broken test. Is keeping dynamic API loading something that can't be left as it is, without making things worse? Honestly, it doesn't require that much effort. -- Dmitry.
Op wo 21 apr. 2021 om 21:30 schreef Dmitry Timoshkov <dmitry(a)baikal.ru>:
I haven't built the ntdll tests for some time, so probably something went in and broke things, however fixing a couple APIs is much less trouble than dealing with completely broken test. Is keeping dynamic API loading something that can't be left as it is, without making things worse? Honestly, it doesn't require that much effort.
I was more trying to explain why I did it this way and not trying to give a reason as to why it should be this way, as I don't have an opinion on that. I'll resend with the requested changes.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Gijs Vermeulen <gijsvrm(a)gmail.com> wrote:
Op wo 21 apr. 2021 om 21:14 schreef Dmitry Timoshkov <dmitry(a)baikal.ru>:
Removing dynamic loading of ntdll APIs will unnecessarily break building the tests with older PSDK versions, in particular Windows 7/8 ones. I'm occasionally use Windows 7 to test things, I'd appreciate if building under that platform keeps working.
Isn't this already the case? (i.e. RtlInitUnicodeString in set_env_var() and RtlSetCurrentEnvironment in test_RtlSetCurrentEnvironment())
I haven't built the ntdll tests for some time, so probably something went in and broke things, however fixing a couple APIs is much less trouble than dealing with completely broken test. Is keeping dynamic API loading something that can't be left as it is, without making things worse? Honestly, it doesn't require that much effort.
It does require some effort, because there's no way of testing it in standard setups, so it gets constantly broken and needs to be fixed up after the fact. And it's clearly not that important to you either if you didn't bother to fix the previous breakages. AFAICT the env.c test would have been broken for a year, and the ntdll tests as a whole for at least five years now. So you can't demand that everybody do extra work for something that you don't seem to be actually using. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Gijs Vermeulen <gijsvrm(a)gmail.com> wrote:
Op wo 21 apr. 2021 om 21:14 schreef Dmitry Timoshkov <dmitry(a)baikal.ru>:
Removing dynamic loading of ntdll APIs will unnecessarily break building the tests with older PSDK versions, in particular Windows 7/8 ones. I'm occasionally use Windows 7 to test things, I'd appreciate if building under that platform keeps working.
Isn't this already the case? (i.e. RtlInitUnicodeString in set_env_var() and RtlSetCurrentEnvironment in test_RtlSetCurrentEnvironment())
I haven't built the ntdll tests for some time, so probably something went in and broke things, however fixing a couple APIs is much less trouble than dealing with completely broken test. Is keeping dynamic API loading something that can't be left as it is, without making things worse? Honestly, it doesn't require that much effort.
It does require some effort, because there's no way of testing it in standard setups, so it gets constantly broken and needs to be fixed up after the fact. And it's clearly not that important to you either if you didn't bother to fix the previous breakages.
As I've mentioned, I compile the ntdll tests under Windows 7 occasionally, I didn't claim that I promise to continuously monitor the tests for breakages. Still, it's nice to spend less effort when it comes to building the tests.
AFAICT the env.c test would have been broken for a year, and the ntdll tests as a whole for at least five years now. So you can't demand that everybody do extra work for something that you don't seem to be actually using.
I hope that I asked nicely enough, and didn't ask that much, I wouldn't call this as a demand. I think that it doesn't count as an extra work simply keeping existing API pointers. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
I hope that I asked nicely enough, and didn't ask that much, I wouldn't call this as a demand. I think that it doesn't count as an extra work simply keeping existing API pointers.
It's still extra work and extra code, so there needs to be some benefit to doing it. Of course keeping the existing pointers isn't much work, but it doesn't do any good unless new code also uses pointers everywhere. Clearly that's not the case. It used to be necessary to do this since there was no ntdll import lib, but it's been available in the PSDK for about 10 years now, so honestly I don't see the point anymore. If you absolutely need to use such an old PSDK, is there a good reason you can't simply copy an ntdll import lib into it once and for all? -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Dmitry Timoshkov -
Gijs Vermeulen