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

[Vijay Kiran Kamuju]

Signed-off-by: Vijay Kiran Kamuju

[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

[Vijay Kiran Kamuju]

On Sat, Mar 21, 2020 at 4:49 PM Zebediah Figura  wrote:
> 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.

This is used for tracing the quota values on different test systems
with various RAM configurations.
If you recommend, I will remove this 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.
> > +}
> > -
> >
> > +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.

I wanted to test on a new process rather than the current process.
> > -
> > - 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.

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",&quotalimits);
>
> 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(&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.)

Will do the changes.
> > - 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

[Zebediah Figura]

On 3/21/20 11:06 AM, Vijay Kiran Kamuju wrote:
> On Sat, Mar 21, 2020 at 4:49 PM Zebediah Figura  wrote:
> > 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.
>
> 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, &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.
>
> I wanted to test on a new process rather than the current process.

Why?
> > > -
> > > - 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.
>
> 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",&quotalimits);
> >
> > 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(&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.)
>
> Will do the changes.
> > > - 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

[Vijay Kiran Kamuju]

On Sat, Mar 21, 2020 at 5:18 PM Zebediah Figura  wrote:
> On 3/21/20 11:06 AM, Vijay Kiran Kamuju wrote:
> > On Sat, Mar 21, 2020 at 4:49 PM Zebediah Figura  wrote:
> > > 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.
> >
> > 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.

Ok, I will do that as well.
> > > > +{
> > > > - 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.

Well, I have seen some presentations related to PagedPoolLimit,
NonPagedPoolLimitMinimumWorkingSetSize, MaximumWorkingSetSize
https://techcommunity.microsoft.com/t5/windows-blog-archive/pushing-the-limi...
I forgot the link for the ppt presentation.
> > > > +}
> > > > -
> > > >
> > > > +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.
> >
> > I wanted to test on a new process rather than the current process.
>
> Why?

I thought the winetest may take up lot of resources, which might
change the limits for the current process.
I will change it to use current process.
> > > > -
> > > > - 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.
> >
> > 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",&quotalimits);
> > >
> > > 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(&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.)
> >
> > Will do the changes.
> > > > - 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