Re: [PATCH v2 0/5] MR7630: winebth.sys part 7: Introduce support for authentication/pairing.

[Elizabeth Figura (＠zfigura)]

> From 2/5:

```
The PE driver then broadcasts a GUID_BLUETOOTH_AUTHENTICATION_REQUEST PnP event on the newly
added WINEBTHAUTH device,
```
That should be GUID_WINEBTH_AUTHENTICATION_REQUEST.

We also need to correctly handle IRP_MJ_PNP requests for this new device.
> From 4/5:

```
+    if (!addr)
+        return wine_dbg_sprintf( "%p", addr );
```

Just "(null)" seems more idiomatic.

```
+            work = CreateThreadpoolWork( tp_auth_callback_work_proc, data, NULL );
```
Why the threadpool? Why can't we call the callback right now?

```
+    struct bluetooth_auth_listener *hreg;
```

Bikeshedding, but "hreg" seems an odd variable name; why not "listener"?

```
+static DWORD bluetooth_auth_register( BOOL reg )
+{
+    DWORD ret;
+
+    if (reg)
+    {
+        ...
+    }
+    else
+    {
+        ...
+    }
+
+    return ret;
+}
```

This is what I like to call an "anti-helper". Just make this two separate functions, and probably inline at least the unregister side as it's literally two lines.

```
+        DeviceIoControl( winebth_auth, IOCTL_WINEBTH_AUTH_REGISTER, NULL, 0, NULL, 0, &bytes, NULL );
+        ret = GetLastError();
```

Don't check GetLastError() without also checking the return of DeviceIoControl(). Most functions which update the last error don't update it on success.
> From 5/5:

```
 SOURCES = \
+       auth.c \
        device.c \
        radio.c \
        sdp.c
```

I really should have said something earlier, but we don't need all these different source files with less than 500 lines in each of them, especially if it's going to necessitate an extra header for helpers.

```
+    if (hreg)
+         ok( BluetoothUnregisterAuthentication( hreg ), "BluetoothUnregisterAuthentication failed: %lu\n", GetLastError() );
+    if (hreg2)
+        ok( BluetoothUnregisterAuthentication( hreg2 ), "BluetoothUnregisterAuthentication failed: %lu\n", GetLastError() );
```

You don't need the checks, you already checked those in an earlier ok().

Also, you don't want to test GetLastError() and the call at the same time, there's no sequence point between those. This is the one point where you *do* need to assign a bool to a temporary variable.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7630#note_98631