Re: [RFC PATCH 3/9] setupapi: Implement SetupDiGetDevicePropertyW.

[Zebediah Figura]

On 12/18/18 10:20 AM, Zhiyi Zhang wrote:
> Signed-off-by: Zhiyi Zhang
>
> dlls/setupapi/devinst.c       |  69 ++++++++++++-
>   dlls/setupapi/tests/devinst.c | 181 +++++++++++++++++++++++++++++++++-
>   2 files changed, 244 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c
> index 556bba4684..2a71b59518 100644
> --- a/dlls/setupapi/devinst.c
> +++ b/dlls/setupapi/devinst.c
> @@ -3848,9 +3848,70 @@ BOOL WINAPI SetupDiGetDevicePropertyW(HDEVINFO info_set, PSP_DEVINFO_DATA info_d
>                   const DEVPROPKEY *prop_key, DEVPROPTYPE *prop_type, BYTE *prop_buff,
>                   DWORD prop_buff_size, DWORD *required_size, DWORD flags)
>   {
> - FIXME("%p, %p, %p, %p, %p, %d, %p, 0x%08x stub\n", info_set, info_data, prop_key,
> -
>           prop_type, prop_buff, prop_buff_size, required_size, flags);
> - static const WCHAR formatW[] = {'\', '%', '0', '4', 'X', 0};
> - WCHAR property_hkey_path[55] = {'P', 'r', 'o', 'p', 'e', 'r', 't', 'i', 'e', 's', '\'};
> - DWORD reg_key_type;
> - DWORD value_size;
> - LSTATUS ls;
> - struct device *device;
> - SetLastError(ERROR_NOT_FOUND);
> - return FALSE;
> - TRACE("%p, %p, %p, %p, %p, %d, %p, %#x\n", info_set, info_data, prop_key, prop_type, prop_buff, prop_buff_size,
> -
>      required_size, flags);
> -
> - if (!(device = get_device(info_set, info_data)))
> -
>    return FALSE;
> -
> - if (!prop_key)
> - {
> -
>    SetLastError(ERROR_INVALID_DATA);
> -
>    return FALSE;
> - }
> -
> - if (!prop_type || (!prop_buff && prop_buff_size))
> - {
> -
>    SetLastError(ERROR_INVALID_USER_BUFFER);
> -
>    return FALSE;
> - }
> -
> - if (flags)
> - {
> -
>    SetLastError(ERROR_INVALID_FLAGS);
> -
>    return FALSE;
> - }

Same comments as regards patch 2/9.
> -
> - SETUPDI_GuidToString(&prop_key->fmtid, property_hkey_path + 11);
> - sprintfW(property_hkey_path + 49, formatW, prop_key->pid);
> -
> - value_size = prop_buff_size;
> - ls = RegGetValueW(device->key, property_hkey_path, NULL, RRF_RT_ANY, &reg_key_type, prop_buff, &value_size);

Do we really want to use RegGetValue() here and not RegQueryValueEx()? 
This would need tests at least. (E.g. is a string value null-terminated? 
Does REG_EXPAND_SZ auto-expand?)
> -
> - switch (ls)
> - {
> - case ERROR_FILE_NOT_FOUND:
> -
>    *prop_type = DEVPROP_TYPE_EMPTY;
> -
>    if (required_size)
> -
>        *required_size = 0;
> -
>    SetLastError(ERROR_NOT_FOUND);
> -
>    return FALSE;
> - default:

Is there a reason why this is a switch and not a simple if-else? 
Moreover, I'd expect we'd want to handle more errors than just 
ERROR_FILE_NOT_FOUND (e.g. ERROR_ACCESS_DENIED). I'd guess the above 
code path should be taken except for ERROR_SUCCESS and ERROR_MORE_DATA.
> -
>    *prop_type = 0xffff & reg_key_type;
> -
>    if (required_size)
> -
>        *required_size = value_size;
> -
> -
>    if (prop_buff_size < value_size)
> -
>    {
> -
>        SetLastError(ERROR_INSUFFICIENT_BUFFER);
> -
>        return FALSE;
> -
>    }
> -
>    else if (ls == ERROR_UNSUPPORTED_TYPE)
> -
>    {
> -
>        SetLastError(NO_ERROR);
> -
>        return TRUE;
> -
>    }

If you pass RRF_RT_ANY, this shouldn't even happen, should it?
> -
>    else
> -
>    {
> -
>        SetLastError(ls);
> -
>        return FALSE;
> -
>    }
> - }
> }
>
> diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c
> index 274c28745c..6928415c76 100644
> --- a/dlls/setupapi/tests/devinst.c
> +++ b/dlls/setupapi/tests/devinst.c
> @@ -39,6 +39,7 @@ static GUID guid = {0x6a55b5a4, 0x3f65, 0x11db, {0xb7,0x04,0x00,0x11,0x95,0x5c,0
>   static GUID guid2 = {0x6a55b5a5, 0x3f65, 0x11db, {0xb7,0x04,0x00,0x11,0x95,0x5c,0x2b,0xdb}};
>
> BOOL (WINAPI *pSetupDiSetDevicePropertyW)(HDEVINFO, PSP_DEVINFO_DATA, const DEVPROPKEY *, DEVPROPTYPE, const BYTE *, DWORD, DWORD);
> +BOOL (WINAPI *pSetupDiGetDevicePropertyW)(HDEVINFO, PSP_DEVINFO_DATA, const DEVPROPKEY *, DEVPROPTYPE *, BYTE *, DWORD, DWORD *, DWORD);
>
> static LSTATUS devinst_RegDeleteTreeW(HKEY hKey, LPCWSTR lpszSubKey)
>   {
> @@ -418,15 +419,19 @@ static void test_device_property(void)
>       HDEVINFO set;
>       SP_DEVINFO_DATA device = {sizeof(device)};
>       DEVPROPKEY key;
> -
>
> DEVPROPTYPE type;
> -
>
> DWORD size;
> -
>
> BYTE buffer[256];
>  DWORD err;
>  BOOL ret;
>
> hmod = LoadLibraryA("setupapi.dll");
>  pSetupDiSetDevicePropertyW = (void *)GetProcAddress(hmod, "SetupDiSetDevicePropertyW");
> -
>
> pSetupDiGetDevicePropertyW = (void *)GetProcAddress(hmod, "SetupDiGetDevicePropertyW");
> - if (!pSetupDiSetDevicePropertyW)
> - if (!pSetupDiSetDevicePropertyW || !pSetupDiGetDevicePropertyW)
>  {
> -
>    win_skip("SetupDiSetDevicePropertyW() are only available on vista+, skipping tests.\n");
> -
>    win_skip("SetupDi{Set,Get}DevicePropertyW() are only available on vista+, skipping tests.\n");
>     FreeLibrary(hmod);
>     return;
> }
>
> @@ -592,6 +597,178 @@ static void test_device_property(void)
>       ok(!ret, "Expect failure\n");
>       ok(err == ERROR_NOT_FOUND, "Expect last error %#x, got %#x\n", ERROR_NOT_FOUND, err);
> -
> - /* SetupDiGetDevicePropertyW */
> - ret = pSetupDiSetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, DEVPROP_TYPE_STRING, (const BYTE *)valueW, sizeof(valueW), 0);
> - ok(ret, "Expect success\n");
> -
> - /* #1 Null device info list */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(NULL, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_HANDLE, "Expect last error %#x, got %#x\n", ERROR_INVALID_HANDLE, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #2 Null device */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, NULL, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_PARAMETER, "Expect last error %#x, got %#x\n", ERROR_INVALID_PARAMETER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #3 Null property key */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, NULL, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_DATA, "Expect last error %#x, got %#x\n", ERROR_INVALID_DATA, err);
> - /* Type unchanged on win8+ */
> - ok(type == DEVPROP_TYPE_STRING || type == DEVPROP_TYPE_EMPTY, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #4 Null property type */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, NULL, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_USER_BUFFER, "Expect last error %#x, got %#x\n", ERROR_INVALID_USER_BUFFER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #5 Null buffer */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, NULL, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_USER_BUFFER, "Expect last error %#x, got %#x\n", ERROR_INVALID_USER_BUFFER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #6 Null buffer with zero size and wrong type */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_UINT64;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, NULL, 0, &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INSUFFICIENT_BUFFER, "Expect last error %#x, got %#x\n", ERROR_INSUFFICIENT_BUFFER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == sizeof(valueW), "Expect size %d, got %d\n", sizeof(valueW), size);
> -
> - /* #7 Zero buffer size */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, 0, &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INSUFFICIENT_BUFFER, "Expect last error %#x, got %#x\n", ERROR_INSUFFICIENT_BUFFER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == sizeof(valueW), "Expect size %d, got %d\n", sizeof(valueW), size);
> -
> - /* #8 Null required size */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), NULL, 0);
> - err = GetLastError();
> - ok(ret, "Expect success\n");
> - ok(err == NO_ERROR, "Expect last error %#x, got %#x\n", NO_ERROR, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> -
> - /* #9 Flags not zero */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 1);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INVALID_FLAGS, "Expect last error %#x, got %#x\n", ERROR_INVALID_FLAGS, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #10 Non-existent property key */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_HardwareIds, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_NOT_FOUND, "Expect last error %#x, got %#x\n", ERROR_NOT_FOUND, err);
> - /* Type unchanged on win8+ */
> - ok(type == DEVPROP_TYPE_EMPTY || broken(type == DEVPROP_TYPE_STRING), "Expect type %#x, got %#x\n", DEVPROP_TYPE_EMPTY, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #11 Wrong property key type */
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_UINT64;
> - size = 0;
> - memset(buffer, 0, sizeof(buffer));
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(ret, "Expect success\n");
> - ok(err == NO_ERROR, "Expect last error %#x, got %#x\n", NO_ERROR, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == sizeof(valueW), "Expect size %d, got %d\n", sizeof(valueW), size);
> - ok(!lstrcmpW((WCHAR *)buffer, valueW), "Expect buffer %s, got %s\n", wine_dbgstr_w(valueW), wine_dbgstr_w((WCHAR *)buffer));
> -
> - /* #12 Get null property value */
> - ret = pSetupDiSetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, DEVPROP_TYPE_STRING, (const BYTE *)valueW, sizeof(valueW), 0);
> - ok(ret, "Expect success\n");
> - ret = pSetupDiSetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, DEVPROP_TYPE_NULL, NULL, 0, 0);
> - ok(ret, "Expect success\n");
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_NOT_FOUND, "Expect last error %#x, got %#x\n", ERROR_NOT_FOUND, err);
> - /* Type unchanged on win8+ */
> - ok(type == DEVPROP_TYPE_EMPTY || broken(type == DEVPROP_TYPE_STRING), "Expect type %#x, got %#x\n", DEVPROP_TYPE_EMPTY, type);
> - ok(size == 0, "Expect size %d, got %d\n", 0, size);
> -
> - /* #13 Insufficient buffer size */
> - ret = pSetupDiSetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, DEVPROP_TYPE_STRING, (const BYTE *)valueW, sizeof(valueW), 0);
> - ok(ret, "Expect success\n");
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(valueW) - 1, &size, 0);
> - err = GetLastError();
> - ok(!ret, "Expect failure\n");
> - ok(err == ERROR_INSUFFICIENT_BUFFER, "Expect last error %#x, got %#x\n", ERROR_INSUFFICIENT_BUFFER, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == sizeof(valueW), "Expect size %d, got %d\n", sizeof(valueW), size);
> -
> - /* #14 Normal */
> - ret = pSetupDiSetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, DEVPROP_TYPE_STRING, (const BYTE *)valueW, sizeof(valueW), 0);
> - ok(ret, "Expect success\n");
> - SetLastError(0xdeadbeef);
> - type = DEVPROP_TYPE_STRING;
> - size = 0;
> - memset(buffer, 0, sizeof(buffer));
> - ret = pSetupDiGetDevicePropertyW(set, &device, &DEVPKEY_Device_FriendlyName, &type, buffer, sizeof(buffer), &size, 0);
> - err = GetLastError();
> - ok(ret, "Expect success\n");
> - ok(err == NO_ERROR, "Expect last error %#x, got %#x\n", NO_ERROR, err);
> - ok(type == DEVPROP_TYPE_STRING, "Expect type %#x, got %#x\n", DEVPROP_TYPE_STRING, type);
> - ok(size == sizeof(valueW), "Expect size %d, got %d\n", sizeof(valueW), size);
> - ok(!lstrcmpW((WCHAR *)buffer, valueW), "Expect buffer %s, got %s\n", wine_dbgstr_w(valueW), wine_dbgstr_w((WCHAR *)buffer));
> -
> ret = SetupDiRemoveDevice(set, &device);
> ok(ret, "Got unexpected error %#x.\n", GetLastError());