Re: [PATCH v2 1/2] ntoskrnl.exe: Implement IoRegisterDeviceInterface

[Zebediah Figura]

On 03/10/18 10:02, Aric Stewart wrote:
> +static NTSTATUS get_instance_id(DEVICE_OBJECT *device, WCHAR

**instance_id)
> +{
> - WCHAR *id, *ptr;
> - NTSTATUS status;
> -
> - status = get_device_id( device, BusQueryInstanceID, &id );
> - if (status != STATUS_SUCCESS)
> - {
> -
>    FIXME( "failed to get device ID\n" );
> -
>    return status;
> - }

Would ERR be more appropriate?
> -
> - struprW( id );
> - ptr = strchrW( id, '\' );
> -
> - while (ptr)
> - {
> -
>    *ptr = '#';
> -
>    ptr = strchrW( id,'\\' );
> - }

Stylistic choice, perhaps, but maybe this would be simpler?

while ((ptr = strchrW( id, '\' )))
    *ptr = '#';
> -
> - *instance_id = id;
> - return STATUS_SUCCESS;
>
> +}
> -
> -
>
> +/*****************************************************
> -
> -
>       IoRegisterDeviceInterface(NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device,

const GUID *class_guid, UNICODE_STRING *reference_string, UNICODE_STRING
*symbolic_link)
> +{
> - WCHAR *instance_id;
> - NTSTATUS status = STATUS_SUCCESS;
> - HDEVINFO infoset;
> - WCHAR *referenceW = NULL;
> - SP_DEVINFO_DATA devInfo;
> - SP_DEVICE_INTERFACE_DATA infoData;
> - SP_DEVICE_INTERFACE_DETAIL_DATA_W *data;
> - BOOL rc;
> -
> - TRACE( "(%p, %s, %s, %p)\n", device, debugstr_guid(class_guid),

debugstr_us(reference_string), symbolic_link );
> -
> - if (reference_string != NULL)
> -
>    referenceW = reference_string->Buffer;
> -
> - infoset = SetupDiGetClassDevsW(class_guid, referenceW, NULL,

DIGCF_DEVICEINTERFACE);
> -
> - status = get_instance_id( device, &instance_id );
> - if (status != STATUS_SUCCESS)
> - {
> -
>    ERR( "Failed to generate Instance ID\n" );
> -
>    return status;
> - }
> -
> - devInfo.cbSize = sizeof(devInfo);
> - rc = SetupDiCreateDeviceInfoW( infoset, instance_id, class_guid,

NULL, NULL, 0, &devInfo );
> - if (rc == 0)
> - {
> -
>    if (GetLastError() == ERROR_DEVINST_ALREADY_EXISTS)
> -
>    {
> -
>        DWORD required;
> -
>        DWORD index = 0;
> -
>        DWORD size = strlenW(instance_id) + 2;
> -
>        WCHAR *id = HeapAlloc(GetProcessHeap(), 0, size *

sizeof(WCHAR));
> -
>        do
> -
>        {
> -
>            rc = SetupDiEnumDeviceInfo( infoset, index, &devInfo );
> -
>            if (IsEqualGUID(&devInfo.ClassGuid, class_guid))
> -
>            {
> -
>                BOOL check;
> -
>                check = SetupDiGetDeviceInstanceIdW(infoset,

&devInfo, id, size, &required);
> -
>                if (check && strcmpW(id, instance_id) == 0)
> -
>                    break;
> -
>            }
> -
>            index++;
> -
>        } while (rc);

I guess it maybe shouldn't block this patch, but it would be good to
implement SetupDiOpenDeviceInfo() at some point; it would result in much
cleaner code on both sides.
> -
> -
>        HeapFree(GetProcessHeap(), 0, id);
> -
>        if (!rc)
> -
>        {
> -
>            ERR( "Failed to find Device\n" );
> -
>            HeapFree( GetProcessHeap(), 0, instance_id );
> -
>            return STATUS_UNSUCCESSFUL;
> -
>        }
> -
>    }
> -
>    else
> -
>    {
> -
>        ERR( "Failed to Create Device\n" );
> -
>        HeapFree( GetProcessHeap(), 0, instance_id );
> -
>        return STATUS_UNSUCCESSFUL;
> -
>    }
> - }
> - HeapFree( GetProcessHeap(), 0, instance_id );
> -
> - infoData.cbSize = sizeof(infoData);
> - rc = SetupDiCreateDeviceInterfaceW(infoset, &devInfo, class_guid,

NULL, 0, &infoData);
> - if (!rc)
> - {
> -
>    ERR("Failed to create device interface\n");
> -
>    return STATUS_UNSUCCESSFUL;
> - }
> -
> - data = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY ,

sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W) + 1000);

This seems kind of ugly; can't we just call it twice?
> - data->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
> -
> - rc = SetupDiGetDeviceInterfaceDetailW(infoset, &infoData, data,

500, NULL, NULL);
> - if (!rc)
> - {
> -
>    ERR("Failed Get device details\n");
> -
>    return STATUS_UNSUCCESSFUL;
> - }
> -
> - data->DevicePath[1] = '?';
> - ERR("Device path %s\n",debugstr_w(data->DevicePath));

Leftover debug string?
> -
> - if (symbolic_link)
> -
>    RtlCreateUnicodeString( symbolic_link, data->DevicePath);
> -
> - HeapFree( GetProcessHeap(), 0, data );
> -
> - return status;
>
> +}
> -
> -
>
> /***********************************************************************
> -
>       IoRegisterDriverReinitialization    (NTOSKRNL.EXE.@)
>
> */
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec

b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 422d575926..1a6139de61 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -428,7 +428,7 @@
>  @ stub IoReadPartitionTableEx
>  @ stub IoReadTransferCount
>  @ stub IoRegisterBootDriverReinitialization
> -@ stub IoRegisterDeviceInterface
> +@ stdcall IoRegisterDeviceInterface(ptr ptr ptr ptr)
>  @ stdcall IoRegisterDriverReinitialization(ptr ptr ptr)
>  @ stdcall IoRegisterFileSystem(ptr)
>  @ stub IoRegisterFsRegistrationChange
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 0cd1673dbe..cd057d5a9f 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -1405,6 +1405,7 @@ PDEVICE_OBJECT WINAPI

IoGetRelatedDeviceObject(PFILE_OBJECT);
> void      WINAPI IoInitializeIrp(IRP*,USHORT,CCHAR);
>  VOID      WINAPI

IoInitializeRemoveLockEx(PIO_REMOVE_LOCK,ULONG,ULONG,ULONG,ULONG);
> void      WINAPI

IoInvalidateDeviceRelations(PDEVICE_OBJECT,DEVICE_RELATION_TYPE);
> +NTSTATUS  WINAPI IoRegisterDeviceInterface(PDEVICE_OBJECT,const

GUID*,PUNICODE_STRING,PUNICODE_STRING);
> void      WINAPI IoReleaseCancelSpinLock(KIRQL);
>  NTSTATUS  WINAPI IoSetDeviceInterfaceState(UNICODE_STRING*,BOOLEAN);
>  NTSTATUS  WINAPI IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);