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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8174#note_105268