Re: [PATCH resend] ntdll: Add RtlGetDeviceFamilyInfoEnum and RtlGetDeviceFamilyInfoEnum

[Kacper rączy]

Thanks for the feedback. Will add some unit tests later today.

As for RtlConvertDeviceFamilyInfoToString you’re right, size should be in bytes and on Windows it copies the output only if provided buffer sizes are sufficient. Otherwise only buffer sizes are assigned.
> Wiadomość napisana przez Paul Gofman  w dniu 22.06.2022, o godz. 16:16:
>
> Hi, thanks for the patch!
>
> This could use a test which includes checks for return data. Without that it is hard to say if the patch is correct. Version data is not constant of course but it could be checked if it matches RtlGetVersion(). A few thoughts regarding on some possible improvements to the patch regardless (once there is a test which verifies it general correctness):
>
> On 6/22/22 08:49, Kacper Raczy wrote:
> > +/*********************************************************************
> > -
> > - RtlGetDeviceFamilyInfoEnum (NTDLL.@)
> > -
> > -
> > -
> > - NOTES
> > -
> > - Introduced in Windows 10 (NT10.0)
> > - */
> >
> > +VOID WINAPI RtlGetDeviceFamilyInfoEnum(ULONGLONG *uap_info, DWORD *device_family, DWORD *device_form)
>
> I think we currently prefer 'void' over 'VOID' and such.
>
> Formatting: spaces around function arguments (see, e. g., RtlVerifyVersionInfo above).
> > +{
> > - ULONGLONG uap_info_temp;
> > -
>
> Do you really need this temporary variable, any reason not to use *uap_info directly?
> > - if (device_form != NULL)
>
> We usually check such just as 'if (device_form) ...'
> > - uap_info_temp |= (((ULONGLONG) current_version->dwMajorVersion & 0xffff) << 48); /* os version major */
>
> Space after (ULONGLONG)
> > - uap_info_temp |= (((ULONGLONG) current_version->dwMinorVersion & 0xffff) << 32); /* os version minor */
> > - uap_info_temp |= (((ULONGLONG) current_version->dwBuildNumber & 0xffff) << 16); /* current build number */
> > - /* UBR not available */
> > - *uap_info = uap_info_temp;
> >
> > +}
> > -
> >
> > +/*********************************************************************
> > -
> > - RtlConvertDeviceFamilyInfoToString (NTDLL.@)
> > -
> > -
> > -
> > - NOTES
> > -
> > - Introduced in Windows 10 (NT10.0)
> > - */
> >
> > +VOID WINAPI RtlConvertDeviceFamilyInfoToString(
> > - DWORD *device_family_bufsize,
> > - DWORD *device_form_bufsize,
> > - const LPWSTR device_family,
> > - const LPWSTR device_form
>
> Parameters formatting. Also I think we currently prefer const WCHAR * over LPWSTR.
> > +) {
> > - if (device_family_bufsize != NULL)
> > -
> >    *device_family_bufsize = (DWORD)wcslen(DEVICEFAMILYDEVICEFORM_DESKTOP_STR) + 1;
> > - if (device_family != NULL)
> > -
> >    wcscpy(device_family, DEVICEFAMILYDEVICEFORM_DESKTOP_STR);
>
> This looks suspicious. Does Windows really only sets buffer size but does not check it before copying the output? Also, not apparent without the test if bufsize is wide character count or buffer size in bytes.
>
> Formatting; " wcscpy(device_family, DEVICEFAMILYDEVICEFORM_DESKTOP_STR);" -> " wcscpy( device_family, DEVICEFAMILYDEVICEFORM_DESKTOP_STR );"
> > - if (device_form_bufsize != NULL)
> > -
> >    *device_form_bufsize = (DWORD)wcslen(DEVICEFAMILYINFOENUM_DESKTOP_STR) + 1;
>
> (DWORD) cast is not needed