Re: [2/10]hidclass.sys: add hidclass.sys

[Henri Verbeet]

I didn't really look at the rest of the series, but here are a few
things that caught my eye.

On 6 July 2015 at 17:46, Aric Stewart  wrote:
> +struct list minidriver_list = LIST_INIT(minidriver_list);

You don't seem to access this outside of main.c anywhere in this
series, and it seems somewhat unlikely that you'll ever have to.
Shouldn't this just be static?
> +static VOID WINAPI UnloadDriver(DRIVER_OBJECT *driver)

"void"
> - TRACE("Driver Unload\n");
> - LIST_FOR_EACH_ENTRY_SAFE(md, next, &minidriver_list, minidriver, entry)
> - {
> -
>    if (md->minidriver.DriverObject == driver)
> -
>    {
> -
>        if (md->DriverUnload)
> -
>            md->DriverUnload(md->minidriver.DriverObject);
> -
>        list_remove(&md->entry);
> -
>        break;
> -
>    }
> - }
>
> +}

Did you intend to use find_minidriver() here?
> +minidriver* find_minidriver(DRIVER_OBJECT* driver)
> +{
> - minidriver *md;
> - LIST_FOR_EACH_ENTRY(md, &minidriver_list, minidriver, entry)
> - {
> -
>    if (md->minidriver.DriverObject == driver)
> -
>        return md;
> - }
> - return NULL;
>
> +}

As the code currently is this isn't actually used until patch 10/10.
You could potentially use it from UnloadDriver() as mentioned above,
but it should still initially be static in that case. Your '*'
placement is inconsistent.
> +NTSTATUS WINAPI HidRegisterMinidriver(PHID_MINIDRIVER_REGISTRATION registration)

"HID_MINIDRIVER_REGISTRATION *registration"
> +BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID lpv)
> +{
> - switch(fdwReason)
> - {
> -
>    case DLL_PROCESS_ATTACH:
> -
>        DisableThreadLibraryCalls(hInstDLL);
> -
>        break;
> - }
> - return TRUE;
>
> +}

As long as you don't do anything special in here, you don't need an
explicit DllMain(). (But if you did, "void *" instead of LPVOID, and
parameter naming.)
> +/* These are from hidport.h but are only used here anyway */
> +typedef struct _HID_MINIDRIVER_REGISTRATION {

The brace placement is inconsistent with the rest of the code.
> - ULONG           Revision;
> - PDRIVER_OBJECT  DriverObject;
> - PUNICODE_STRING RegistryPath;

"UNICODE_STRING *RegistryPath;"
> - ULONG           DeviceExtensionSize;
> - BOOLEAN         DevicesArePolled;
> - UCHAR           Reserved[3];
>
> +} HID_MINIDRIVER_REGISTRATION, *PHID_MINIDRIVER_REGISTRATION;

If this is really only used inside hidclass.sys, you shouldn't need
the PHID_MINIDRIVER_REGISTRATION typedef.
> +NTSTATUS WINAPI HidRegisterMinidriver(PHID_MINIDRIVER_REGISTRATION  MinidriverRegistration);

The whitespace is odd here. Also, "HID_MINIDRIVER_REGISTRATION *registration".
> +typedef struct _minidriver
> +{
> - struct list entry;
> -
> - HID_MINIDRIVER_REGISTRATION minidriver;
> -
> - PDRIVER_UNLOAD DriverUnload;
>
> +} minidriver;

I suppose it's somewhat up to personal style, but you don't need a
typedef for this. Aside from saving you from typing "struct" a few
times it only obfuscates the code.

I suspect some of the imports and includes aren't actually used until
later in the series, but I didn't explicitly check that.