[PATCH v3] ntdll: Add RtlGetDeviceFamilyInfoEnum and RtlConvertDeviceFamilyInfoToString
Kacper Rączy
gfw.kra at gmail.com
Mon Jul 4 17:03:10 CDT 2022
Thanks for the feedback.
> Wiadomość napisana przez Zebediah Figura (she/her) <zfigura at codeweavers.com> w dniu 03.07.2022, o godz. 22:05:
>
> Hello Kacper, thanks for the patch! I have a few comments and questions inlined.
>
>> On 6/23/22 15:56, Kacper Raczy wrote:
>> APIs introduced in Windows 10 (NT10.0).
>> Device form and family are hardcoded to Unknown and Windows.Desktop respectively.
>> Unit tests included in dlls/ntdll/tests/version.c
>> Signed-off-by: Kacper Raczy <gfw.kra at gmail.com>
>> ---
>
> Is there a known application which requires this patch? We tend to avoid introducing new code when not necessary, since it's more work to maintain and can cause regressions.
Microsoft’s GameServices libraries which are mostly used by games packaged as UWP in Microsoft Store - these libs contain Xbox specific segments, and use this APIs to differentiate between platforms. I’m aware that UWP support may not be priority right now, but this is first step to get UWP packaged games to even link/launch (although most of them may still not work, but thats something I’m also working on one step at a time).
>
>> dlls/ntdll/ntdll.spec | 2 +
>> dlls/ntdll/rtl.c | 2 +-
>> dlls/ntdll/tests/Makefile.in | 3 +-
>> dlls/ntdll/tests/version.c | 87 ++++++++++++++++++++++++++++++++++++
>> dlls/ntdll/version.c | 48 +++++++++++++++++++-
>> include/winnt.h | 13 ++++++
>> 6 files changed, 152 insertions(+), 3 deletions(-)
>> create mode 100644 dlls/ntdll/tests/version.c
>> diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec
>> index b1650ab4306..4539a9bd0ed 100644
>> --- a/dlls/ntdll/ntdll.spec
>> +++ b/dlls/ntdll/ntdll.spec
>> @@ -712,6 +712,8 @@
>> @ stdcall RtlGetCurrentProcessorNumberEx(ptr)
>> @ stdcall RtlGetCurrentTransaction()
>> @ stdcall RtlGetDaclSecurityDescriptor(ptr ptr ptr ptr)
>> +@ stdcall RtlGetDeviceFamilyInfoEnum(ptr ptr ptr)
>> +@ stdcall RtlConvertDeviceFamilyInfoToString(ptr ptr ptr ptr)
>> @ stdcall RtlGetElementGenericTable(ptr long)
>> # @ stub RtlGetElementGenericTableAvl
>> @ stdcall RtlGetEnabledExtendedFeatures(int64)
>
> There
>
>> diff --git a/dlls/ntdll/rtl.c b/dlls/ntdll/rtl.c
>> index 11067f44941..421cf377d32 100644
>> --- a/dlls/ntdll/rtl.c
>> +++ b/dlls/ntdll/rtl.c
>> @@ -2211,4 +2211,4 @@ char WINAPI RtlQueryProcessPlaceholderCompatibilityMode(void)
>> {
>> FIXME("stub\n");
>> return PHCM_APPLICATION_DEFAULT;
>> -}
>> +}
>> \ No newline at end of file
>
> This looks like a stray whitespace change; please try to avoid those.
>
>> diff --git a/dlls/ntdll/tests/Makefile.in b/dlls/ntdll/tests/Makefile.in
>> index 90deb5865f8..e64b70b15e4 100644
>> --- a/dlls/ntdll/tests/Makefile.in
>> +++ b/dlls/ntdll/tests/Makefile.in
>> @@ -25,5 +25,6 @@ C_SRCS = \
>> thread.c \
>> threadpool.c \
>> time.c \
>> + version.c \
>
> In general we also try to avoid adding a new test file just for a few tests. env.c is probably a reasonable place to put these.
Ok
>> virtual.c \
>> - wow64.c
>> + wow64.c
>> \ No newline at end of file
>
> Another stray whitespace change here.
>
>> diff --git a/dlls/ntdll/tests/version.c b/dlls/ntdll/tests/version.c
>> new file mode 100644
>> index 00000000000..296889efc8e
>> --- /dev/null
>> +++ b/dlls/ntdll/tests/version.c
>> @@ -0,0 +1,87 @@
>> +/*
>> + * Unit test suite for Rtl* Version API functions
>> + *
>> + * Copyright 2022 Kacper Rączy
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>> + */
>> +
>> +#include "ntdll_test.h"
>> +#include "wine/test.h"
>> +
>> +static const WCHAR DeviceForms[][32] = {
>> + L"Unknown", L"Phone", L"Tablet", L"Desktop", L"Notebook",
>> + L"Convertible", L"Detachable", L"All-in-One", L"Stick PC", L"Puck",
>> + L"Surface Hub", L"Head-mounted display", L"Industry handheld", L"Industry tablet", L"Banking",
>> + L"Building automation", L"Digital signage", L"Gaming", L"Home automation", L"Industrial automation",
>> + L"Kiosk", L"Maker board", L"Medical", L"Networking", L"Point of Service",
>> + L"Printing", L"Thin client", L"Toy", L"Vending", L"Industry other"
>> +};
>> +
>> +static void test_RtlGetDeviceFamilyInfoEnum(void)
>> +{
>> + DWORD family, form;
>> + ULONGLONG uap;
>> + RTL_OSVERSIONINFOEXW version;
>> +
>> + RtlGetVersion(&version);
>> + RtlGetDeviceFamilyInfoEnum(&uap, &family, &form);
>> + ok( ((uap >> 48) & 0xffff) == version.dwMajorVersion,
>> + "First 16-bit chunk of UAP does not match major system version %llx\n", uap );
>> + ok( ((uap >> 32) & 0xffff) == version.dwMinorVersion,
>> + "Second 16-bit chunk of UAP does not match minor system version %llx\n", uap );
>> + ok( ((uap >> 16) & 0xffff) == version.dwBuildNumber,
>> + "Third 16-bit chunk of UAP does not match build number %llx\n", uap );
>> + ok( family <= DEVICEFAMILYINFOENUM_MAX,
>> + "Device family is not valid: %lx\n", family );
>> + ok( form <= DEVICEFAMILYDEVICEFORM_MAX,
>> + "Device form is not valid: %lx\n", form );
>> +}
>> +
>> +static void test_RtlConvertDeviceFamilyInfoToString(void)
>> +{
>> + DWORD family_bufsize = 0, form_bufsize = 0;
>> + WCHAR *family, *form;
>> + BOOL form_valid = FALSE;
>> +
>> + RtlConvertDeviceFamilyInfoToString(&family_bufsize, &form_bufsize, NULL, NULL);
>> + ok( family_bufsize == sizeof(WCHAR) * 16, /* Windows.Desktop length */
>> + "Device family bufsize does not match: %lu\n", family_bufsize );
>> + ok( form_bufsize > 0, "Device form bufsize is invalid: %lu\n", form_bufsize );
>> +
>> + family = (WCHAR*)malloc(family_bufsize);
>> + form = (WCHAR*)malloc(form_bufsize);
>
> No need to cast these.
>
>> + RtlConvertDeviceFamilyInfoToString(&family_bufsize, &form_bufsize, family, form);
>> + ok( wcscmp(family, L"Windows.Desktop") == 0,
>> + "Device family string is not equal to Windows.Desktop: %ls\n", family );
>> + /* Device form depends on OEM setting in registry,
>> + * lets check all possible values to make it work on Windows */
>> + for (int i = 0; i < sizeof(DeviceForms) / sizeof(WCHAR*); i++) {
>> + if (wcscmp(form, DeviceForms[i]) == 0) {
>> + form_valid = TRUE;
>> + break;
>> + }
>> + }
>> + ok( form_valid, "Device form string is not valid or known: %ls\n", form);
>> +
>> + free(family);
>> + free(form);
>> +}
>> +
>> +START_TEST(version)
>> +{
>> + test_RtlGetDeviceFamilyInfoEnum();
>> + test_RtlConvertDeviceFamilyInfoToString();
>> +}
>> diff --git a/dlls/ntdll/version.c b/dlls/ntdll/version.c
>> index 492c24cc636..d474635ac2f 100644
>> --- a/dlls/ntdll/version.c
>> +++ b/dlls/ntdll/version.c
>> @@ -609,7 +609,6 @@ void WINAPI RtlGetNtVersionNumbers( LPDWORD major, LPDWORD minor, LPDWORD build
>> if (build) *build = (0xF0000000 | current_version->dwBuildNumber);
>> }
>> -
>> /******************************************************************************
>> * RtlGetNtProductType (NTDLL.@)
>> */
>
> Stray whitespace change.
>
>> @@ -760,6 +759,53 @@ NTSTATUS WINAPI RtlVerifyVersionInfo( const RTL_OSVERSIONINFOEXW *info,
>> return STATUS_SUCCESS;
>> }
>> +/*********************************************************************
>> + * RtlGetDeviceFamilyInfoEnum (NTDLL.@)
>> + *
>> + * NOTES
>> + * Introduced in Windows 10 (NT10.0)
>
> I don't think there's any need for this comment. Same below.
I think the APIs above have those. It is convention for new APIs?
>
>> + */
>> +void WINAPI RtlGetDeviceFamilyInfoEnum( ULONGLONG *uap_info, DWORD *device_family, DWORD *device_form )
>> +{
>
> Can you please add a TRACE to any new functions?
Ok
>
>> + if (device_form)
>> + *device_form = DEVICEFAMILYDEVICEFORM_UNKNOWN;
>
> Does Windows really return this?
This seem to depend on OEM - there’s special registry key for that Software\Microsoft\Windows NT\CurrentVersion\OEM\DeviceForm. If value is not there, windows returns Unknown - this is also what is happening on desktop PCs with OS manually installed.
I’ve seen this API used to differentiate whether host is an Xbox or PC, that’s why I thought leaving it fixed to values returned by most PCs is sufficient.
>
>> + if (device_family)
>> + *device_family = DEVICEFAMILYINFOENUM_DESKTOP;
>> + if (!uap_info)
>> + return;
>
> Is it valid to call this function with NULL parameters?
As I was testing this on windows, it’s possible to only pass some of these parameters, if you don’t need all of them.
>> +
>> + /**
>> + * UAP info is 64 bit unsigned integer which contains four 16-bit chunks:
>> + * 1. os version major
>> + * 2. os version minor
>> + * 3. current build number
>> + * 4. update build revision
>> + */
>> + *uap_info = 0;
>> + *uap_info |= (((ULONGLONG)current_version->dwMajorVersion & 0xffff) << 48); /* os version major */
>> + *uap_info |= (((ULONGLONG)current_version->dwMinorVersion & 0xffff) << 32); /* os version minor */
>> + *uap_info |= (((ULONGLONG)current_version->dwBuildNumber & 0xffff) << 16); /* current build number */
>
> These comments seem superfluous; they're just repeating field names.
Ok
>> + /* UBR not available */
>> +}
>> +
>> +/*********************************************************************
>> + * RtlConvertDeviceFamilyInfoToString (NTDLL.@)
>> + *
>> + * NOTES
>> + * Introduced in Windows 10 (NT10.0)
>> + */
>> +void WINAPI RtlConvertDeviceFamilyInfoToString( DWORD *device_family_bufsize, DWORD *device_form_bufsize,
>> + const WCHAR *device_family, const WCHAR *device_form)
>> +{
>> + DWORD device_family_len = (wcslen( L"Windows.Desktop" ) + 1) * sizeof(WCHAR);
>> + DWORD device_form_len = (wcslen( L"Unknown" ) + 1) * sizeof(WCHAR);
>
> Or more simply "sizeof". These can also be static const.
You’re right
>
>> + if (*device_family_bufsize >= device_family_len)
>> + wcscpy( device_family, L"Windows.Desktop" );
>> + if (*device_form_bufsize >= device_form_len)
>> + wcscpy( device_form, L"Unknown" ); > + *device_family_bufsize = device_family_len;
>> + *device_form_bufsize = device_form_len;
>> +}
>> /******************************************************************************
>> * VerSetConditionMask (NTDLL.@)
>> diff --git a/include/winnt.h b/include/winnt.h
>> index 87c4b4da92d..88948122b81 100644
>> --- a/include/winnt.h
>> +++ b/include/winnt.h
>> @@ -6224,6 +6224,19 @@ typedef struct _SYSTEM_CPU_SET_INFORMATION
>> } DUMMYUNIONNAME;
>> } SYSTEM_CPU_SET_INFORMATION, *PSYSTEM_CPU_SET_INFORMATION;
>> +/* Windows 10 Rtl apis */
>
> This comment seems superfluous. We don't sort by version.
Will remove unnecessary and self-explanatory comments then.
>
>> +
>> +#define DEVICEFAMILYINFOENUM_DESKTOP 0x00000003
>> +
>> +#define DEVICEFAMILYINFOENUM_MAX 0x00000011
>> +
>> +#define DEVICEFAMILYDEVICEFORM_UNKNOWN 0x00000000
>> +
>> +#define DEVICEFAMILYDEVICEFORM_MAX 0x00000021
>> +
>> +NTSYSAPI VOID WINAPI RtlGetDeviceFamilyInfoEnum(ULONGLONG*, DWORD*, DWORD*);
>> +NTSYSAPI VOID WINAPI RtlConvertDeviceFamilyInfoToString(DWORD*, DWORD*, const WCHAR*, const WCHAR*);
>
> My copy of the Windows SDK shows the latter function as returning DWORD, and having non-const WCHAR parameters. "const" doesn't make sense anyway since they're output.
You’re right
>> +
>> /* Threadpool things */
>> typedef DWORD TP_VERSION,*PTP_VERSION;
More information about the wine-devel
mailing list