1/5:
``` + swprintf( name, ARRAY_SIZE( name ), L"\Device\WINEBTH-DEVICE-%d", device_index++ ); ```
This probably is thread-safe, but I'd kind of prefer not to have to think about it, and just use AUTOGENERATED_DEVICE_NAME or whatever the flag is. Not to mention there's no reason to give the device a specific name here.
The PDO directly having children is a bit weird, although not impossible. Is this what Windows does?
``` +DEFINE_DEVPROPKEY( DEVPKEY_Bluetooth_DeviceAddress, 0x2bd67d8b, 0x8beb, 0x48d5, 0x87, 0xe0, 0x6c, 0xda, 0x34, 0x28, + 0x04, 0x0a, 1 ); /* DEVPROP_TYPE_STRING */ ``` I'd rather not wrap for GUID definitions. Honestly, if you get rid of the spaces and the comment, the line isn't too long anyway.
2/5:
``` CRITICAL_SECTION props_cs; winebluetooth_device_props_mask_t props_mask; /* Guarded by props_cs */ struct winebluetooth_device_properties props; /* Guarded by props_cs */ + unsigned int started : 1; /* Whether the device has been started. Guarded by props_cs */ unsigned int removed : 1; + + CRITICAL_SECTION le_cs; + unsigned int le : 1; /* Guarded by le_cs */ + UNICODE_STRING bthle_symlink_name; /* Guarded by le_cs */ + struct list gatt_services; /* Guarded by le_cs */ ```
Why the separate CS?
Also, this is a preexisting pattern, but bitfields are kind of pointless if you aren't going to pack them. They're also kind of pointless anyway if you have less than 8 of them—these could be stdbool, and would be more declarative (and kinder to the compiler, not that that matters) while taking up the same amount of space.
``` @@ -59,6 +59,7 @@ SOURCES = \ bitsmsg.h \ bluetoothapis.h \ bthdef.h \ + bthledef.h \ bthioctl.h \ bthsdpdef.h \ cderr.h \ ```
That's not quite in alphabetical order ;-)
3/5:
``` + irp->IoStatus.Information += sizeof( *services ); + if (services->count) + irp->IoStatus.Information -= sizeof( struct winebth_gatt_service ); ```
This logic seems weirdly complex, can we just use offsetof(services, services[count]) instead?
I think it should also be possible to define the struct using a sizeless array. I don't know if you can safely use sizeof() in that case, but I'm not sure we want to use sizeof() anyway, probably better is offsetof(services, services[0]).
4/5:
Any reason not to put this code in main.c? Same for the tests in the next commit.
``` + FIXME( "(%p, %u, %p, %p, %#lx): semi-stub!\n", le_device, count, buf, actual, flags ); ```
What's a semi-stub about it?
Is there a particular reason not to just use BTH_LE_GATT_SERVICE in the driver directly, and move most of this logic there?
``` +typedef struct _BTH_LE_UUID { + BOOLEAN IsShortUuid; + union { + USHORT ShortUuid; + GUID LongUuid; + } Value; +} BTH_LE_UUID, *PBTH_LE_UUID; ```
Usually in new code we put all braces on their own line.
5/5:
``` + ok( devinfo != INVALID_HANDLE_VALUE, "SetupDiGetClassDevsW failed: %lu\n", ret ); + if (devinfo == INVALID_HANDLE_VALUE) + { + skip( "No LE devices found.\n" ); + return; + } ```
This if() is dead code.
``` + if (!SetupDiGetDeviceInterfaceDetailW( devinfo, &iface_data, iface_detail, sizeof( buffer ), NULL, + &devinfo_data )) + continue; ```
When would this happen?
``` + device = CreateFileW( iface_detail->DevicePath, GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL ); + if (device != INVALID_HANDLE_VALUE) ```
When would we fail to open the device?
``` + winetest_push_context( "device %lu", n++ ); + trace( "Address: %s\n", debugstr_w( addr_str ) ); + func( device, data ); + winetest_pop_context(); ```
Why not put the address in the context instead?
``` + if (!found) + { + skip_( __FILE__, line )( "No LE devices found.\n" ); + return; + } ```
This return isn't doing anything.
``` +static const char *debugstr_BTH_LE_UUID( const BTH_LE_UUID *uuid ) +{ + if (!uuid) + return wine_dbg_sprintf( "(null)" ); ```
We control the caller, of which there is only one anyway. We don't need to check for null pointers, and I'd argue against even having a helper function for this.
``` + ok( HRESULT_CODE( ret ) == ERROR_INVALID_HANDLE, "%lu != %d\n", HRESULT_CODE( ret ), ERROR_INVALID_HANDLE ); ```
Why not "ret == HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE)"? That's a more accurate test while being about as much code.
I also personally find it a waste of space to print the expected value when it's a constant...
``` + ret = BluetoothGATTGetServices( NULL, 0, NULL, NULL, 0 ); + ok( ret == E_POINTER, "%lu != %lu\n", ret, E_POINTER ); ```
%#lx, surely.