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 <aric(a)codeweavers.com> 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.