Re: [PATCH v2 2/2] hid/tests: Add HID device enumeration test

[Sebastian Lackner]

On 17.10.2016 15:58, Aric Stewart wrote:
> v2: Suggestions from Sebastian Lackner
> Signed-off-by: Aric Stewart
>
> configure                  |  1 +
>  configure.ac               |  1 +
>  dlls/hid/tests/Makefile.in |  5 +++
>  dlls/hid/tests/device.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 89 insertions(+)
>  create mode 100644 dlls/hid/tests/Makefile.in
>  create mode 100644 dlls/hid/tests/device.c
>
> v2-0002-hid-tests-Add-HID-device-enumeration-test.txt
>
> diff --git a/configure b/configure
> index c21bda3..b397b38 100755
> --- a/configure
> +++ b/configure
> @@ -17940,6 +17940,7 @@ wine_fn_config_dll gpkcsp enable_gpkcsp
>  wine_fn_config_dll hal enable_hal
>  wine_fn_config_dll hhctrl.ocx enable_hhctrl_ocx clean,implib htmlhelp
>  wine_fn_config_dll hid enable_hid implib
> +wine_fn_config_test dlls/hid/tests hid_test
>  wine_fn_config_dll hidclass.sys enable_hidclass_sys implib hidclass
>  wine_fn_config_dll hlink enable_hlink clean,implib
>  wine_fn_config_test dlls/hlink/tests hlink_test
> diff --git a/configure.ac b/configure.ac
> index c624c9e..40060b7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -3027,6 +3027,7 @@ WINE_CONFIG_DLL(gpkcsp)
>  WINE_CONFIG_DLL(hal)
>  WINE_CONFIG_DLL(hhctrl.ocx,,[clean,implib],[htmlhelp])
>  WINE_CONFIG_DLL(hid,,[implib])
> +WINE_CONFIG_TEST(dlls/hid/tests)
>  WINE_CONFIG_DLL(hidclass.sys,,[implib],[hidclass])
>  WINE_CONFIG_DLL(hlink,,[clean,implib])
>  WINE_CONFIG_TEST(dlls/hlink/tests)
> diff --git a/dlls/hid/tests/Makefile.in b/dlls/hid/tests/Makefile.in
> new file mode 100644
> index 0000000..6bf3773
> --- /dev/null
> +++ b/dlls/hid/tests/Makefile.in
> @@ -0,0 +1,5 @@
> +TESTDLL   = hid.dll
> +IMPORTS   = hid setupapi
> -
>
> +C_SRCS = \
> - device.c
>
> diff --git a/dlls/hid/tests/device.c b/dlls/hid/tests/device.c
> new file mode 100644
> index 0000000..47ad1a3
> --- /dev/null
> +++ b/dlls/hid/tests/device.c
> @@ -0,0 +1,82 @@
> +/*
> -
> - Copyright (c) 2016 Aric Stewart
> -
> -
> -
> - 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 "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windows.h"
> +#include "setupapi.h"
> +#include "ddk/hidsdi.h"
> -
>
> +#include "wine/test.h"
> -
>
> +static void enumerate_devices(void)

I didn't point this out earlier, but usually test functions start with "test_". An
exception would be if you plan to use this as a helper function to run other tests
for all enumerated devices. Then you could either move most of the code to START_TEST,
for example:

--- snip ---
START_TEST(device)
{
    ... enumeration code ...
    test_device_info(file);
    ... enumeration code ...
}
--- snip ---

or rename it to something like "run_for_each_device(callback)" and use a callback-based
approach:

--- snip ---
START_TEST(device)
{
    run_for_each_device(test_device_info);
}
--- snip ---
> +{
> - GUID hid_guid;
> - WCHAR device_name[128];
> - HDEVINFO info_set;
> - DWORD index = 0;
> - SP_DEVICE_INTERFACE_DATA interface_data;
> - DWORD detail_size = MAX_PATH * sizeof(WCHAR);
> - SP_DEVICE_INTERFACE_DETAIL_DATA_W *data;
> -
> - HidD_GetHidGuid(&hid_guid);
> -
> - ZeroMemory(&interface_data, sizeof(interface_data));
> - interface_data.cbSize = sizeof(interface_data);
> -
> - data = HeapAlloc(GetProcessHeap(), 0, sizeof(*data) + detail_size);
> - data->cbSize = sizeof(*data);
> -
> - info_set = SetupDiGetClassDevsW(&hid_guid, NULL, NULL, DIGCF_DEVICEINTERFACE);
> - while (SetupDiEnumDeviceInterfaces(info_set, NULL, &hid_guid, index, &interface_data))
> - {
> -
>    index ++;
> -
> -
>    if (SetupDiGetDeviceInterfaceDetailW(info_set, &interface_data, data, sizeof(*data) + detail_size, NULL, NULL))
> -
>    {
> -
>        NTSTATUS status;
> -
>        BOOL rc;
> -
>        PHIDP_PREPARSED_DATA ppd;
> -
>        HIDP_CAPS Caps;
> -
>        HANDLE file = CreateFileW(data->DevicePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0);
> -
>        if (file == INVALID_HANDLE_VALUE)
> -
>        {
> -
>            trace("Failed to access device %s, likely not plugged in or access is denied.\n", wine_dbgstr_w(data->DevicePath));
> -
>            continue;
> -
>        }
> -
>        rc = HidD_GetPreparsedData(file, &ppd);
> -
>        ok(rc, "Failed to get preparsed data(0x%x)\n", GetLastError());
> -
>        status = HidP_GetCaps(ppd, &Caps);
> -
>        ok(status == HIDP_STATUS_SUCCESS, "Failed to get Caps(0x%x)\n", status);
> -
>        rc = HidD_GetProductString(file, device_name, sizeof(device_name));
> -
>        ok(rc, "Failed to get product string(0x%x)\n", GetLastError());
> -
>        trace("Found device %s (%s, %02x, %02x)\n", wine_dbgstr_w(device_name), wine_dbgstr_w(data->DevicePath), Caps.UsagePage, Caps.Usage);
> -
>        rc = HidD_FreePreparsedData(ppd);
> -
>        ok(rc, "Failed to free preparsed data(0x%x)\n", GetLastError());
> -
>        CloseHandle(file);
> -
>    }
> - }
> - HeapFree(GetProcessHeap(), 0, data);
> - if (info_set)
> -
>    SetupDiDestroyDeviceInfoList(info_set);

According to MSDN SetupDiGetClassDevs returns INVALID_HANDLE_VALUE on failure, not NULL.
If this check is not necessary you can also remove it.
> +}
> -
>
> +START_TEST(device)
> +{
> - enumerate_devices();
>
> +}