[PATCH 0/2] MR5139: qwave: Add QOSCloseHandle() stub.

[Aida Jonikienė (＠DodoGTA)]

Discord calls this function when hanging up a call.

[PATCH 1/2] qwave: Add QOSCloseHandle() stub.

[Aida Jonikienė]

From: Aida Jonikienė

Discord calls this function when hanging up a call.
---
 dlls/qwave/main.c     | 10 ++++++++++
 dlls/qwave/qwave.spec |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/dlls/qwave/main.c b/dlls/qwave/main.c
index a8755d993d1..6b5a373efe2 100644
--- a/dlls/qwave/main.c
+++ b/dlls/qwave/main.c
@@ -43,3 +43,13 @@ BOOL WINAPI QOSAddSocketToFlow(HANDLE handle, SOCKET socket, PSOCKADDR addr,
     SetLastError(ERROR_NOT_SUPPORTED);
     return FALSE;
 }
+
+BOOL WINAPI QOSCloseHandle(HANDLE handle)
+{
+    FIXME("%p stub!\n", handle);
+    if (!handle || IsBadReadPtr(handle, sizeof(HANDLE)))
+        SetLastError(ERROR_INVALID_HANDLE);
+    else
+        SetLastError(ERROR_NOT_SUPPORTED);
+    return FALSE;
+}
diff --git a/dlls/qwave/qwave.spec b/dlls/qwave/qwave.spec
index 83dfc6787cc..ff2994d11fb 100644
--- a/dlls/qwave/qwave.spec
+++ b/dlls/qwave/qwave.spec
@@ -2,7 +2,7 @@
 @ stub QDLHStartDiagnosingPath
 @ stdcall QOSAddSocketToFlow(ptr long ptr long long ptr)
 @ stub QOSCancel
-@ stub QOSCloseHandle
+@ stdcall QOSCloseHandle(ptr)
 @ stdcall QOSCreateHandle(ptr ptr)
 @ stub QOSEnumerateFlows
 @ stub QOSNotifyFlow

[PATCH 2/2] qwave: Add tests for QOSCloseHandle().

[Aida Jonikienė]

From: Aida Jonikienė

---
 dlls/qwave/tests/qos.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/dlls/qwave/tests/qos.c b/dlls/qwave/tests/qos.c
index 7e8fddf64c4..0071e2827d5 100644
--- a/dlls/qwave/tests/qos.c
+++ b/dlls/qwave/tests/qos.c
@@ -64,7 +64,35 @@ static void test_QOSCreateHandle(void)
     ok(GetLastError() == ERROR_INVALID_PARAMETER, "Expected ERROR_INVALID_PARAMETER, got %ld\n", GetLastError());
 }

+static void test_QOSCloseHandle(void)
+{
+    QOS_VERSION ver;
+    HANDLE h;
+    BOOL ret;
+
+    SetLastError(0xdeadbeef);
+    ret = QOSCloseHandle(NULL);
+    ok(ret == FALSE, "Expected FALSE, got %d\n", ret);
+    ok(GetLastError() == ERROR_INVALID_HANDLE, "Expected ERROR_INVALID_HANDLE, got %ld\n", GetLastError());
+
+    ver.MajorVersion = 1;
+    ver.MinorVersion = 0;
+    ret = QOSCreateHandle(&ver, &h);
+    todo_wine ok(ret == TRUE, "Expected TRUE, got %d\n", ret);
+
+    SetLastError(0xdeadbeef);
+    ret = QOSCloseHandle(h);
+    todo_wine ok(ret == TRUE, "Expected TRUE, got %d\n", ret);
+    todo_wine ok(GetLastError() == 0xdeadbeef, "Expected 0xdeadbeef, got %ld\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    ret = QOSCloseHandle((HANDLE)0xdeadbeef);
+    ok(ret == FALSE, "Expected FALSE, got %d\n", ret);
+    ok(GetLastError() == ERROR_INVALID_HANDLE, "Expected ERROR_INVALID_HANDLE, got %ld\n", GetLastError());
+}
+
 START_TEST(qos)
 {
     test_QOSCreateHandle();
+    test_QOSCloseHandle();
 }

[Zebediah Figura (＠zfigura)]

Why are you checking for a null handle, especially without tests? Moreover, why are you using IsBadReadPtr() on a handle?

[Aida Jonikienė (＠DodoGTA)]

On Thu Feb 22 19:43:50 2024 +0000, Zebediah Figura wrote:
> Why are you checking for a null handle, especially without tests?
> Moreover, why are you using IsBadReadPtr() on a handle?

1) I already have a test case for the NULL handle
2) I think I need it to pass the last QOSCloseHandle() test case (the one where I set the handle value to 0xdeadbeef)

