[PATCH v3 2/2] ntdll/tests: Add tests for ProcessQuotaLimits class
Zebediah Figura
z.figura12 at gmail.com
Sat Mar 21 11:11:04 CDT 2020
On 3/21/20 11:06 AM, Vijay Kiran Kamuju wrote:
> On Sat, Mar 21, 2020 at 4:49 PM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>
>> Hello Vijay,
>>
>> On 3/20/20 4:46 PM, Vijay Kiran Kamuju wrote:
>>> From: Vijay Kiran Kamuju <infyquest at gmail.com>
>>> Subject: [PATCH v3 2/2] ntdll/tests: Add tests for ProcessQuotaLimits class
>>> Message-Id: <CACfa+KKTLOLG6pTUJO-8b7ez43oSVRnbtPKJ9tALc3JqVcFodw at mail.gmail.com>
>>> Date: Fri, 20 Mar 2020 22:46:50 +0100
>>>
>>> Signed-off-by: Vijay Kiran Kamuju <infyquest at gmail.com>
>>>
>>> From 6bada725c1bb59bb127729cd5f373c29868d880d Mon Sep 17 00:00:00 2001
>>> From: Vijay Kiran Kamuju <infyquest at gmail.com>
>>> Date: Sat, 15 Feb 2020 10:03:51 +0100
>>> Subject: [PATCH v3 2/2] ntdll/tests: Add tests for ProcessQuotaLimits class
>>>
>>> Signed-off-by: Vijay Kiran Kamuju <infyquest at gmail.com>
>>> ---
>>> dlls/ntdll/tests/info.c | 72 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 72 insertions(+)
>>>
>>> diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c
>>> index 8dc8bad645..7d3278b32e 100644
>>> --- a/dlls/ntdll/tests/info.c
>>> +++ b/dlls/ntdll/tests/info.c
>>> @@ -1308,6 +1308,74 @@ static void test_query_process_basic(void)
>>> ok( pbi.UniqueProcessId > 0, "Expected a ProcessID > 0, got 0\n");
>>> }
>>>
>>> +static void dump_quota_limits(const char *header, const QUOTA_LIMITS *pql)
>>
>> Is there any particular reason this needs to be a separate function?
>> Note that unlike dump_vm_counters(), this is only called once.
> This is used for tracing the quota values on different test systems
> with various RAM configurations.
> If you recommend, I will remove this function.
I'm not necessarily suggesting to remove the traces, I just mean that
you don't need to put them in a separate function.
>>
>>> +{
>>> + trace("%s:\n", header);
>>> + trace("PagedPoolLimit : %lu\n", pql->PagedPoolLimit);
>>> + trace("NonPagedPoolLimit : %lu\n", pql->NonPagedPoolLimit);
>>> + trace("MinimumWorkingSetSize : %lu\n", pql->MinimumWorkingSetSize);
>>> + trace("MaximumWorkingSetSize : %lu\n", pql->MaximumWorkingSetSize);
>>> + trace("PagefileLimit : %lu\n", pql->PagefileLimit);
>>> + trace("TimeLimit : %s\n", wine_dbgstr_longlong(pql->TimeLimit.QuadPart));
>>
>> You already test these last four members against fixed values below; I'm
>> not sure there's a point in printing their values in that case.
> I am testing these as per previous recommendations, but some of values
> change by RAM values.
I'm not sure I see the review you're referring to, but my point is that
the last four members (MinimumWorkingSetSize, MaximumWorkingSetSize,
PagefileLimit, TimeLimit) are compared against fixed values, so there's
no point printing their actual values. If the values are different from
what you expect, you'll already get a failure in the ok() messages
below. If those values do differ across different machines, then you
probably shouldn't be using ok() to check them.
>>
>>> +}
>>> +
>>> +static void test_query_process_quotalimits(void)
>>> +{
>>> + NTSTATUS status;
>>> + QUOTA_LIMITS quotalimits;
>>> + ULONG ReturnLength;
>>> + HANDLE process;
>>> +
>>> + status = pNtQueryInformationProcess(NULL, ProcessQuotaLimits, NULL, sizeof(quotalimits), NULL);
>>> + ok( status == STATUS_ACCESS_VIOLATION || status == STATUS_INVALID_HANDLE,
>>> + "Expected STATUS_ACCESS_VIOLATION or STATUS_INVALID_HANDLE(W2K3), got %08x\n", status);
>>> +
>>> + status = pNtQueryInformationProcess(NULL, ProcessQuotaLimits, "alimits, sizeof(quotalimits), NULL);
>>> + ok( status == STATUS_INVALID_HANDLE, "Expected STATUS_INVALID_HANDLE, got %08x\n", status);
>>> +
>>> + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, "alimits, 2, &ReturnLength);
>>> + ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status);
>>> +
>>> + process = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, one_before_last_pid);
>>> + if (!process)
>>> + {
>>> + trace("Could not open process with ID : %d, error : %u. Going to use current one.\n", one_before_last_pid, GetLastError());
>>> + process = GetCurrentProcess();
>>> + trace("ProcessQuotaLimits for current process\n");
>>> + }
>>> + else
>>> + trace("ProcessQuotaLimits for process with ID : %d\n", one_before_last_pid);
>>
>> I don't think there's a point in doing this for this test. See the
>> comment regarding one_before_last_pid at the top of the file.
> I wanted to test on a new process rather than the current process.
Why?
>>
>>> +
>>> + memset("alimits, 0, sizeof(quotalimits));
>>> + status = pNtQueryInformationProcess( process, ProcessQuotaLimits, "alimits, sizeof(quotalimits), &ReturnLength);
>>> + ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08x\n", status);
>>> + ok( sizeof(quotalimits) == ReturnLength, "Inconsistent length %d\n", ReturnLength);
>>> + ok(quotalimits.MinimumWorkingSetSize == 204800,"Got MinimumWorkingSetSize = %lu\n",quotalimits.MinimumWorkingSetSize);
>>> + ok(quotalimits.MaximumWorkingSetSize == 1413120,"Got MaximumWorkingSetSize = %lu\n",quotalimits.MaximumWorkingSetSize);
>>> + ok(quotalimits.PagefileLimit == -1,"Got PagefileLimit = %lu\n",quotalimits.PagefileLimit);
>>
>> Instead of comparing unsigned values to -1, comparing them to ~0 or
>> SIZE_MAX might be better.
> Will do the changes.
>>
>>> + ok(quotalimits.TimeLimit.QuadPart == -1,"Got TimeLimit = %s\n",wine_dbgstr_longlong(quotalimits.TimeLimit.QuadPart));
>>> + CloseHandle(process);
>>
>> Style nitpick, the spacing here and elsewhere is rather inconsistent.
> Will do the changes.
>>
>>> + dump_quota_limits("Quota Limits","alimits);
>>
>> I think we need less tracing in this test by default, not more. I'd
>> recommend checking against winetest_debug > 1.
> Will do the changes.
>>
>>> +
>>> + memset("alimits, 0, sizeof(quotalimits));
>>> + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, "alimits, sizeof(quotalimits)*2, &ReturnLength);
>>> + ok( status == STATUS_INFO_LENGTH_MISMATCH,
>>> + "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status);
>> Style nitpick: it seems weird to break this line here when the previous
>> line is longer and unbroken; I'd break both lines or neither.
>>
>> (Using shorter ok() messages might not be a bad idea, either.)
>>
> Will do the changes.
>>> + ok( sizeof(quotalimits) == ReturnLength, "Inconsistent length %d\n", ReturnLength);
>>> +
>>> + memset("alimits, 0, sizeof(quotalimits));
>>> + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, "alimits, sizeof(quotalimits)-1, &ReturnLength);
>>> + ok( status == STATUS_INFO_LENGTH_MISMATCH,
>>> + "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status);
>>> + ok( sizeof(quotalimits) == ReturnLength, "Inconsistent length %d\n", ReturnLength);
>>> +
>>> + memset("alimits, 0, sizeof(quotalimits));
>>> + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, "alimits, sizeof(quotalimits)+1, &ReturnLength);
>>> + ok( status == STATUS_INFO_LENGTH_MISMATCH,
>>> + "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status);
>>> + ok( sizeof(quotalimits) == ReturnLength, "Inconsistent length %d\n", ReturnLength);
>>> +}
>>> +
>>> static void dump_vm_counters(const char *header, const VM_COUNTERS *pvi)
>>> {
>>> trace("%s:\n", header);
>>> @@ -2590,6 +2658,10 @@ START_TEST(info)
>>> trace("Starting test_query_process_basic()\n");
>>> test_query_process_basic();
>>>
>>> + /* 0x1 ProcessQuotaLimits */
>>> + trace("Starting test_query_process_quotalimits()\n");
>>> + test_query_process_quotalimits();
>>> +
>>> /* 0x2 ProcessIoCounters */
>>> trace("Starting test_query_process_io()\n");
>>> test_query_process_io();
>>>
>>> --
>>> 2.25.2
>>>
>>
>
More information about the wine-devel
mailing list