[PATCH v15 0/6] MR6402: bluetoothapis: Implement basic SDP methods.
The MR adds implementations (+ a few tests) for the following methods in `bluetoothapis.h`: * `BluetoothSdpEnumAttributes` * `BluetoothSdpGetContainerElementData` * `BluetoothSdpGetElementData` * `BluetoothSdpGetAttributeValue` -- v15: bluetoothapis/tests: Add tests for BluetoothSdpEnumAttributes and BluetoothSdpGetAttributeValue. bluetoothapis: Implement BluetoothSdpGetAttributeValue. bluetoothapis/tests: Add unit tests for BluetoothSdpGetContainerElementData and BluetoothSdpGetElementData. bluetoothapis: Implement BluetoothSdpEnumAttributes. bluetoothapis: Implement BluetoothSdpGetContainerElementData. bluetoothapis: Implement BluetoothSdpGetElementData. https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/bluetoothapis/Makefile.in | 3 +- dlls/bluetoothapis/bluetoothapis.spec | 2 +- dlls/bluetoothapis/sdp.c | 265 ++++++++++++++++++++++++++ dlls/bthprops.cpl/bthprops.cpl.spec | 2 +- dlls/irprops.cpl/irprops.cpl.spec | 2 +- 5 files changed, 270 insertions(+), 4 deletions(-) create mode 100644 dlls/bluetoothapis/sdp.c diff --git a/dlls/bluetoothapis/Makefile.in b/dlls/bluetoothapis/Makefile.in index 78518822519..1fd74074703 100644 --- a/dlls/bluetoothapis/Makefile.in +++ b/dlls/bluetoothapis/Makefile.in @@ -4,4 +4,5 @@ IMPORTLIB = bluetoothapis EXTRADLLFLAGS = -Wb,--prefer-native SOURCES = \ - main.c + main.c \ + sdp.c diff --git a/dlls/bluetoothapis/bluetoothapis.spec b/dlls/bluetoothapis/bluetoothapis.spec index 625dffb8254..a35e1e22ecc 100644 --- a/dlls/bluetoothapis/bluetoothapis.spec +++ b/dlls/bluetoothapis/bluetoothapis.spec @@ -56,7 +56,7 @@ @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue @ stub BluetoothSdpGetContainerElementData -@ stub BluetoothSdpGetElementData +@ stdcall BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSendAuthenticationResponse @ stub BluetoothSendAuthenticationResponseEx diff --git a/dlls/bluetoothapis/sdp.c b/dlls/bluetoothapis/sdp.c new file mode 100644 index 00000000000..550de3c7f44 --- /dev/null +++ b/dlls/bluetoothapis/sdp.c @@ -0,0 +1,265 @@ +/* + * SDP APIs + * + * Copyright 2024 Vibhav Pant + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + * + */ + +#include <stdarg.h> +#include <windef.h> +#include <winbase.h> +#include <winternl.h> + +#include "wine/debug.h" +#include "bthsdpdef.h" +#include "bluetoothapis.h" + +WINE_DEFAULT_DEBUG_CHANNEL( bluetoothapis ); + +#ifdef WORDS_BIGENDIAN +#define BTH_READ_UINT16( s ) (s) +#define BTH_READ_UINT32( s ) (s) +#define BTH_READ_UINT64( s ) (s) +#else +#define BTH_READ_UINT16( s ) RtlUshortByteSwap( *(USHORT *)(s) ) +#define BTH_READ_UINT32( s ) RtlUlongByteSwap( *(ULONG *)(s) ) +#define BTH_READ_UINT64( s ) RtlUlonglongByteSwap( *(ULONGLONG *)(s) ) +#endif + +#define BTH_READ_UINT128( s, v ) \ + do \ + { \ + (v)->HighPart = BTH_READ_UINT64( (s) ); \ + (v)->LowPart = BTH_READ_UINT64( (s + 8) ); \ + } while (0) + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +static inline BYTE data_elem_type( BYTE elem ) { return ( elem & 0b11111000 ) >> 3; } +static inline BYTE data_elem_size_desc( BYTE elem ) { return elem & 0b00000111; } + +#define SDP_ELEMENT_IS_UINT16( d ) ( (d)->type == SDP_TYPE_UINT && (d)->specificType == SDP_ST_UINT16 ) +#define SDP_ELEMENT_IS_ATTRID( d ) SDP_ELEMENT_IS_UINT16((d)) + +/* Read the data element's size/length as described by the size descriptor, starting from stream. Only + * valid for SDP_SIZEDESC_NEXT_* types. */ +static BOOL sdp_elem_read_var_size( BYTE *stream, ULONG stream_size, SIZE_T *read, BYTE size_desc, + UINT32 *size ) +{ + switch(size_desc) + { + case SDP_SIZEDESC_NEXT_UINT8: + if (stream_size < sizeof( UINT8 )) return FALSE; + *size = (UINT8)(*stream); + *read += sizeof( UINT8 ); + return TRUE; + case SDP_SIZEDESC_NEXT_UINT16: + if (stream_size < sizeof( UINT16 )) return FALSE; + *size = BTH_READ_UINT16( stream ); + *read += sizeof( UINT16 ); + return TRUE; + case SDP_SIZEDESC_NEXT_UINT32: + if ( stream_size < sizeof( UINT32 ) ) return FALSE; + *size = BTH_READ_UINT32( stream ); + *read += sizeof( UINT32 ); + return TRUE; + default: + return FALSE; + } +} + +const static SDP_SPECIFICTYPE SDP_BASIC_TYPES[4][5] = { + [SDP_TYPE_UINT] = + { + [SDP_SIZEDESC_1_BYTE] = SDP_ST_UINT8, + [SDP_SIZEDESC_2_BYTES] = SDP_ST_UINT16, + [SDP_SIZEDESC_4_BYTES] = SDP_ST_UINT32, + [SDP_SIZEDESC_8_BYTES] = SDP_ST_UINT64, + [SDP_SIZEDESC_16_BYTES] = SDP_ST_UINT128, + }, + [SDP_TYPE_INT] = + { + [SDP_SIZEDESC_1_BYTE] = SDP_ST_INT8, + [SDP_SIZEDESC_2_BYTES] = SDP_ST_INT16, + [SDP_SIZEDESC_4_BYTES] = SDP_ST_INT32, + [SDP_SIZEDESC_8_BYTES] = SDP_ST_INT64, + [SDP_SIZEDESC_16_BYTES] = SDP_ST_INT128, + }, + [SDP_TYPE_UUID] = + { + [SDP_SIZEDESC_2_BYTES] = SDP_ST_UUID16, + [SDP_SIZEDESC_4_BYTES] = SDP_ST_UUID32, + [SDP_SIZEDESC_16_BYTES] = SDP_ST_UUID128, + }, +}; + +static BOOL sdp_read_specific_type( BYTE *stream, ULONG stream_size, SDP_SPECIFICTYPE st, + SDP_ELEMENT_DATA *data, SIZE_T *read ) +{ + switch (st) + { + case SDP_ST_UINT8: + case SDP_ST_INT8: + if (stream_size < sizeof( UINT8 )) return FALSE; + data->data.uint8 = *stream; + *read += sizeof( UINT8 ); + break; + case SDP_ST_UINT16: + case SDP_ST_INT16: + case SDP_ST_UUID16: + if (stream_size < sizeof( UINT16 )) return FALSE; + data->data.uint16 = BTH_READ_UINT16( stream ); + *read += sizeof( UINT16 ); + break; + case SDP_ST_UINT32: + case SDP_ST_INT32: + if (stream_size < sizeof( UINT32 )) return FALSE; + data->data.uint32 = BTH_READ_UINT32( stream ); + *read += sizeof( UINT32 ); + break; + case SDP_ST_UINT64: + case SDP_ST_INT64: + if (stream_size < sizeof( UINT64 )) return FALSE; + data->data.uint64 = BTH_READ_UINT64( stream ); + *read += sizeof( UINT64 ); + break; + case SDP_ST_UINT128: + case SDP_ST_INT128: + case SDP_ST_UUID128: + if (stream_size < sizeof( SDP_ULARGE_INTEGER_16 )) return FALSE; + BTH_READ_UINT128( stream, &data->data.uint128 ); + *read += sizeof( SDP_ULARGE_INTEGER_16 ); + break; + default: + return FALSE; + } + + return TRUE; +} + +static DWORD sdp_read_element_data( BYTE *stream, ULONG stream_size, SDP_ELEMENT_DATA *data, + SIZE_T *read ) +{ + BYTE type, size_desc, elem; + SDP_SPECIFICTYPE st; + + if (stream_size < sizeof( BYTE )) return ERROR_INVALID_PARAMETER; + + elem = *stream; + type = data_elem_type( elem ); + size_desc = data_elem_size_desc( elem ); + + stream += sizeof( BYTE ); + *read += sizeof( BYTE ); + stream_size -= sizeof( BYTE ); + + memset( data, 0, sizeof( *data ) ); + switch (type) + { + case SDP_TYPE_NIL: + if (size_desc != 0) return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = SDP_ST_NONE; + break; + case SDP_TYPE_UINT: + case SDP_TYPE_INT: + case SDP_TYPE_UUID: + if (size_desc > SDP_SIZEDESC_16_BYTES) return ERROR_INVALID_PARAMETER; + + st = SDP_BASIC_TYPES[type][size_desc]; + if (st == SDP_ST_NONE) return ERROR_INVALID_PARAMETER; + + if (!sdp_read_specific_type( stream, stream_size, st, data, read )) + return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = st; + break; + case SDP_TYPE_BOOLEAN: + if (size_desc != SDP_SIZEDESC_1_BYTE) return ERROR_INVALID_PARAMETER; + if (stream_size < sizeof(BYTE)) return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = SDP_ST_NONE; + data->data.booleanVal = *stream; + *read += sizeof( BYTE ); + break; + case SDP_TYPE_STRING: + case SDP_TYPE_URL: + case SDP_TYPE_SEQUENCE: + case SDP_TYPE_ALTERNATIVE: + { + UINT32 elems_size; + SIZE_T size_read = 0; + + if (!(size_desc >= SDP_SIZEDESC_NEXT_UINT8 && size_desc <= SDP_SIZEDESC_NEXT_UINT32)) + return ERROR_INVALID_PARAMETER; + if (!sdp_elem_read_var_size( stream, stream_size, &size_read, size_desc, &elems_size )) + return ERROR_INVALID_PARAMETER; + + stream_size -= size_read; + if (type == SDP_TYPE_STRING || type == SDP_TYPE_URL) + stream += size_read; + + if (stream_size < elems_size) return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = SDP_ST_NONE; + if (type == SDP_TYPE_STRING || type == SDP_TYPE_URL) + { + data->data.string.value = stream; + data->data.string.length = elems_size; + } + else + { + /* For sequence and alternative containers, the stream should begin at the container + * header. */ + data->data.sequence.value = stream - sizeof( BYTE ); + data->data.sequence.length = elems_size + *read + size_read; + } + *read += size_read + elems_size; + break; + } + default: + return ERROR_INVALID_PARAMETER; + } + + return ERROR_SUCCESS; +} + +/********************************************************************* + * BluetoothSdpGetElementData + */ +DWORD WINAPI BluetoothSdpGetElementData( BYTE *stream, ULONG stream_size, SDP_ELEMENT_DATA *data ) +{ + SIZE_T read = 0; + + TRACE( "(%p, %lu, %p)\n", stream, stream_size, data ); + + if (stream == NULL || stream_size < sizeof( BYTE ) || data == NULL) + return ERROR_INVALID_PARAMETER; + + return sdp_read_element_data( stream, stream_size, data, &read ); +} diff --git a/dlls/bthprops.cpl/bthprops.cpl.spec b/dlls/bthprops.cpl/bthprops.cpl.spec index 1698344581e..0bef83810db 100644 --- a/dlls/bthprops.cpl/bthprops.cpl.spec +++ b/dlls/bthprops.cpl/bthprops.cpl.spec @@ -47,7 +47,7 @@ @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue @ stub BluetoothSdpGetContainerElementData -@ stub BluetoothSdpGetElementData +@ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSelectDevices @ stub BluetoothSelectDevicesFree diff --git a/dlls/irprops.cpl/irprops.cpl.spec b/dlls/irprops.cpl/irprops.cpl.spec index b2bdf3c3b2a..c9cd4df62d6 100644 --- a/dlls/irprops.cpl/irprops.cpl.spec +++ b/dlls/irprops.cpl/irprops.cpl.spec @@ -41,7 +41,7 @@ @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue @ stub BluetoothSdpGetContainerElementData -@ stub BluetoothSdpGetElementData +@ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSelectDevices @ stub BluetoothSelectDevicesFree -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/bluetoothapis/bluetoothapis.spec | 2 +- dlls/bluetoothapis/sdp.c | 59 +++++++++++++++++++++++++++ dlls/bthprops.cpl/bthprops.cpl.spec | 2 +- dlls/irprops.cpl/irprops.cpl.spec | 2 +- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/dlls/bluetoothapis/bluetoothapis.spec b/dlls/bluetoothapis/bluetoothapis.spec index a35e1e22ecc..51fdf55d8ed 100644 --- a/dlls/bluetoothapis/bluetoothapis.spec +++ b/dlls/bluetoothapis/bluetoothapis.spec @@ -55,7 +55,7 @@ @ stub BluetoothRemoveDevice @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue -@ stub BluetoothSdpGetContainerElementData +@ stdcall BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSendAuthenticationResponse diff --git a/dlls/bluetoothapis/sdp.c b/dlls/bluetoothapis/sdp.c index 550de3c7f44..e15da99d8ef 100644 --- a/dlls/bluetoothapis/sdp.c +++ b/dlls/bluetoothapis/sdp.c @@ -263,3 +263,62 @@ DWORD WINAPI BluetoothSdpGetElementData( BYTE *stream, ULONG stream_size, SDP_EL return sdp_read_element_data( stream, stream_size, data, &read ); } + +/********************************************************************* + * BluetoothSdpGetContainerElementData + */ +DWORD WINAPI BluetoothSdpGetContainerElementData( BYTE *stream, ULONG stream_size, + HBLUETOOTH_CONTAINER_ELEMENT *handle, + SDP_ELEMENT_DATA *data ) +{ + BYTE *cursor; + DWORD result; + SIZE_T read = 0; + + TRACE( "(%p, %lu, %p, %p)\n", stream, stream_size, handle, data ); + + if (stream == NULL || stream_size < sizeof( BYTE ) || handle == NULL || data == NULL) + return ERROR_INVALID_PARAMETER; + + cursor = (BYTE *)(*handle); + + if (cursor == NULL) + { + BYTE header; + BYTE type, size_desc; + UINT32 elems_size = 0; + SIZE_T read = 0; + + header = *stream; + type = data_elem_type( header ); + size_desc = data_elem_size_desc( header ); + + if (type != SDP_TYPE_SEQUENCE && type != SDP_TYPE_ALTERNATIVE) + return ERROR_INVALID_PARAMETER; + if (!( size_desc >= SDP_SIZEDESC_NEXT_UINT8 && size_desc <= SDP_SIZEDESC_NEXT_UINT32 )) + return ERROR_INVALID_PARAMETER; + + stream++; + if (!sdp_elem_read_var_size( stream, stream_size, &read, size_desc, &elems_size )) + return ERROR_INVALID_PARAMETER; + + stream += read; + stream_size -= read; + } + else + { + if (cursor < stream) return ERROR_INVALID_PARAMETER; + if (cursor == (stream + stream_size)) return ERROR_NO_MORE_ITEMS; + + stream = cursor; + stream_size = stream_size - (ptrdiff_t)(cursor - stream); + } + result = sdp_read_element_data( stream, stream_size, data, &read ); + if (result != ERROR_SUCCESS) return result; + + stream += read; + TRACE("handle=%p\n", stream); + *handle = stream; + return ERROR_SUCCESS; +} + diff --git a/dlls/bthprops.cpl/bthprops.cpl.spec b/dlls/bthprops.cpl/bthprops.cpl.spec index 0bef83810db..cd17f0f108c 100644 --- a/dlls/bthprops.cpl/bthprops.cpl.spec +++ b/dlls/bthprops.cpl/bthprops.cpl.spec @@ -46,7 +46,7 @@ @ stub BluetoothRemoveDevice @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue -@ stub BluetoothSdpGetContainerElementData +@ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSelectDevices diff --git a/dlls/irprops.cpl/irprops.cpl.spec b/dlls/irprops.cpl/irprops.cpl.spec index c9cd4df62d6..847b1552603 100644 --- a/dlls/irprops.cpl/irprops.cpl.spec +++ b/dlls/irprops.cpl/irprops.cpl.spec @@ -40,7 +40,7 @@ @ stub BluetoothRemoveDevice @ stub BluetoothSdpEnumAttributes @ stub BluetoothSdpGetAttributeValue -@ stub BluetoothSdpGetContainerElementData +@ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString @ stub BluetoothSelectDevices -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/bluetoothapis/bluetoothapis.spec | 2 +- dlls/bluetoothapis/sdp.c | 64 +++++++++++++++++++++++++++ dlls/bthprops.cpl/bthprops.cpl.spec | 2 +- dlls/irprops.cpl/irprops.cpl.spec | 2 +- 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/dlls/bluetoothapis/bluetoothapis.spec b/dlls/bluetoothapis/bluetoothapis.spec index 51fdf55d8ed..3531e2f901c 100644 --- a/dlls/bluetoothapis/bluetoothapis.spec +++ b/dlls/bluetoothapis/bluetoothapis.spec @@ -53,7 +53,7 @@ @ stub BluetoothRegisterForAuthentication @ stdcall BluetoothRegisterForAuthenticationEx(ptr ptr ptr ptr) @ stub BluetoothRemoveDevice -@ stub BluetoothSdpEnumAttributes +@ stdcall BluetoothSdpEnumAttributes(ptr long ptr ptr) @ stub BluetoothSdpGetAttributeValue @ stdcall BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall BluetoothSdpGetElementData(ptr long ptr) diff --git a/dlls/bluetoothapis/sdp.c b/dlls/bluetoothapis/sdp.c index e15da99d8ef..b09daf01c8d 100644 --- a/dlls/bluetoothapis/sdp.c +++ b/dlls/bluetoothapis/sdp.c @@ -322,3 +322,67 @@ DWORD WINAPI BluetoothSdpGetContainerElementData( BYTE *stream, ULONG stream_siz return ERROR_SUCCESS; } +/********************************************************************* + * BluetoothSdpEnumAttributes + */ +BOOL WINAPI BluetoothSdpEnumAttributes( BYTE *stream, ULONG stream_size, + PFN_BLUETOOTH_ENUM_ATTRIBUTES_CALLBACK callback, void *param ) +{ + SDP_ELEMENT_DATA data = {0}; + DWORD result; + HBLUETOOTH_CONTAINER_ELEMENT cursor = NULL; + + TRACE( "(%p, %ld, %p, %p)\n", stream, stream_size, callback, param ); + + if (stream == NULL || callback == NULL) return ERROR_INVALID_PARAMETER; + + result = BluetoothSdpGetElementData( stream, stream_size, &data ); + if (result != ERROR_SUCCESS) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + switch (data.type) + { + case SDP_TYPE_SEQUENCE: + case SDP_TYPE_ALTERNATIVE: + break; + default: + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + while (TRUE) + { + SDP_ELEMENT_DATA attrid = {0}; + SDP_ELEMENT_DATA attr = {0}; + BYTE *raw_attr_stream; + + result = BluetoothSdpGetContainerElementData( + data.data.sequence.value, data.data.sequence.length, &cursor, &attrid ); + if (result == ERROR_NO_MORE_ITEMS) return TRUE; + if (result != ERROR_SUCCESS) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + if (!SDP_ELEMENT_IS_ATTRID( &attrid )) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + raw_attr_stream = cursor; + result = BluetoothSdpGetContainerElementData( data.data.sequence.value, + data.data.sequence.length, &cursor, &attr ); + if (result != ERROR_SUCCESS) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + if (!callback( attrid.data.uint16, raw_attr_stream, ((BYTE *)cursor - raw_attr_stream), + param )) + return TRUE; + } +} diff --git a/dlls/bthprops.cpl/bthprops.cpl.spec b/dlls/bthprops.cpl/bthprops.cpl.spec index cd17f0f108c..8e4d2e02a42 100644 --- a/dlls/bthprops.cpl/bthprops.cpl.spec +++ b/dlls/bthprops.cpl/bthprops.cpl.spec @@ -44,7 +44,7 @@ @ stub BluetoothRegisterForAuthentication @ stdcall -import BluetoothRegisterForAuthenticationEx(ptr ptr ptr ptr) @ stub BluetoothRemoveDevice -@ stub BluetoothSdpEnumAttributes +@ stdcall -import BluetoothSdpEnumAttributes(ptr long ptr ptr) @ stub BluetoothSdpGetAttributeValue @ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) diff --git a/dlls/irprops.cpl/irprops.cpl.spec b/dlls/irprops.cpl/irprops.cpl.spec index 847b1552603..a4cc145dc8f 100644 --- a/dlls/irprops.cpl/irprops.cpl.spec +++ b/dlls/irprops.cpl/irprops.cpl.spec @@ -38,7 +38,7 @@ @ stub BluetoothMapClassOfDeviceToString @ stub BluetoothRegisterForAuthentication @ stub BluetoothRemoveDevice -@ stub BluetoothSdpEnumAttributes +@ stdcall -import BluetoothSdpEnumAttributes(ptr long ptr ptr) @ stub BluetoothSdpGetAttributeValue @ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- configure.ac | 1 + dlls/bluetoothapis/tests/Makefile.in | 5 + dlls/bluetoothapis/tests/sdp.c | 516 +++++++++++++++++++++++++++ 3 files changed, 522 insertions(+) create mode 100644 dlls/bluetoothapis/tests/Makefile.in create mode 100644 dlls/bluetoothapis/tests/sdp.c diff --git a/configure.ac b/configure.ac index d729fb56b89..c0a7d364f86 100644 --- a/configure.ac +++ b/configure.ac @@ -2415,6 +2415,7 @@ WINE_CONFIG_MAKEFILE(dlls/bcrypt) WINE_CONFIG_MAKEFILE(dlls/bcrypt/tests) WINE_CONFIG_MAKEFILE(dlls/bcryptprimitives) WINE_CONFIG_MAKEFILE(dlls/bluetoothapis) +WINE_CONFIG_MAKEFILE(dlls/bluetoothapis/tests) WINE_CONFIG_MAKEFILE(dlls/browseui) WINE_CONFIG_MAKEFILE(dlls/browseui/tests) WINE_CONFIG_MAKEFILE(dlls/bthprops.cpl) diff --git a/dlls/bluetoothapis/tests/Makefile.in b/dlls/bluetoothapis/tests/Makefile.in new file mode 100644 index 00000000000..c2e8bc0377a --- /dev/null +++ b/dlls/bluetoothapis/tests/Makefile.in @@ -0,0 +1,5 @@ +TESTDLL = bluetoothapis.dll +IMPORTS = bluetoothapis + +SOURCES = \ + sdp.c diff --git a/dlls/bluetoothapis/tests/sdp.c b/dlls/bluetoothapis/tests/sdp.c new file mode 100644 index 00000000000..ea223aeaa40 --- /dev/null +++ b/dlls/bluetoothapis/tests/sdp.c @@ -0,0 +1,516 @@ +/* Tests for bluetoothapis.dll's SDP API + * + * Copyright 2024 Vibhav Pant + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + * + */ + +#include <stdarg.h> +#include <windef.h> +#include <winbase.h> + +#include "bthsdpdef.h" +#include "bluetoothapis.h" + +#include "wine/test.h" + +#define XX(v) case (v): return #v + +static inline const char *debugstr_SDP_TYPE( SDP_TYPE t ) +{ + switch (t) + { + XX( SDP_TYPE_NIL ); + XX( SDP_TYPE_UINT ); + XX( SDP_TYPE_INT ); + XX( SDP_TYPE_UUID ); + XX( SDP_TYPE_STRING ); + XX( SDP_TYPE_BOOLEAN ); + XX( SDP_TYPE_SEQUENCE ); + XX( SDP_TYPE_ALTERNATIVE ); + XX( SDP_TYPE_URL ); + XX( SDP_TYPE_CONTAINER ); + default: + return wine_dbg_sprintf( "(unknown %#x)", t ); + } +} + +static inline const char *debugstr_SDP_SPECIFIC_TYPE( SDP_SPECIFICTYPE st ) +{ + switch (st) + { + XX( SDP_ST_NONE ); + XX( SDP_ST_UINT8 ); + XX( SDP_ST_UINT16 ); + XX( SDP_ST_UINT32 ); + XX( SDP_ST_UINT64 ); + XX( SDP_ST_UINT128 ); + XX( SDP_ST_INT8 ); + XX( SDP_ST_INT16 ); + XX( SDP_ST_INT32 ); + XX( SDP_ST_INT64 ); + XX( SDP_ST_INT128 ); + XX( SDP_ST_UUID16 ); + XX( SDP_ST_UUID128 ); + default: + return wine_dbg_sprintf( "(unknown %#x)", st ); + } +} + +#undef XX + +static inline const char *debugstr_sdp_element_data_unknown( const SDP_ELEMENT_DATA *data ) +{ + return wine_dbg_sprintf( "{%s %s %s}", debugstr_SDP_TYPE( data->type ), + debugstr_SDP_SPECIFIC_TYPE( data->specificType ), + debugstr_an( (const char *)&data->data, sizeof( data->data ) ) ); +} + +static const char *debugstr_SDP_ELEMENT_DATA( const SDP_ELEMENT_DATA *data ) +{ + switch (data->type) + { + case SDP_TYPE_UINT: + { + switch (data->specificType) + { + case SDP_ST_UINT8: + return wine_dbg_sprintf( "{.uint8=%u}", data->data.uint8 ); + case SDP_ST_UINT16: + return wine_dbg_sprintf( "{.uint16=%u}", data->data.uint16 ); + case SDP_ST_UINT32: + return wine_dbg_sprintf( "{.uint32=%lu}", data->data.uint32 ); + case SDP_ST_UINT64: + return wine_dbg_sprintf( "{.uint64=%llu}", data->data.uint64 ); + case SDP_ST_UINT128: + return wine_dbg_sprintf( "{.uint128={%llu %llu}}", data->data.uint128.LowPart, + data->data.uint128.HighPart ); + default: + return debugstr_sdp_element_data_unknown( data ); + } + } + case SDP_TYPE_INT: + switch (data->specificType) + { + case SDP_ST_INT8: + return wine_dbg_sprintf( "{.int8=%d}", data->data.int8 ); + case SDP_ST_INT16: + return wine_dbg_sprintf( "{.int16=%d}", data->data.int16 ); + case SDP_ST_INT32: + return wine_dbg_sprintf( "{.int32=%ld}", data->data.int32 ); + case SDP_ST_INT64: + return wine_dbg_sprintf( "{.int64=%lld}", data->data.int64 ); + case SDP_ST_INT128: + return wine_dbg_sprintf( "{.int128={%lld %lld}}", + data->data.int128.LowPart, + data->data.int128.HighPart ); + default: + return debugstr_sdp_element_data_unknown( data ); + } + case SDP_TYPE_UUID: + switch (data->specificType) + { + case SDP_ST_UUID16: + return wine_dbg_sprintf( "{.uuid16=%#02x}", data->data.uuid16 ); + case SDP_ST_UUID32: + return wine_dbg_sprintf( "{.uuid32=%#04lx}", data->data.uuid32 ); + case SDP_ST_UUID128: + return wine_dbg_sprintf( "{.uuid128=%s}", + debugstr_guid( &data->data.uuid128 ) ); + default: + return debugstr_sdp_element_data_unknown( data ); + } + case SDP_TYPE_BOOLEAN: + if (data->specificType == SDP_ST_NONE) + return wine_dbg_sprintf( "{.bool=%u}", data->data.booleanVal ); + else + return debugstr_sdp_element_data_unknown( data ); + case SDP_TYPE_STRING: + return wine_dbg_sprintf( "{.string={%p %lu}}", data->data.string.value, + data->data.string.length ); + case SDP_TYPE_URL: + return wine_dbg_sprintf( "{.url={%p %lu}}", data->data.url.value, + data->data.url.length ); + case SDP_TYPE_SEQUENCE: + return wine_dbg_sprintf( "{.sequence={%p %lu}}", data->data.sequence.value, + data->data.sequence.length ); + case SDP_TYPE_ALTERNATIVE: + return wine_dbg_sprintf( "{.alternative={%p %lu}}", data->data.alternative.value, + data->data.alternative.length ); + default: + return debugstr_sdp_element_data_unknown( data ); + } +} + +static void test_BluetoothSdpGetElementData( BYTE *stream, SIZE_T size, DWORD error, + const SDP_ELEMENT_DATA *sdp_data ) +{ + DWORD ret; + SDP_ELEMENT_DATA result = {0}; + + ret = BluetoothSdpGetElementData( stream, size, &result ); + ok( ret == error, "Expected BluetoothSdpGetElementData to return %ld, got %ld.\n", error, ret ); + if (error == ERROR_SUCCESS) + { + ok( result.type == sdp_data->type, "Expected SDP_TYPE %s, got %s.\n", + debugstr_SDP_TYPE( sdp_data->type ), debugstr_SDP_TYPE( result.type ) ); + ok( result.specificType == sdp_data->specificType, + "Expected SDP_SPECIFIC_TYPE %s, got %s.\n", + debugstr_SDP_SPECIFIC_TYPE( sdp_data->specificType ), + debugstr_SDP_SPECIFIC_TYPE( result.specificType ) ); + ok( !memcmp( &sdp_data->data, &result.data, sizeof( result.data ) ), "Expected %s, got %s.\n", + debugstr_SDP_ELEMENT_DATA( sdp_data ), debugstr_SDP_ELEMENT_DATA( &result ) ); + + } +} + +#define TEST_CASE_NAME( n ) ("%s %d"), __func__, (int)(n) + +static void test_BluetoothSdpGetElementData_invalid( void ) +{ + SDP_ELEMENT_DATA data; + BYTE stream[] = {0b00000000}; + DWORD ret; + + ret = BluetoothSdpGetElementData( NULL, 10, &data ); + ok( ret == ERROR_INVALID_PARAMETER, + "Expected BluetoothSdpGetElementData to return %d, got %ld.\n", ERROR_INVALID_PARAMETER, + ret ); + + ret = BluetoothSdpGetElementData( stream, 1, NULL ); + ok( ret == ERROR_INVALID_PARAMETER, + "Expected BluetoothSdpGetElementData to return %d, got %ld.\n", ERROR_INVALID_PARAMETER, + ret ); + + ret = BluetoothSdpGetElementData( stream, 0, &data ); + ok( ret == ERROR_INVALID_PARAMETER, + "Expected BluetoothSdpGetElementData to return %d, got %ld.\n", ERROR_INVALID_PARAMETER, + ret ); + + ret = BluetoothSdpGetElementData( NULL, 0, NULL ); + ok( ret == ERROR_INVALID_PARAMETER, + "Expected BluetoothSdpGetElementData to return %d, got %ld.\n", ERROR_INVALID_PARAMETER, + ret ); +} + +static void test_BluetoothSdpGetElementData_nil( void ) +{ + static struct + { + BYTE data_elem; + DWORD error; + SDP_ELEMENT_DATA data; + } test_cases[] = { + {0b00000000, ERROR_SUCCESS, {.type = SDP_TYPE_NIL, .specificType = SDP_ST_NONE }}, + {0b00000001, ERROR_INVALID_PARAMETER}, + {0b00000011, ERROR_INVALID_PARAMETER}, + {0b00000100, ERROR_INVALID_PARAMETER}, + }; + SIZE_T i; + + for (i = 0; i < ARRAY_SIZE( test_cases ); i++) + { + winetest_push_context(TEST_CASE_NAME( i+1 )); + test_BluetoothSdpGetElementData( &test_cases[i].data_elem, 1, test_cases[i].error, + &test_cases[i].data ); + winetest_pop_context(); + } +} + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +#define SDP_DATA_ELEM_TYPE_DESC(t,s) ((t) << 3 | SDP_SIZEDESC_##s) + +#define SDP_DEF_TYPE(n, t, s) const static BYTE SDP_TYPEDISC_##n = SDP_DATA_ELEM_TYPE_DESC(SDP_TYPE_##t, s) +#define SDP_DEF_INTEGRAL( w, s ) \ + SDP_DEF_TYPE( INT##w, INT, s ); \ + SDP_DEF_TYPE( UINT##w, UINT, s); + +SDP_DEF_INTEGRAL( 8, 1_BYTE ); +SDP_DEF_INTEGRAL( 16, 2_BYTES ); +SDP_DEF_INTEGRAL( 32, 4_BYTES ); +SDP_DEF_INTEGRAL( 64, 8_BYTES ); +SDP_DEF_INTEGRAL( 128, 16_BYTES ); + +SDP_DEF_TYPE( STR8, STRING, NEXT_UINT8 ); +SDP_DEF_TYPE( STR16, STRING, NEXT_UINT16 ); +SDP_DEF_TYPE( STR32, STRING, NEXT_UINT32 ); + +SDP_DEF_TYPE( SEQ8, SEQUENCE, NEXT_UINT8 ); +SDP_DEF_TYPE( SEQ16, SEQUENCE, NEXT_UINT16 ); +SDP_DEF_TYPE( SEQ32, SEQUENCE, NEXT_UINT32 ); + +static void test_BluetoothSdpGetElementData_ints( void ) +{ + static struct + { + BYTE data_elem[17]; + SIZE_T size; + DWORD error; + SDP_ELEMENT_DATA data; + } test_cases[] = { + { + {SDP_TYPEDISC_INT8, 0xde}, + 2, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT8, {.int8 = 0xde}} + }, + { + {SDP_TYPEDISC_UINT8, 0xde}, + 2, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xde}}, + }, + { + {SDP_TYPEDISC_INT16, 0xde, 0xad}, + 3, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT16, {.int16 = 0xdead}}, + }, + { + {SDP_TYPEDISC_UINT16, 0xde, 0xad }, + 3, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0xdead}}, + }, + { + {SDP_TYPEDISC_INT32, 0xde, 0xad, 0xbe, 0xef}, + 5, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT32, {.int32 = 0xdeadbeef}}, + }, + { + {SDP_TYPEDISC_UINT32, 0xde, 0xad, 0xbe, 0xef}, + 5, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT32, {.uint32 = 0xdeadbeef}}, + }, + { + {SDP_TYPEDISC_INT64, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT64, {.int64 = 0xdeadbeefdeadbeef}}, + }, + { + {SDP_TYPEDISC_UINT64, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT64, {.uint64 = 0xdeadbeefdeadbeef}}, + }, + { + {SDP_TYPEDISC_INT64, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT64, {.int64 = 0xdeadbeefdeadbeef}}, + }, + { + {SDP_TYPEDISC_UINT64, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT64, {.uint64 = 0xdeadbeefdeadbeef}}, + }, + { + {SDP_TYPEDISC_INT128, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef }, + 17, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT128, {.int128 = {0xdeadbeefdeadbeef, 0xdeadbeefdeadbeef}}}, + }, + { + {SDP_TYPEDISC_UINT128, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 17, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT128, {.uint128 = {0xdeadbeefdeadbeef, 0xdeadbeefdeadbeef}}}, + } + }; + SIZE_T i; + + for (i = 0; i < ARRAY_SIZE( test_cases ); i++) + { + winetest_push_context( TEST_CASE_NAME( i+1 ) ); + test_BluetoothSdpGetElementData( test_cases[i].data_elem, test_cases[i].size, + test_cases[i].error, &test_cases[i].data ); + winetest_pop_context(); + } +} + +static void test_BluetoothSdpGetElementData_str( void ) +{ + static struct { + BYTE stream[11]; + SIZE_T size; + DWORD error; + SDP_ELEMENT_DATA data; + const char *string; + } test_cases[] = { + { + {SDP_TYPEDISC_STR8, 0x06, 'f', 'o', 'o', 'b', 'a', 'r'}, + 8, + ERROR_SUCCESS, + {SDP_TYPE_STRING, SDP_ST_NONE, .data = {.string = {&test_cases[0].stream[2], 6}}}, + "foobar", + }, + { + {SDP_TYPEDISC_STR16, 0x00, 0x06, 'f', 'o', 'o', 'b', 'a', 'r'}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_STRING, SDP_ST_NONE, .data = {.string = {&test_cases[1].stream[3], 6}}}, + "foobar", + }, + { + {SDP_TYPEDISC_STR32, 0x00, 0x00, 0x00, 0x06, 'f', 'o', 'o', 'b', 'a', 'r'}, + 11, + ERROR_SUCCESS, + {SDP_TYPE_STRING, SDP_ST_NONE, .data = {.string = {&test_cases[2].stream[5], 6}}}, + "foobar", + } + }; + SIZE_T i; + + for (i = 0; i < ARRAY_SIZE( test_cases ); i++) + { + winetest_push_context( TEST_CASE_NAME( i+1 ) ); + test_BluetoothSdpGetElementData( test_cases[i].stream, test_cases[i].size, test_cases[i].error, + &test_cases[i].data ); + if (test_cases[i].error == ERROR_SUCCESS) + { + SDP_ELEMENT_DATA result = {0}; + BluetoothSdpGetElementData( test_cases[i].stream, test_cases[i].size, &result ); + ok( strlen( test_cases[i].string ) == result.data.string.length, + "Expected string %s, got %s.\n", debugstr_a( test_cases[i].string ), + debugstr_an( (const char *)result.data.string.value, result.data.string.length ) ); + ok( !memcmp( result.data.string.value, test_cases[i].string, + result.data.string.length ), + "Expected string %s, got %s.\n", debugstr_a( test_cases[i].string ), + debugstr_an( (const char *)result.data.string.value, result.data.string.length ) ); + } + winetest_pop_context(); + } +} + +static void test_BluetoothSdpGetElementData_seq( void ) +{ + static struct { + BYTE stream[11]; + SIZE_T size; + DWORD error; + SDP_ELEMENT_DATA data; + SDP_ELEMENT_DATA sequence[6]; + SIZE_T container_size; + } test_cases[] = { + { + {SDP_TYPEDISC_SEQ8, 0x06, SDP_TYPEDISC_UINT8, 0xde, SDP_TYPEDISC_UINT8, 0xad, SDP_TYPEDISC_UINT8, 0xbe}, + 8, + ERROR_SUCCESS, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, .data = {.sequence = {&test_cases[0].stream[0], 8}}}, + { + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xde}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xad}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xbe}} + }, + 3 + }, + { + {SDP_TYPEDISC_SEQ16, 0x00, 0x06, SDP_TYPEDISC_UINT8, 0xde, SDP_TYPEDISC_UINT8, 0xad, SDP_TYPEDISC_UINT8, 0xbe}, + 9, + ERROR_SUCCESS, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, .data = {.sequence = {&test_cases[1].stream[0], 9}}}, + { + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xde}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xad}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xbe}} + }, + 3 + }, + { + {SDP_TYPEDISC_SEQ32, 0x00, 0x00, 0x00, 0x06, SDP_TYPEDISC_UINT8, 0xde, SDP_TYPEDISC_UINT8, 0xad, SDP_TYPEDISC_UINT8, 0xbe}, + 11, + ERROR_SUCCESS, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, .data = {.sequence = {&test_cases[2].stream[0], 11}}}, + { + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xde}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xad}}, + {SDP_TYPE_UINT, SDP_ST_UINT8, {.uint8 = 0xbe}} + }, + 3 + }, + { + {SDP_TYPEDISC_SEQ8, SDP_TYPEDISC_UINT8, 0xde, SDP_TYPEDISC_UINT8, 0xad, SDP_TYPEDISC_UINT8, 0xbe}, + 1, + ERROR_INVALID_PARAMETER, + }, + }; + SIZE_T i; + + for (i = 0; i < ARRAY_SIZE( test_cases ); i++) + { + SIZE_T n = 0; + HBLUETOOTH_CONTAINER_ELEMENT handle = NULL; + DWORD ret; + + winetest_push_context( TEST_CASE_NAME( i+1 ) ); + test_BluetoothSdpGetElementData( test_cases[i].stream, test_cases[i].size, test_cases[i].error, + &test_cases[i].data ); + if (test_cases[i].error != ERROR_SUCCESS) + { + winetest_pop_context(); + continue; + } + + while (n < test_cases[i].container_size) + { + SDP_ELEMENT_DATA container_elem = {0}; + + winetest_push_context( "test_cases[%d].sequence[%d]", (int)i, (int)n ); + ret = BluetoothSdpGetContainerElementData( test_cases[i].data.data.sequence.value, + test_cases[i].data.data.sequence.length, + &handle, &container_elem ); + if (ret == ERROR_NO_MORE_ITEMS) + { + ok( n == test_cases[i].container_size, "Expected %d elements, got %d.\n", + (int)test_cases[i].container_size, (int)n ); + winetest_pop_context(); + break; + } + ok( ret == ERROR_SUCCESS, "BluetoothSdpGetContainerElementData failed: %ld.\n", ret ); + if (ret == ERROR_SUCCESS) + { + ok( !memcmp( &test_cases[i].sequence[n], &container_elem, + sizeof( container_elem ) ), + "Expected %s, got %s.\n", debugstr_SDP_ELEMENT_DATA( &test_cases[i].sequence[n] ), + debugstr_SDP_ELEMENT_DATA( &container_elem ) ); + } + n++; + winetest_pop_context(); + } + winetest_pop_context(); + } +} + +START_TEST( sdp ) +{ + test_BluetoothSdpGetElementData_nil(); + test_BluetoothSdpGetElementData_ints(); + test_BluetoothSdpGetElementData_invalid(); + test_BluetoothSdpGetElementData_str(); + test_BluetoothSdpGetElementData_seq(); +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/bluetoothapis/bluetoothapis.spec | 2 +- dlls/bluetoothapis/sdp.c | 40 +++++++++++++++++++++++++++ dlls/bthprops.cpl/bthprops.cpl.spec | 2 +- dlls/irprops.cpl/irprops.cpl.spec | 2 +- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/dlls/bluetoothapis/bluetoothapis.spec b/dlls/bluetoothapis/bluetoothapis.spec index 3531e2f901c..939072d308c 100644 --- a/dlls/bluetoothapis/bluetoothapis.spec +++ b/dlls/bluetoothapis/bluetoothapis.spec @@ -54,7 +54,7 @@ @ stdcall BluetoothRegisterForAuthenticationEx(ptr ptr ptr ptr) @ stub BluetoothRemoveDevice @ stdcall BluetoothSdpEnumAttributes(ptr long ptr ptr) -@ stub BluetoothSdpGetAttributeValue +@ stdcall BluetoothSdpGetAttributeValue(ptr long long ptr) @ stdcall BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString diff --git a/dlls/bluetoothapis/sdp.c b/dlls/bluetoothapis/sdp.c index b09daf01c8d..a5bc325390e 100644 --- a/dlls/bluetoothapis/sdp.c +++ b/dlls/bluetoothapis/sdp.c @@ -386,3 +386,43 @@ BOOL WINAPI BluetoothSdpEnumAttributes( BYTE *stream, ULONG stream_size, return TRUE; } } + +struct get_attr_value_data +{ + USHORT attr_id; + BYTE *attr_stream; + ULONG stream_size; +}; + +static BOOL WINAPI get_attr_value_callback( ULONG attr_id, BYTE *stream, ULONG stream_size, + void *params ) +{ + struct get_attr_value_data *args = params; + if (attr_id == args->attr_id) + { + args->attr_stream = stream; + args->stream_size = stream_size; + return FALSE; + } + return TRUE; +} + +/********************************************************************* + * BluetoothSdpGetAttributeValue + */ +DWORD WINAPI BluetoothSdpGetAttributeValue( BYTE *stream, ULONG stream_size, USHORT attr_id, + SDP_ELEMENT_DATA *data ) +{ + struct get_attr_value_data args = {0}; + + TRACE( "(%p %lu %u %p)\n", stream, stream_size, attr_id, data ); + + if (stream == NULL || data == NULL) return ERROR_INVALID_PARAMETER; + + args.attr_id = attr_id; + if (!BluetoothSdpEnumAttributes( stream, stream_size, get_attr_value_callback, &args )) + return ERROR_INVALID_PARAMETER; + if (!args.attr_stream) return ERROR_FILE_NOT_FOUND; + + return BluetoothSdpGetElementData( args.attr_stream, args.stream_size, data ); +} diff --git a/dlls/bthprops.cpl/bthprops.cpl.spec b/dlls/bthprops.cpl/bthprops.cpl.spec index 8e4d2e02a42..7fe1d82ac89 100644 --- a/dlls/bthprops.cpl/bthprops.cpl.spec +++ b/dlls/bthprops.cpl/bthprops.cpl.spec @@ -45,7 +45,7 @@ @ stdcall -import BluetoothRegisterForAuthenticationEx(ptr ptr ptr ptr) @ stub BluetoothRemoveDevice @ stdcall -import BluetoothSdpEnumAttributes(ptr long ptr ptr) -@ stub BluetoothSdpGetAttributeValue +@ stdcall -import BluetoothSdpGetAttributeValue(ptr long long ptr) @ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString diff --git a/dlls/irprops.cpl/irprops.cpl.spec b/dlls/irprops.cpl/irprops.cpl.spec index a4cc145dc8f..0baeb9179e3 100644 --- a/dlls/irprops.cpl/irprops.cpl.spec +++ b/dlls/irprops.cpl/irprops.cpl.spec @@ -39,7 +39,7 @@ @ stub BluetoothRegisterForAuthentication @ stub BluetoothRemoveDevice @ stdcall -import BluetoothSdpEnumAttributes(ptr long ptr ptr) -@ stub BluetoothSdpGetAttributeValue +@ stdcall -import BluetoothSdpGetAttributeValue(ptr long long ptr) @ stdcall -import BluetoothSdpGetContainerElementData(ptr long ptr ptr) @ stdcall -import BluetoothSdpGetElementData(ptr long ptr) @ stub BluetoothSdpGetString -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/bluetoothapis/tests/sdp.c | 80 ++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/dlls/bluetoothapis/tests/sdp.c b/dlls/bluetoothapis/tests/sdp.c index ea223aeaa40..69049d05363 100644 --- a/dlls/bluetoothapis/tests/sdp.c +++ b/dlls/bluetoothapis/tests/sdp.c @@ -506,6 +506,85 @@ static void test_BluetoothSdpGetElementData_seq( void ) } } +struct attr_callback_data +{ + const ULONG *attrs_id; + const SDP_ELEMENT_DATA *attrs; + const SIZE_T attrs_n; + SIZE_T i; +}; + +static BYTE sdp_record_bytes[] = { + 0x35, 0x48, 0x09, 0x00, 0x00, 0x0a, 0x00, 0x01, 0x00, 0x00, 0x09, 0x00, 0x01, 0x35, 0x03, + 0x19, 0x12, 0x00, 0x09, 0x00, 0x05, 0x35, 0x03, 0x19, 0x10, 0x02, 0x09, 0x00, 0x09, 0x35, + 0x08, 0x35, 0x06, 0x19, 0x12, 0x00, 0x09, 0x01, 0x03, 0x09, 0x02, 0x00, 0x09, 0x01, 0x03, + 0x09, 0x02, 0x01, 0x09, 0x1d, 0x6b, 0x09, 0x02, 0x02, 0x09, 0x02, 0x46, 0x09, 0x02, 0x03, + 0x09, 0x05, 0x4d, 0x09, 0x02, 0x04, 0x28, 0x01, 0x09, 0x02, 0x05, 0x09, 0x00, 0x02 }; + +static BOOL WINAPI enum_attr_callback( ULONG attr_id, BYTE *stream, ULONG stream_size, void *param ) +{ + struct attr_callback_data *params = param; + winetest_push_context( TEST_CASE_NAME( params->i + 1) ); + if (params->i < params->attrs_n) + { + SDP_ELEMENT_DATA data = {0}, data2 = {0}; + DWORD result; + + ok( attr_id == params->attrs_id[params->i], "Expected attribute id %lu, got %lu.\n", + params->attrs_id[params->i], attr_id ); + result = BluetoothSdpGetElementData( stream, stream_size, &data ); + ok( result == ERROR_SUCCESS, "BluetoothSdpGetElementData failed: %ld.\n", result ); + ok( !memcmp( ¶ms->attrs[params->i], &data, sizeof( data ) ), "Expected %s, got %s.\n", + debugstr_SDP_ELEMENT_DATA( ¶ms->attrs[params->i] ), + debugstr_SDP_ELEMENT_DATA( &data ) ); + + result = BluetoothSdpGetAttributeValue( sdp_record_bytes, ARRAY_SIZE( sdp_record_bytes ), + params->attrs_id[params->i], &data2 ); + ok( result == ERROR_SUCCESS, "BluetoothSdpGetAttributeValue failed: %ld.\n", result ); + ok( !memcmp( ¶ms->attrs[params->i], &data2, sizeof( data2 ) ), "Expected %s, got %s.\n", + debugstr_SDP_ELEMENT_DATA( ¶ms->attrs[params->i] ), + debugstr_SDP_ELEMENT_DATA( &data2 ) ); + + params->i++; + } + winetest_pop_context(); + return TRUE; +} + +static void test_BluetoothSdpEnumAttributes( void ) +{ + static SDP_ELEMENT_DATA attributes[] = { + {SDP_TYPE_UINT, SDP_ST_UINT32, {.uint32 = 0x10000}}, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, {.sequence = {&sdp_record_bytes[13], 5}}}, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, {.sequence = {&sdp_record_bytes[21], 5}}}, + {SDP_TYPE_SEQUENCE, SDP_ST_NONE, {.sequence = {&sdp_record_bytes[29], 10}}}, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0x0103}}, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0x1d6b}}, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0x0246}}, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0x054d}}, + {SDP_TYPE_BOOLEAN, SDP_ST_NONE, {.booleanVal = 1}}, + {SDP_TYPE_UINT, SDP_ST_UINT16, {.uint16 = 0x02}}, + }; + const ULONG attrs_id[] = {0x0, 0x1, 0x5, 0x9, 0x200, 0x201, 0x202, 0x203, 0x204, 0x205}; + struct attr_callback_data data = {attrs_id, attributes, ARRAY_SIZE( attributes ), 0}; + SDP_ELEMENT_DATA elem_data = {0}; + DWORD result; + + BOOL ret; + + SetLastError( 0xdeadbeef ); + ret = BluetoothSdpEnumAttributes( sdp_record_bytes, ARRAY_SIZE( sdp_record_bytes ), enum_attr_callback, + &data ); + ok( ret, "BluetoothSdpEnumAttributes failed with %ld.\n", GetLastError() ); + ok( data.i == data.attrs_n, "%d != %d\n", (int)data.i, (int)data.attrs_n ); + + result = BluetoothSdpGetAttributeValue( sdp_record_bytes, ARRAY_SIZE( sdp_record_bytes ), 0xff, + &elem_data ); + ok( result == ERROR_FILE_NOT_FOUND, + "Expected BluetoothSdpGetAttributeValue to return %d, got %ld.\n", ERROR_FILE_NOT_FOUND, + result ); +} + START_TEST( sdp ) { test_BluetoothSdpGetElementData_nil(); @@ -513,4 +592,5 @@ START_TEST( sdp ) test_BluetoothSdpGetElementData_invalid(); test_BluetoothSdpGetElementData_str(); test_BluetoothSdpGetElementData_seq(); + test_BluetoothSdpEnumAttributes(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6402
Please reorder the commits so that the tests come first in the git history, with todo_wine where appropriate, *then* the commits that implement the things you added tests for, removing the todo_wine as they get implemented. It's of course also a good idea to split into batches like you already did, only with the tests first in each batch. In this case, as the tests use functions which aren't yet exported, you will need to add stub functions to bluetoothapi first, then the tests, then the implementation. It's also generally well appreciated to split changes (a lot, you can hardly overdo it), and test and implement things gradually. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82788
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ ok( ret == error, "Expected BluetoothSdpGetElementData to return %ld, got %ld.\n", error, ret ); + if (error == ERROR_SUCCESS) + { + ok( result.type == sdp_data->type, "Expected SDP_TYPE %s, got %s.\n", + debugstr_SDP_TYPE( sdp_data->type ), debugstr_SDP_TYPE( result.type ) ); + ok( result.specificType == sdp_data->specificType, + "Expected SDP_SPECIFIC_TYPE %s, got %s.\n", + debugstr_SDP_SPECIFIC_TYPE( sdp_data->specificType ), + debugstr_SDP_SPECIFIC_TYPE( result.specificType ) ); + ok( !memcmp( &sdp_data->data, &result.data, sizeof( result.data ) ), "Expected %s, got %s.\n", + debugstr_SDP_ELEMENT_DATA( sdp_data ), debugstr_SDP_ELEMENT_DATA( &result ) ); + + } +} + +#define TEST_CASE_NAME( n ) ("%s %d"), __func__, (int)(n) Similarly, it is often a good idea to keep the test context as short as possible. It's being added to every todo line, and quickly adds up to the output size failure threshold.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82790
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ ok( result.specificType == sdp_data->specificType, + "Expected SDP_SPECIFIC_TYPE %s, got %s.\n", + debugstr_SDP_SPECIFIC_TYPE( sdp_data->specificType ), + debugstr_SDP_SPECIFIC_TYPE( result.specificType ) ); + ok( !memcmp( &sdp_data->data, &result.data, sizeof( result.data ) ), "Expected %s, got %s.\n", + debugstr_SDP_ELEMENT_DATA( sdp_data ), debugstr_SDP_ELEMENT_DATA( &result ) ); + + } +} + +#define TEST_CASE_NAME( n ) ("%s %d"), __func__, (int)(n) + +static void test_BluetoothSdpGetElementData_invalid( void ) +{ + SDP_ELEMENT_DATA data; + BYTE stream[] = {0b00000000}; I don't think we have 0b anywhere else, how portable is it, and is it really useful here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82791
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ "Expected BluetoothSdpGetElementData to return %d, got %ld.\n", ERROR_INVALID_PARAMETER, + ret ); +} + +static void test_BluetoothSdpGetElementData_nil( void ) +{ + static struct + { + BYTE data_elem; + DWORD error; + SDP_ELEMENT_DATA data; + } test_cases[] = { + {0b00000000, ERROR_SUCCESS, {.type = SDP_TYPE_NIL, .specificType = SDP_ST_NONE }}, + {0b00000001, ERROR_INVALID_PARAMETER}, + {0b00000011, ERROR_INVALID_PARAMETER}, + {0b00000100, ERROR_INVALID_PARAMETER}, Same thing here, do we need 0b? I have no idea how "safe" it is to use it, I believe we keep MSVC compatibility for anything that is built as PE, does it work there?
IMO would be simpler to avoid the question and not use it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82792
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ test_BluetoothSdpGetElementData( &test_cases[i].data_elem, 1, test_cases[i].error, + &test_cases[i].data ); + winetest_pop_context(); + } +} + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +#define SDP_DATA_ELEM_TYPE_DESC(t,s) ((t) << 3 | SDP_SIZEDESC_##s) Any reason this is named SIZEDESC instead of SIZE_DESC, as every other word is separated?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82793
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ winetest_pop_context(); + } +} + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +#define SDP_DATA_ELEM_TYPE_DESC(t,s) ((t) << 3 | SDP_SIZEDESC_##s) + +#define SDP_DEF_TYPE(n, t, s) const static BYTE SDP_TYPEDISC_##n = SDP_DATA_ELEM_TYPE_DESC(SDP_TYPE_##t, s) Same question with TYPE_DESC (which you probably mean instead of DISC).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82794
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+} + +static void test_BluetoothSdpGetElementData_str( void ) +{ + static struct { + BYTE stream[11]; + SIZE_T size; + DWORD error; + SDP_ELEMENT_DATA data; + const char *string; + } test_cases[] = { + { + {SDP_TYPEDISC_STR8, 0x06, 'f', 'o', 'o', 'b', 'a', 'r'}, + 8, + ERROR_SUCCESS, + {SDP_TYPE_STRING, SDP_ST_NONE, .data = {.string = {&test_cases[0].stream[2], 6}}}, Nit: I find the mix of designated initializers / non-designated a bit weird, `.data =` doesn't seem useful?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82795
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ case SDP_TYPE_STRING: + return wine_dbg_sprintf( "{.string={%p %lu}}", data->data.string.value, + data->data.string.length ); + case SDP_TYPE_URL: + return wine_dbg_sprintf( "{.url={%p %lu}}", data->data.url.value, + data->data.url.length ); + case SDP_TYPE_SEQUENCE: + return wine_dbg_sprintf( "{.sequence={%p %lu}}", data->data.sequence.value, + data->data.sequence.length ); + case SDP_TYPE_ALTERNATIVE: + return wine_dbg_sprintf( "{.alternative={%p %lu}}", data->data.alternative.value, + data->data.alternative.length ); + default: + return debugstr_sdp_element_data_unknown( data ); + } +} This is a lot of code for debug printing, do we really need it in the tests?
There's a lot of tests, and I don't think being verbose in the test messages have been considered very useful yet. After a couple hundred of written tests you will likely find the `ok("got %#x", err)` pattern very appealing. In general tests aren't supposed to fail, so the messages don't matter much. When they do, you will anyway likely need to reproduce the failure locally, where you can add as much detail about it as you like. Note that we also have a strong test output size limit of 32K, and any todo_wine that is printed out should stay as little verbose as possible in order to not reach that limit and fail the test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82789
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ +#ifdef WORDS_BIGENDIAN +#define BTH_READ_UINT16( s ) (s) +#define BTH_READ_UINT32( s ) (s) +#define BTH_READ_UINT64( s ) (s) +#else +#define BTH_READ_UINT16( s ) RtlUshortByteSwap( *(USHORT *)(s) ) +#define BTH_READ_UINT32( s ) RtlUlongByteSwap( *(ULONG *)(s) ) +#define BTH_READ_UINT64( s ) RtlUlonglongByteSwap( *(ULONGLONG *)(s) ) +#endif + +#define BTH_READ_UINT128( s, v ) \ + do \ + { \ + (v)->HighPart = BTH_READ_UINT64( (s) ); \ + (v)->LowPart = BTH_READ_UINT64( (s + 8) ); \
(v)->LowPart = BTH_READ_UINT64( ((s) + 8) ); \
Then what about a function helper instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82799
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ +#define BTH_READ_UINT128( s, v ) \ + do \ + { \ + (v)->HighPart = BTH_READ_UINT64( (s) ); \ + (v)->LowPart = BTH_READ_UINT64( (s + 8) ); \ + } while (0) + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 Same comment as in the test, why is SIZEDESC kept as one word?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82800
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ 17, + ERROR_SUCCESS, + {SDP_TYPE_INT, SDP_ST_INT128, {.int128 = {0xdeadbeefdeadbeef, 0xdeadbeefdeadbeef}}}, + }, + { + {SDP_TYPEDISC_UINT128, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef}, + 17, + ERROR_SUCCESS, + {SDP_TYPE_UINT, SDP_ST_UINT128, {.uint128 = {0xdeadbeefdeadbeef, 0xdeadbeefdeadbeef}}}, + } + }; + SIZE_T i; + + for (i = 0; i < ARRAY_SIZE( test_cases ); i++) + { + winetest_push_context( TEST_CASE_NAME( i+1 ) ); Using i+1 as the printed index is very misleading IMO.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82796
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ (v)->HighPart = BTH_READ_UINT64( (s) ); \ + (v)->LowPart = BTH_READ_UINT64( (s + 8) ); \ + } while (0) + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +static inline BYTE data_elem_type( BYTE elem ) { return ( elem & 0b11111000 ) >> 3; } +static inline BYTE data_elem_size_desc( BYTE elem ) { return elem & 0b00000111; } + Same comment as in the tests wrt 0b prefix. Then, do these need to be marked inline at all?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82801
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +static inline BYTE data_elem_type( BYTE elem ) { return ( elem & 0b11111000 ) >> 3; } +static inline BYTE data_elem_size_desc( BYTE elem ) { return elem & 0b00000111; } + +#define SDP_ELEMENT_IS_UINT16( d ) ( (d)->type == SDP_TYPE_UINT && (d)->specificType == SDP_ST_UINT16 ) +#define SDP_ELEMENT_IS_ATTRID( d ) SDP_ELEMENT_IS_UINT16((d)) + +/* Read the data element's size/length as described by the size descriptor, starting from stream. Only + * valid for SDP_SIZEDESC_NEXT_* types. */ +static BOOL sdp_elem_read_var_size( BYTE *stream, ULONG stream_size, SIZE_T *read, BYTE size_desc, + UINT32 *size ) +{ + switch(size_desc)
switch (size_desc)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82802
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/tests/sdp.c:
+ test_BluetoothSdpGetElementData( test_cases[i].stream, test_cases[i].size, test_cases[i].error, + &test_cases[i].data ); + if (test_cases[i].error != ERROR_SUCCESS) + { + winetest_pop_context(); + continue; + } + + while (n < test_cases[i].container_size) + { + SDP_ELEMENT_DATA container_elem = {0}; + + winetest_push_context( "test_cases[%d].sequence[%d]", (int)i, (int)n ); + ret = BluetoothSdpGetContainerElementData( test_cases[i].data.data.sequence.value, + test_cases[i].data.data.sequence.length, + &handle, &container_elem ); Specifically, as it's implemented separately I would suggest to split these tests as well, test one function, the implement it.
BluetoothSdpGetContainerElementData tests for instance could be split to a separate change, then implemented. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82797
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ +#include "wine/debug.h" +#include "bthsdpdef.h" +#include "bluetoothapis.h" + +WINE_DEFAULT_DEBUG_CHANNEL( bluetoothapis ); + +#ifdef WORDS_BIGENDIAN +#define BTH_READ_UINT16( s ) (s) +#define BTH_READ_UINT32( s ) (s) +#define BTH_READ_UINT64( s ) (s) +#else +#define BTH_READ_UINT16( s ) RtlUshortByteSwap( *(USHORT *)(s) ) +#define BTH_READ_UINT32( s ) RtlUlongByteSwap( *(ULONG *)(s) ) +#define BTH_READ_UINT64( s ) RtlUlonglongByteSwap( *(ULONGLONG *)(s) ) +#endif I don't think we care about big endian on the PE side at all (not sure we even do on the unix side, but who knows).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82798
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ case SDP_TYPE_INT: + case SDP_TYPE_UUID: + if (size_desc > SDP_SIZEDESC_16_BYTES) return ERROR_INVALID_PARAMETER; + + st = SDP_BASIC_TYPES[type][size_desc]; + if (st == SDP_ST_NONE) return ERROR_INVALID_PARAMETER; + + if (!sdp_read_specific_type( stream, stream_size, st, data, read )) + return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = st; + break; + case SDP_TYPE_BOOLEAN: + if (size_desc != SDP_SIZEDESC_1_BYTE) return ERROR_INVALID_PARAMETER; + if (stream_size < sizeof(BYTE)) return ERROR_INVALID_PARAMETER; A bit sad that this one lost its parens.
Fwiw, the "space in paren Wine code style" usually only applies to function decls and calls and macros, with a couple of exceptions "where it looks better". So, sizeof operator doesn't usually have spaces inside. I don't want to bikeshed about it, but please be consistent. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82805
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ [SDP_SIZEDESC_16_BYTES] = SDP_ST_INT128, + }, + [SDP_TYPE_UUID] = + { + [SDP_SIZEDESC_2_BYTES] = SDP_ST_UUID16, + [SDP_SIZEDESC_4_BYTES] = SDP_ST_UUID32, + [SDP_SIZEDESC_16_BYTES] = SDP_ST_UUID128, + }, +}; + +static BOOL sdp_read_specific_type( BYTE *stream, ULONG stream_size, SDP_SPECIFICTYPE st, + SDP_ELEMENT_DATA *data, SIZE_T *read ) +{ + switch (st) + { + case SDP_ST_UINT8: Same thing wrt consistency, please chose a way to indent cases within a switch, and stick to it.
I personally prefer cases on the same level as the switch like here, but consistency is what matters. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82806
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ } while (0) + +#define SDP_SIZEDESC_1_BYTE 0 +#define SDP_SIZEDESC_2_BYTES 1 +#define SDP_SIZEDESC_4_BYTES 2 +#define SDP_SIZEDESC_8_BYTES 3 +#define SDP_SIZEDESC_16_BYTES 4 +#define SDP_SIZEDESC_NEXT_UINT8 5 +#define SDP_SIZEDESC_NEXT_UINT16 6 +#define SDP_SIZEDESC_NEXT_UINT32 7 + +static inline BYTE data_elem_type( BYTE elem ) { return ( elem & 0b11111000 ) >> 3; } +static inline BYTE data_elem_size_desc( BYTE elem ) { return elem & 0b00000111; } + +#define SDP_ELEMENT_IS_UINT16( d ) ( (d)->type == SDP_TYPE_UINT && (d)->specificType == SDP_ST_UINT16 ) +#define SDP_ELEMENT_IS_ATTRID( d ) SDP_ELEMENT_IS_UINT16((d)) These macros don't seem to be used?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82807
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ cursor = (BYTE *)(*handle); + + if (cursor == NULL) + { + BYTE header; + BYTE type, size_desc; + UINT32 elems_size = 0; + SIZE_T read = 0; + + header = *stream; + type = data_elem_type( header ); + size_desc = data_elem_size_desc( header ); + + if (type != SDP_TYPE_SEQUENCE && type != SDP_TYPE_ALTERNATIVE) + return ERROR_INVALID_PARAMETER; + if (!( size_desc >= SDP_SIZEDESC_NEXT_UINT8 && size_desc <= SDP_SIZEDESC_NEXT_UINT32 ))
if (!(size_desc >= SDP_SIZEDESC_NEXT_UINT8 && size_desc <= SDP_SIZEDESC_NEXT_UINT32))
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82808
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ return ERROR_INVALID_PARAMETER; + + stream++; + if (!sdp_elem_read_var_size( stream, stream_size, &read, size_desc, &elems_size )) + return ERROR_INVALID_PARAMETER; + + stream += read; + stream_size -= read; + } + else + { + if (cursor < stream) return ERROR_INVALID_PARAMETER; + if (cursor == (stream + stream_size)) return ERROR_NO_MORE_ITEMS; + + stream = cursor; + stream_size = stream_size - (ptrdiff_t)(cursor - stream); Do you really need a cast here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82809
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+static inline BYTE data_elem_type( BYTE elem ) { return ( elem & 0b11111000 ) >> 3; } +static inline BYTE data_elem_size_desc( BYTE elem ) { return elem & 0b00000111; } + +#define SDP_ELEMENT_IS_UINT16( d ) ( (d)->type == SDP_TYPE_UINT && (d)->specificType == SDP_ST_UINT16 ) +#define SDP_ELEMENT_IS_ATTRID( d ) SDP_ELEMENT_IS_UINT16((d)) + +/* Read the data element's size/length as described by the size descriptor, starting from stream. Only + * valid for SDP_SIZEDESC_NEXT_* types. */ +static BOOL sdp_elem_read_var_size( BYTE *stream, ULONG stream_size, SIZE_T *read, BYTE size_desc, + UINT32 *size ) +{ + switch(size_desc) + { + case SDP_SIZEDESC_NEXT_UINT8: + if (stream_size < sizeof( UINT8 )) return FALSE; + *size = (UINT8)(*stream); You don't need a cast?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82803
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ UINT32 *size ) +{ + switch(size_desc) + { + case SDP_SIZEDESC_NEXT_UINT8: + if (stream_size < sizeof( UINT8 )) return FALSE; + *size = (UINT8)(*stream); + *read += sizeof( UINT8 ); + return TRUE; + case SDP_SIZEDESC_NEXT_UINT16: + if (stream_size < sizeof( UINT16 )) return FALSE; + *size = BTH_READ_UINT16( stream ); + *read += sizeof( UINT16 ); + return TRUE; + case SDP_SIZEDESC_NEXT_UINT32: + if ( stream_size < sizeof( UINT32 ) ) return FALSE;
if (stream_size < sizeof( UINT32 )) return FALSE;
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82804
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ data->type = type; + data->specificType = st; + break; + case SDP_TYPE_BOOLEAN: + if (size_desc != SDP_SIZEDESC_1_BYTE) return ERROR_INVALID_PARAMETER; + if (stream_size < sizeof(BYTE)) return ERROR_INVALID_PARAMETER; + + data->type = type; + data->specificType = SDP_ST_NONE; + data->data.booleanVal = *stream; + *read += sizeof( BYTE ); + break; + case SDP_TYPE_STRING: + case SDP_TYPE_URL: + case SDP_TYPE_SEQUENCE: + case SDP_TYPE_ALTERNATIVE: As a suggestion for how this large chunk could be split (maybe not strictly necessary here but feel free to do it if you are happy to): you could test, then implement, each category of element type separately.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82815
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ SDP_ELEMENT_DATA attr = {0}; + BYTE *raw_attr_stream; + + result = BluetoothSdpGetContainerElementData( + data.data.sequence.value, data.data.sequence.length, &cursor, &attrid ); + if (result == ERROR_NO_MORE_ITEMS) return TRUE; + if (result != ERROR_SUCCESS) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + if (!SDP_ELEMENT_IS_ATTRID( &attrid )) + { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + }
if (result || !SDP_ELEMENT_IS_ATTRID( &attrid ))
{
SetLastError( ERROR_INVALID_DATA );
return FALSE;
}
What about grouping these? Note that we often prefer `!` instead of explicit comparison with ERROR_SUCCESS / STATUS_SUCCESS. Same thing goes with NULL btw, for the various pointer checks in this file. (With HRESULT it's a bit different because of SUCCEEDED/FAILED macros) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82814
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ case SDP_TYPE_SEQUENCE: + case SDP_TYPE_ALTERNATIVE: + break; + default: + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + while (TRUE) + { + SDP_ELEMENT_DATA attrid = {0}; + SDP_ELEMENT_DATA attr = {0}; + BYTE *raw_attr_stream; + + result = BluetoothSdpGetContainerElementData( + data.data.sequence.value, data.data.sequence.length, &cursor, &attrid );
result = BluetoothSdpGetContainerElementData( data.data.sequence.value, data.data.sequence.length,
&cursor, &attrid );
You used this style for continuation above and below, and it better matches the "space in paren" Wine code style. Don't be afraid of longer lines when it makes things more readable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82813
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ { + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + switch (data.type) + { + case SDP_TYPE_SEQUENCE: + case SDP_TYPE_ALTERNATIVE: + break; + default: + SetLastError( ERROR_INVALID_DATA ); + return FALSE; + } + + while (TRUE) I think `for (;;)` is usually preferred for such loops.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82812
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+{ + BYTE *cursor; + DWORD result; + SIZE_T read = 0; + + TRACE( "(%p, %lu, %p, %p)\n", stream, stream_size, handle, data ); + + if (stream == NULL || stream_size < sizeof( BYTE ) || handle == NULL || data == NULL) + return ERROR_INVALID_PARAMETER; + + cursor = (BYTE *)(*handle); + + if (cursor == NULL) + { + BYTE header; + BYTE type, size_desc; We usually keep declarations grouped by type.
```suggestion:-1+0 BYTE header, type, size_desc; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82811
Rémi Bernon (@rbernon) commented about dlls/bluetoothapis/sdp.c:
+ stream += read; + stream_size -= read; + } + else + { + if (cursor < stream) return ERROR_INVALID_PARAMETER; + if (cursor == (stream + stream_size)) return ERROR_NO_MORE_ITEMS; + + stream = cursor; + stream_size = stream_size - (ptrdiff_t)(cursor - stream); + } + result = sdp_read_element_data( stream, stream_size, data, &read ); + if (result != ERROR_SUCCESS) return result; + + stream += read; + TRACE("handle=%p\n", stream);
TRACE( "handle=%p\n", stream );
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82810
Sorry for the ton of comments, mostly nits and some general guidance but fwiw it looks pretty good otherwise. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6402#note_82816
participants (3)
-
Rémi Bernon -
Vibhav Pant
-
Vibhav Pant (＠vibhavp)