2/6:
``` +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_BROADCAST 1 +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_READ (1 << 1) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_WRITE (1 << 2) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_NOTIFY (1 << 3) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_INDICATE (1 << 4) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_WRITE_SIGNED (1 << 5) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_EXTENDED_PROPS (1 << 6) +#define WINEBLUETOOTH_GATT_CHARACTERISTIC_FLAGS_WRITE_WITHOUT_RESPONSE (1 << 7) + +struct winebluetooth_watcher_event_gatt_characteristic_added +{ + winebluetooth_gatt_characteristic_t characteristic; + winebluetooth_gatt_service_t service; + GUID uuid; + UINT32 flags; + UINT16 handle; +}; + ```
This looks like a rewrite of BTH_LE_GATT_CHARACTERISTIC. Why not use that struct directly?
3/6:
``` + CRITICAL_SECTION chars_cs; + struct list characteristics; /* Guarded by chars_cs */ ```
In this patch series this is only ever taken under props_cs. Is there a point to having this separate CS? Even if there will be a case where it can be taken by itself, will it really make a measurable performance difference?
(The same can be asked of the four other mutexes already used in this file. Those at least are already used by themselves, but do we really need all of them?)
``` + irp->IoStatus.Information = offsetof( struct winebth_le_device_get_gatt_characteristics_params, characteristics[chars->count] ); ```
You don't actually use this later, but instead check ->count directly in the caller. Since this is a private ioctl you might as well get rid of everything you're not using.
5/6:
``` +static const char *debugstr_BTH_LE_UUID( const BTH_LE_UUID *uuid ) +{ + if (!uuid) + return wine_dbg_sprintf( "(null)" ); + if (uuid->IsShortUuid) + return wine_dbg_sprintf("{ IsShortUuid=1 {%#x} }", uuid->Value.ShortUuid ); + return wine_dbg_sprintf( "{ IsShortUuid=0 %s }", debugstr_guid( &uuid->Value.LongUuid ) ); +} + ```
You're never passing a NULL value to this function. Same with debugstr_BTH_LE_GATT_CHARACTERISTIC() in 6/6.
``` + if (!actual) + return E_POINTER; + + if ((buf && !count) || !actual) + return E_INVALIDARG; ```
That doesn't look right. Also, the spacing is a bit off.
``` + for (;;) ```
Why the loop? The API function only asks for the first N and the total count; you can just allocate a buffer large enough to hold the first N and then return it, can't you?
Do we want a FIXME for the flags?
6/6:
``` + ok( ret == S_OK, "BluetoothGATTGetCharacteristics failed: %#lx\n", ret ); + if (FAILED( ret )) + { + skip( "BluetoothGATTGetCharacteristics failed.\n" ); + free( buf ); + return; + } ```
I always feel that this sort of thing is unnecessary. If a test failed, it needs to be fixed, no matter if there's one failure, many failures, or a crash.