Re: [PATCH 1/2] ntdll: Add NtQueryMutant
Hi Daniel, thanks for working on this. I have a couple of remarks, see below. On 29.04.2016 03:53, Daniel Lehman wrote:
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/ntdll/sync.c | 38 +++++++++++++---- dlls/ntdll/tests/om.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++ server/mutex.c | 16 ++++++++ server/protocol.def | 10 +++++ 4 files changed, 167 insertions(+), 9 deletions(-)
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index e87e672..576593f 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -520,15 +520,35 @@ NTSTATUS WINAPI NtReleaseMutant( IN HANDLE handle, OUT PLONG prev_count OPTIONAL * NtQueryMutant [NTDLL.@] * ZwQueryMutant [NTDLL.@] */ -NTSTATUS WINAPI NtQueryMutant(IN HANDLE handle, - IN MUTANT_INFORMATION_CLASS MutantInformationClass, - OUT PVOID MutantInformation, - IN ULONG MutantInformationLength, - OUT PULONG ResultLength OPTIONAL ) -{ - FIXME("(%p %u %p %u %p): stub!\n", - handle, MutantInformationClass, MutantInformation, MutantInformationLength, ResultLength);
Please convert the FIXME to a TRACE instead of removing it.
- return STATUS_NOT_IMPLEMENTED; +NTSTATUS WINAPI NtQueryMutant( HANDLE handle, MUTANT_INFORMATION_CLASS class, + void *info, ULONG len, ULONG *ret_len ) +{ + NTSTATUS ret; + MUTANT_BASIC_INFORMATION *out = info; + + if (class != MutantBasicInformation) + { + FIXME("(%p, %d, %d) Unknown class\n", + handle, class, len); + return STATUS_INVALID_INFO_CLASS; + } + + if (len != sizeof(MUTANT_BASIC_INFORMATION)) return STATUS_INFO_LENGTH_MISMATCH; + + SERVER_START_REQ( query_mutex ) + { + req->handle = wine_server_obj_handle( handle ); + if (!(ret = wine_server_call( req ))) + { + out->CurrentCount = 1 - reply->count; + out->OwnedByCaller = reply->owned; + out->AbandonedState = reply->abandoned; + if (ret_len) *ret_len = sizeof(MUTANT_BASIC_INFORMATION); + } + } + SERVER_END_REQ; + + return ret; }
/* diff --git a/dlls/ntdll/tests/om.c b/dlls/ntdll/tests/om.c index c05f31d..ee850d3 100644 --- a/dlls/ntdll/tests/om.c +++ b/dlls/ntdll/tests/om.c @@ -43,6 +43,8 @@ static NTSTATUS (WINAPI *pNtCreateMailslotFile)( PHANDLE, ACCESS_MASK, POBJECT_A ULONG, ULONG, ULONG, PLARGE_INTEGER ); static NTSTATUS (WINAPI *pNtCreateMutant)( PHANDLE, ACCESS_MASK, const POBJECT_ATTRIBUTES, BOOLEAN ); static NTSTATUS (WINAPI *pNtOpenMutant) ( PHANDLE, ACCESS_MASK, const POBJECT_ATTRIBUTES ); +static NTSTATUS (WINAPI *pNtQueryMutant) ( HANDLE, MUTANT_INFORMATION_CLASS, PVOID, ULONG, PULONG ); +static NTSTATUS (WINAPI *pNtReleaseMutant)( HANDLE, PLONG ); static NTSTATUS (WINAPI *pNtCreateSemaphore)( PHANDLE, ACCESS_MASK,const POBJECT_ATTRIBUTES,LONG,LONG ); static NTSTATUS (WINAPI *pNtOpenSemaphore)( PHANDLE, ACCESS_MASK, const POBJECT_ATTRIBUTES ); static NTSTATUS (WINAPI *pNtCreateTimer) ( PHANDLE, ACCESS_MASK, const POBJECT_ATTRIBUTES, TIMER_TYPE ); @@ -1865,6 +1867,113 @@ static void test_null_device(void) CloseHandle(ov.hEvent); }
+static DWORD WINAPI mutant_thread( void *arg ) +{ + MUTANT_BASIC_INFORMATION info; + NTSTATUS status; + HANDLE mutant; + + mutant = arg; + status = WaitForSingleObject( mutant, 1000 ); + ok( status == STATUS_SUCCESS, "WaitForSingleObject failed %08x\n", status );
WaitForSingleObject does not return STATUS_* values. This error also appears a couple more times below.
+ + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == 0, "NtQueryMutant failed, expected 0, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == TRUE, "NtQueryMutant failed, expected TRUE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.AbandonedState );
Its a matter of taste, but I am not sure if it makes sense to duplicate the "NtQueryMutant failed". Better print the variable which was tested, for example "expected CurrentCount == 0, got %d\n", and so on.
+ /* abandon mutant */ + + return 0; +} + +static void test_mutant(void) +{ + static const WCHAR name[] = {'\\','B','a','s','e','N','a','m','e','d','O','b','j','e','c','t','s', + '\\','t','e','s','t','_','m','u','t','a','n','t',0}; + MUTANT_BASIC_INFORMATION info; + OBJECT_ATTRIBUTES attr; + UNICODE_STRING str; + NTSTATUS status; + HANDLE mutant; + HANDLE thread; + ULONG len; + LONG prev; + + pRtlInitUnicodeString(&str, name); + InitializeObjectAttributes(&attr, &str, 0, 0, NULL); + status = pNtCreateMutant(&mutant, GENERIC_ALL, &attr, TRUE); + ok( status == STATUS_SUCCESS, "Failed to create Mutant(%08x)\n", status ); + + /* bogus */ + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, 0, NULL); + ok( status == STATUS_INFO_LENGTH_MISMATCH, + "Failed to NtQueryMutant, expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status ); + status = pNtQueryMutant(mutant, 0x42, &info, sizeof(info), NULL); + ok( status == STATUS_INVALID_INFO_CLASS, + "Failed to NtQueryMutant, expected STATUS_INVALID_INFO_CLASS, got %08x\n", status ); + status = pNtQueryMutant((HANDLE)0xdeadbeef, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_INVALID_HANDLE, + "Failed to NtQueryMutant, expected STATUS_INVALID_HANDLE, got %08x\n", status );
Not sure if you have seen it, but your patch causes new test failures, see: http://newtestbot.winehq.org/JobDetails.pl?Key=22713
+ + /* new */ + len = -1; + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), &len); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == 0, "NtQueryMutant failed, expected 0, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == TRUE, "NtQueryMutant failed, expected TRUE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.AbandonedState ); + ok( len == sizeof(info), "NtQueryMutant failed, expected %u, got %u\n", (DWORD)sizeof(info), len );
Its better to avoid printing sizeof() values, only print the "len" value here.
+
There is a whitespace issue in the line above.
+ status = WaitForSingleObject( mutant, 1000 ); + ok( status == STATUS_SUCCESS, "WaitForSingleObject failed %08x\n", status ); + + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == -1, "NtQueryMutant failed, expected -1, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == TRUE, "NtQueryMutant failed, expected TRUE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.AbandonedState ); + + prev = 0xdeadbeef; + status = pNtReleaseMutant(mutant, &prev); + ok( status == STATUS_SUCCESS, "NtQueryRelease failed %08x\n", status ); + todo_wine ok( prev == -1, "NtQueryRelease failed, expected -1, got %d\n", prev ); + + prev = 0xdeadbeef; + status = pNtReleaseMutant(mutant, &prev); + ok( status == STATUS_SUCCESS, "NtQueryRelease failed %08x\n", status ); + todo_wine ok( prev == 0, "NtQueryRelease failed, expected 0, got %d\n", prev ); + + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == 1, "NtQueryMutant failed, expected 1, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.AbandonedState ); + + /* abandoned */ + thread = CreateThread( NULL, 0, mutant_thread, mutant, 0, NULL ); + status = WaitForSingleObject( thread, 1000 ); + ok( status == 0, "WaitForSingleObject failed %08x\n", status ); + NtClose( thread ); + + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == 1, "NtQueryMutant failed, expected 0, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == TRUE, "NtQueryMutant failed, expected TRUE, got %d\n", info.AbandonedState ); + + status = WaitForSingleObject( mutant, 1000 ); + ok( status == STATUS_ABANDONED_WAIT_0, "WaitForSingleObject failed %08x\n", status ); + + status = pNtQueryMutant(mutant, MutantBasicInformation, &info, sizeof(info), NULL); + ok( status == STATUS_SUCCESS, "NtQueryMutant failed %08x\n", status ); + ok( info.CurrentCount == 0, "NtQueryMutant failed, expected 0, got %d\n", info.CurrentCount ); + ok( info.OwnedByCaller == TRUE, "NtQueryMutant failed, expected TRUE, got %d\n", info.OwnedByCaller ); + ok( info.AbandonedState == FALSE, "NtQueryMutant failed, expected FALSE, got %d\n", info.AbandonedState ); + + NtClose( mutant ); +} + START_TEST(om) { HMODULE hntdll = GetModuleHandleA("ntdll.dll"); @@ -1892,6 +2001,8 @@ START_TEST(om) pNtQueryEvent = (void *)GetProcAddress(hntdll, "NtQueryEvent"); pNtPulseEvent = (void *)GetProcAddress(hntdll, "NtPulseEvent"); pNtOpenMutant = (void *)GetProcAddress(hntdll, "NtOpenMutant"); + pNtQueryMutant = (void *)GetProcAddress(hntdll, "NtQueryMutant"); + pNtReleaseMutant = (void *)GetProcAddress(hntdll, "NtReleaseMutant"); pNtOpenFile = (void *)GetProcAddress(hntdll, "NtOpenFile"); pNtClose = (void *)GetProcAddress(hntdll, "NtClose"); pRtlInitUnicodeString = (void *)GetProcAddress(hntdll, "RtlInitUnicodeString"); @@ -1925,6 +2036,7 @@ START_TEST(om) test_query_object(); test_type_mismatch(); test_event(); + test_mutant(); test_keyed_events(); test_null_device(); } diff --git a/server/mutex.c b/server/mutex.c index 3693095..b183934 100644 --- a/server/mutex.c +++ b/server/mutex.c @@ -251,3 +251,19 @@ DECL_HANDLER(release_mutex) release_object( mutex ); } } + +/* return details about the event */
Copy-paste mistake?
+DECL_HANDLER(query_mutex) +{ + struct mutex *mutex; + + if ((mutex = (struct mutex *)get_handle_obj( current->process, req->handle, + MUTANT_QUERY_STATE, &mutex_ops ))) + { + reply->count = mutex->count; + reply->owned = mutex->owner == current;
I usually prefer brackets around such comparisons, but not sure if its strictly required.
+ reply->abandoned = mutex->abandoned; + + release_object( mutex ); + } +} diff --git a/server/protocol.def b/server/protocol.def index a5a45eb..3359b77 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1093,6 +1093,16 @@ enum event_op { PULSE_EVENT, SET_EVENT, RESET_EVENT }; @END
+/* Query a mutex */ +(a)REQ(query_mutex) + obj_handle_t handle; /* handle to event */ +(a)REPLY + unsigned int count; /* current count of mutex */ + int owned; /* true if owned by current thread */ + int abandoned; /* true if abandoned */ +(a)END + + /* Create a semaphore */ @REQ(create_semaphore) unsigned int access; /* wanted access rights */
participants (1)
-
Sebastian Lackner