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.