[Zebediah Figura (＠zfigura)]

> - I already have a test case for the NULL handle

Eh, sorry, I'm blind. Still, why do you care?
> - I think I need it to pass the last QOSCloseHandle() test case (the one where I set the handle value to 0xdeadbeef)

That test is failing because the handle was never allocated. Why would dereferencing the handle make any sense?

[Aida Jonikienė (＠DodoGTA)]

> Eh, sorry, I'm blind. Still, why do you care?

I want to make the function as accurate to Windows until it gets properly implemented
> That test is failing because the handle was never allocated.

All of my tests are passing on Windows: https://testbot.winehq.org/JobDetails.pl?Key=143303 (including the 0xdeadbeef handle one) so I want to replicate that handle behavior on Wine too

[Zebediah Figura (＠zfigura)]

> > Eh, sorry, I'm blind. Still, why do you care?
>
> I want to make the function as accurate to Windows until it gets properly implemented

What's the point of making it accurate if it's not implemented, especially if you have to add incorrect implementation to do it? Does the program depend on this behaviour?

[Aida Jonikienė (＠DodoGTA)]

> especially if you have to add incorrect implementation to do it

Is incorrect implementation the ERROR_NOT_SUPPORTED part or the IsBadReadPtr() part?
> Does the program depend on this behaviour?

I just checked Chromium source code and it just ignores return value/last error entirely (so most likely not)

[Zebediah Figura (＠zfigura)]

> > especially if you have to add incorrect implementation to do it
>
> Is incorrect implementation the ERROR_NOT_SUPPORTED part or the IsBadReadPtr() part?

IsBadReadPtr() is an attempt to actually implement some subset of correct behaviour, and it looks very wrong.

[Aida Jonikienė (＠DodoGTA)]

> IsBadReadPtr() is an attempt to actually implement some subset of correct behaviour, and it looks very wrong.

GetHandleInformation() seems to be a more correct function (and it works on Windows too: https://testbot.winehq.org/JobDetails.pl?Key=143336)

I'll update the MR to use that instead I guess

[Zebediah Figura (＠zfigura)]

> > IsBadReadPtr() is an attempt to actually implement some subset of correct behaviour, and it looks very wrong.
>
> GetHandleInformation() seems to be a more correct function (and it works on Windows too: https://testbot.winehq.org/JobDetails.pl?Key=143336)
>
> I'll update the MR to use that instead I guess

Despite the type, I'd be surprised if a QOS handle is an NT handle. And if it were, you'd want to just forward to CloseHandle() anyway.

[Zebediah Figura (＠zfigura)]

> Despite the type, I'd be surprised if a QOS handle is an NT handle. And if it were, you'd want to just forward to CloseHandle() anyway.

Hmm, I spoke too soon. It looks like it is actually an NT handle:

```
Every process intending to use qWAVE must first call QOSCreateHandle. The handle returned can be used for performing overlapped I/O. For example, this handle can be associated with an I/O completion port (IOCP) to receive overlapped completion notifications. This function can be called multiple times to obtain multiple handles although a single handle is sufficient for most applications.
```

Regardless, I doubt it makes sense to implement QOSCloseHandle() if we're not implementing QOSCreateHandle(). Currently if we forward to CloseHandle() we'll just be closing garbage.