[PATCH v3 1/2] winevulkan: Create JSON manifest and registry entry used by official Vulkan loader.

[Brendan Shanks]

Hi Jacek,

Thanks for the feedback.

> On Mar 23, 2020, at 4:50 PM, Jacek Caban <jacek at codeweavers.com> wrote:
> 
> Hi Brendan,
> 
> On 23.03.2020 20:16, Brendan Shanks wrote:
>> +static BOOL writestr(HANDLE handle, const char *str)
>> +{
>> +    DWORD written;
>> +    return WriteFile(handle, str, strlen(str), &written, NULL);
>> +}
> 
> 
> It may be a matter of taste, but it doesn't seem worth a helper. You could just do that with a single WriteFile() call.

My concern with calling WriteFile directly is that I would have to duplicate the string literals (once for the arg and once for calling strlen()). That would be ok for simple one-word strings, but these include whitespace and escape characters, and could easily get out of sync without being visually obvious.

> 
>> +    writestr(file, "{\r\n");
>> +    writestr(file, "    \"file_format_version\": \"1.0.0\",\r\n");
>> +    writestr(file, "    \"ICD\": {\r\n");
>> +    writestr(file, "        \"library_path\": \"winevulkan.dll\",\r\n");
> 
> 
> It may not be too important in this case, but it's usually better to use full path in such places, something like a result of GetModuleFileNameW(). What do real IDSs use?
> 

Here’s what the spec says:
"The "library_path" specifies either a filename, a relative pathname, or a full pathname to an ICD shared library file. If "library_path" specifies a relative pathname, it is relative to the path of the JSON manifest file. If "library_path" specifies a filename, the library must live in the system's shared object search path."

NVIDIA uses a relative path on Windows, and just a filename on Linux. I think you’re right that a full path would be best, I’ll add that with GetModuleFileName().

> 
>> +    writestr(file, "        \"api_version\": \"" WINE_VK_SPEC_VERSION "\"\r\n");
>> +    writestr(file, "    }\r\n");
>> +    writestr(file, "}\r\n");
>> +    CloseHandle(file);
>> +
>> +    result = RegCreateKeyExW(HKEY_LOCAL_MACHINE, vulkan_driversW,
>> +                             0, NULL, 0, KEY_SET_VALUE, NULL, &key, NULL);
>> +    if (result != ERROR_SUCCESS)
>> +        { ERR("Unable to create registry key.\n"); return E_UNEXPECTED; }
>> +
>> +    result = RegSetValueExW(key, path, 0, REG_DWORD, (const BYTE *)&zero, sizeof(zero));
>> +    if (result != ERROR_SUCCESS)
>> +        { ERR("Unable to set registry value.\n"); return E_UNEXPECTED; }
> 
> 
> Style aside, you leak a handle in second error path. Error handling could also be simplified a bit.
> 
> 
>> +HRESULT WINAPI DllUnregisterServer(void)
>> +{
>> +    WCHAR path[MAX_PATH];
>> +    LRESULT result;
>> +    HKEY key;
>> +
>> +    /* Remove the JSON manifest and registry key */
>> +    TRACE("\n");
>> +    GetSystemDirectoryW(path, ARRAY_SIZE(path));
>> +    lstrcatW(path, winevulkan_jsonW);
>> +    if (!DeleteFileW(path))
>> +        ERR("Unable to delete JSON file.\n");
>> +
>> +    result = RegOpenKeyExW(HKEY_LOCAL_MACHINE, vulkan_driversW,
>> +                           0, KEY_SET_VALUE, &key);
>> +    if (result != ERROR_SUCCESS)
>> +        { ERR("Unable to open registry key.\n"); return E_UNEXPECTED; }
> 
> 
> This is not really an error.
> 
> 
>> +
>> +    result = RegDeleteValueW(key, path);
>> +    if (result != ERROR_SUCCESS)
>> +        ERR("Unable to delete registry key.\n");
>> +
>> +    RegCloseKey(key);
>> +    return S_OK;
>> +}
> 
> 
> Those ERRs should we WARNs or removed. Eg. it's not really an error when it's ran with no winevulkan registered or if you run regsvr32 -u twice in a raw. We should report success and print no errors in such case.

Yes you’re right, I’ll fix all these.

Thanks,
Brendan