Re: [PATCH v3 2/2] ntdll/tests: Add tests for ProcessQuotaLimits class

[Zebediah Figura]

Hello Vijay,

On 3/20/20 4:46 PM, Vijay Kiran Kamuju wrote:
> From: Vijay Kiran Kamuju 
> Subject: [PATCH v3 2/2] ntdll/tests: Add tests for ProcessQuotaLimits class
> Message-Id: 
> Date: Fri, 20 Mar 2020 22:46:50 +0100
>
> Signed-off-by: Vijay Kiran Kamuju
>
> From 6bada725c1bb59bb127729cd5f373c29868d880d Mon Sep 17 00:00:00 2001
> From: Vijay Kiran Kamuju 
> 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
>
> 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.
> +{
> - 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.
> +}
> -
>
> +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, &quotalimits, sizeof(quotalimits), NULL);
> - ok( status == STATUS_INVALID_HANDLE, "Expected STATUS_INVALID_HANDLE, got %08x\n", status);
> -
> - status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, &quotalimits, 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.
> -
> - memset(&quotalimits, 0, sizeof(quotalimits));
> - status = pNtQueryInformationProcess( process, ProcessQuotaLimits, &quotalimits, 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.
> - 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.
> - dump_quota_limits("Quota Limits",&quotalimits);

I think we need less tracing in this test by default, not more. I'd
recommend checking against winetest_debug > 1.
> -
> - memset(&quotalimits, 0, sizeof(quotalimits));
> - status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, &quotalimits, 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.)
> - ok( sizeof(quotalimits) == ReturnLength, "Inconsistent length %d\n", ReturnLength);
> -
> - memset(&quotalimits, 0, sizeof(quotalimits));
> - status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, &quotalimits, 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(&quotalimits, 0, sizeof(quotalimits));
> - status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessQuotaLimits, &quotalimits, 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