``` [PATCH v2 1/3] ws_32/tests: Add tests for Bluetooth discovery in WSALookupServiceBegin/Next. ```
Typo in the title here ("ws_32").
``` + for (i = 1; i < 1 << ARRAY_SIZE( device_lookup_flags ); i++) + { + DWORD next_flags = get_nth_lookup_flags( i, device_lookup_flags, ARRAY_SIZE( device_lookup_flags )); ```
For a simpler proposal (and one that will take less time running the tests, although I don't know how much that's a concern since I don't have immediate access to a machine that doesn't skip) how about we just tests the flag one at a time? Or test e.g. (LUP_RETURN_ALL & ~device_lookup_flags[i]).
``` + skip( "WSALookupServiceBeginA failed\n" ); ```
On what platforms does this happen?
``` + winetest_push_context( "flags=%#lx", next_flags ); ```
Why not move the context so that it covers the entire loop?
``` + if (lookup_handle) ```
This if is unnecessary; you already checked it.
``` + ret = WSALookupServiceNextA( lookup_handle, flags, &buf_len, results ); ```
ok(ret == 0 || ret == -1) wouldn't hurt.
``` + todo_wine ok( err == WSA_E_NO_MORE || err == WSAENOMORE || err == WSAEFAULT, + "WSALookupServiceNextA failed: %d\n", WSAGetLastError() ); ```
On which platforms do the different errors appear?
``` + if (flags & LUP_RETURN_NAME) + { + todo_wine ok( !!results->lpszServiceInstanceName, "Expected lpszServiceInstanceName to not be NULL\n" ); ```
Should we check that the name is NULL if that flag isn't returned?
``` + ok( !!info, "Expected pBlobData to not be NULL\n" ); + if (info) ```
I know this is common, but I think this kind of thing is really unnecessary. Either a test succeeds or it doesn't, and if it doesn't succeed it needs to be debugged and fixed. By those metrics crashing isn't meaningfully worse than just failing.
``` + ok( remote_bth_addr->btAddr == info->address, "%llx != %llx\n", remote_bth_addr->btAddr, info->address ); ```
I'm surprised %ll compiles without warnings. In general we use %I64 instead.
From patch 2/3:
``` + __TRY ```
I would leave these out unless you have an application that specifically requires them.
``` + if (query->dwSize == sizeof( *query )) + { ```
``` + switch (query->dwNameSpace) + { + case NS_BTH: + ret = ws_bth_lookup_service_begin( query, flags, &lookup_data.bth ); + break; + default: + FIXME( "Unsupported dwNameSpace: %lu\n", query->dwNameSpace ); + SetLastError( WSA_NOT_ENOUGH_MEMORY ); + ret = -1; + break; + } + if (!ret) ```
``` +union ws_lookup_service_data +{ + struct ws_bth_query_ctx bth; +}; + +struct ws_lookup_service_ctx +{ + DWORD magic; + DWORD namespace; + union ws_lookup_service_data data; +}; ```
I would be inclined leave out the magic until a known application is misbehaving here.
We can define that union inline.
``` +struct ws_bth_query_ctx +{ + DWORD flags; + union { + struct ws_bth_query_device device; + } data; +}; ```
Those flags aren't Bluetooth-specific. Also, what's that union for?
``` + return ret ? ret : WSALookupServiceBeginA( &queryA, flags, lookup ); ```
Forwarding W to A doesn't look right.
``` + assert( search->dwNameSpace == NS_BTH ); ```
That seems a bit excessive...
``` + if (handle->flags & LUP_CONTAINERS) + free( handle->data.device.list ); ```
You already checked for that in ws_bth_lookup_service_begin().
The documentation suggests that LUP_CONTAINERS is just a filter, so I don't imagine it would even need a separate implementation like that. Maybe I'm misled here?
``` +static DWORD bth_for_all_radios( void (*callback)( HANDLE radio, void *data ), void *data ) ```
How many other places are you planning to use this function? The callback design looks kind of excessive for how much it's actually doing...
``` +static ULONG bth_device_info_list_size( ULONG num_devices ) +{ + BTH_DEVICE_INFO_LIST *list; + return sizeof( *list ) + (num_devices - 1) * sizeof( list->deviceList[0] ); +} ```
"offsetof(BTH_DEVICE_INFO_LIST, deviceList[count])", which at that point is simple enough that you could probably write it inline and forgo the helper function.
From patch 3/3:
``` + DWORD buf_left = *buf_len - queryset_size; + DWORD buf_required = queryset_size; + const BTH_DEVICE_INFO *device_info; + BOOL buf_insufficient = FALSE; ```
You've got three variables here which are redundant; you only need the "required" part. "buf_insufficient" is just "*buf_len < buf_required", and "buf_left" is "*buf_len - buf_required".
Does native touch the buffer at all if the size is insufficient? This isn't tested.