From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH v3 5/6] ntoskrnl.exe/tests: Add some HidP_GetLinkCollectionNodes tests.
Message-Id: <6e06877d-95ad-eb8b-7ade-e1bc8181d856@codeweavers.com>
Date: Tue, 8 Jun 2021 16:01:33 -0500
In-Reply-To: <20210604091220.265735-5-rbernon@codeweavers.com>
References: <20210604091220.265735-1-rbernon@codeweavers.com> <20210604091220.265735-5-rbernon@codeweavers.com>

On 6/4/21 4:12 AM, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon <rbernon@codeweavers.com>
> ---
>   dlls/hid/hidp.c                    |  3 +++
>   dlls/ntoskrnl.exe/tests/ntoskrnl.c | 22 ++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/dlls/hid/hidp.c b/dlls/hid/hidp.c
> index c4d162695a9..fd2ac86da32 100644
> --- a/dlls/hid/hidp.c
> +++ b/dlls/hid/hidp.c
> @@ -1002,6 +1002,9 @@ NTSTATUS WINAPI HidP_GetLinkCollectionNodes(HIDP_LINK_COLLECTION_NODE *LinkColle
>   
>       TRACE("(%p, %p, %p)\n", LinkCollectionNode, LinkCollectionNodeLength, PreparsedData);
>   
> +    if (data->magic != HID_MAGIC)
> +        return HIDP_STATUS_INVALID_PREPARSED_DATA;
> +
>       if (*LinkCollectionNodeLength < data->caps.NumberLinkCollectionNodes)
>           return HIDP_STATUS_BUFFER_TOO_SMALL;
>   

It's not a huge deal, but it does bother me when a patch nominally 
against the tests modifies the implementation...

> diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c
> index f8f56c13342..099f747ff86 100644
> --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c
> @@ -1630,10 +1630,12 @@ static void test_hid_device(void)
>       SP_DEVICE_INTERFACE_DETAIL_DATA_A *iface_detail = (void *)buffer;
>       SP_DEVICE_INTERFACE_DATA iface = {sizeof(iface)};
>       SP_DEVINFO_DATA device = {sizeof(device)};
> +    HIDP_LINK_COLLECTION_NODE collections[16];
>       PHIDP_PREPARSED_DATA preparsed_data;
>       HIDP_BUTTON_CAPS button_caps[16];
>       HIDP_VALUE_CAPS value_caps[16];
>       BOOL ret, found = FALSE;
> +    DWORD collection_count;
>       OBJECT_ATTRIBUTES attr;
>       UNICODE_STRING string;
>       IO_STATUS_BLOCK io;
> @@ -1684,6 +1686,26 @@ static void test_hid_device(void)
>       ok(status == HIDP_STATUS_SUCCESS, "HidP_GetCaps returned %#x\n", status);
>       check_hidp_caps(&caps, &expect_hidp_caps);
>   
> +    collection_count = 0;
> +    status = HidP_GetLinkCollectionNodes(collections, &collection_count, preparsed_data);
> +    ok(status == HIDP_STATUS_BUFFER_TOO_SMALL, "HidP_GetLinkCollectionNodes returned %#x\n", status);
> +    todo_wine ok(collection_count == caps.NumberLinkCollectionNodes, "HidP_GetLinkCollectionNodes returned count %d, expected %d\n", collection_count, caps.NumberLinkCollectionNodes);
> +    collection_count = ARRAY_SIZE(collections);
> +    status = HidP_GetLinkCollectionNodes(collections, &collection_count, (PHIDP_PREPARSED_DATA)buffer);
> +    ok(status == HIDP_STATUS_INVALID_PREPARSED_DATA, "HidP_GetLinkCollectionNodes returned %#x\n", status);
> +    status = HidP_GetLinkCollectionNodes(collections, &collection_count, preparsed_data);
> +    ok(status == HIDP_STATUS_SUCCESS, "HidP_GetLinkCollectionNodes returned %#x\n", status);
> +    ok(collection_count == caps.NumberLinkCollectionNodes, "HidP_GetLinkCollectionNodes returned count %d, expected %d\n", collection_count, caps.NumberLinkCollectionNodes);
> +
> +    ok(collections[0].LinkUsage == HID_USAGE_GENERIC_JOYSTICK, "unexpected collection LinkUsage %x, expected %x\n", collections[0].LinkUsage, HID_USAGE_GENERIC_JOYSTICK);
> +    ok(collections[0].LinkUsagePage == HID_USAGE_PAGE_GENERIC, "unexpected collection LinkUsagePage %x, expected %x\n", collections[0].LinkUsagePage, HID_USAGE_PAGE_GENERIC);
> +    ok(collections[0].Parent == 0, "unexpected collection Parent %d, expected %d\n", collections[0].Parent, 0);
> +    ok(collections[0].NumberOfChildren == 0, "unexpected collection NumberOfChildren %d, expected %d\n", collections[0].NumberOfChildren, 0);
> +    ok(collections[0].NextSibling == 0, "unexpected collection NextSibling %d, expected %d\n", collections[0].NextSibling, 0);
> +    ok(collections[0].FirstChild == 0, "unexpected collection FirstChild %d, expected %d\n", collections[0].FirstChild, 0);
> +    ok(collections[0].CollectionType == 1, "unexpected collection CollectionType %d, expected %d\n", collections[0].CollectionType, 1);
> +    ok(collections[0].IsAlias == 0, "unexpected collection IsAlias %d, expected %d\n", collections[0].IsAlias, 0);
> +
>       count = ARRAY_SIZE(button_caps);
>       status = HidP_GetButtonCaps(HidP_Output, button_caps, &count, preparsed_data);
>       todo_wine ok(status == HIDP_STATUS_USAGE_NOT_FOUND, "HidP_GetButtonCaps returned %#x\n", status);
>