-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-11-04 21:15, schrieb Jonathan Vollebregt:
Style suggestion: you can make this an anonymous structure:
struct { HKEY key; const WCHAR *short_name; const WCHAR *long_name; } root_rels[] = { {HKEY_LOCAL_MACHINE, short_hklm, long_hklm}, {HKEY_CURRENT_USER, short_hkcu, long_hkcu}, {HKEY_CLASSES_ROOT, short_hkcr, long_hkcr}, {HKEY_USERS, short_hku, long_hku}, {HKEY_CURRENT_CONFIG, short_hkcc, long_hkcc}, };
Unless of course you need the structure definition elsewhere.
+static LSTATUS path_get_rootkey(const WCHAR *path, HKEY *out)
Since you're only returning two possible values (ERROR_SUCCESS or ERROR_BADKEY) you can make this return a HKEY *.
What do you think about this:
static LSTATUS path_get_key(const WCHAR *path, HKEY *out) { *out = path_get_rootkey(path); path = strchrW(path, '\'); if (path) path++; return RegOpenKeyW(*out, path, out); }
If my understanding of RegOpenKeyW is right this will give you ERROR_INVALID_HANDLE (winnt+) or ERROR_BADKEY (win9x) if the root key is invalid, ERROR_PATH_NOT_FOUND if the path is invalid, and ERROR_SUCCESS otherwise. So pretty much the behavior of your code in 1/3rd of the lines of code.
I think path_open would be a better name for this function.
You could also consider an additional BOOL create parameter to call RegCreateKeyW instead of RegOpenKeyW so you can reuse this code in reg_add.
My reading comprehension may be lacking again, but can't you just call RegDeleteTree(subkey, NULL)? You're also assuming the caller isn't trying to nuke HKEY_LOCAL_MACHINE :-) .
On 11/04/2014 10:48 PM, Stefan Dösinger wrote:
My reading comprehension may be lacking again, but can't you just call RegDeleteTree(subkey, NULL)? You're also assuming the caller isn't trying to nuke HKEY_LOCAL_MACHINE:-) .
That deletes everything below the key, but not the key itself, so I have to do it that way.
Reg is supposed to throw an error if the key ends in more than one backslash, RegCreateKey/RegOpenKey doesn't so it has to be checked manually.
The message with ERROR_INVALID_DATATYPE says it indicates the *data* has the wrong type, nothing about the type itself.
Given an incorrect type RegSetValueEx returns ERROR_ACCESS_DENIED, which isn't the most descriptive of error codes. Perhaps it would still be better to make our own more descriptive error code
Another possibility is that native assumes a default value string of "" if /d is not specified, which then produces behavior like an empty REG_SZ and fails to convert into a DWORD.
Nice catch! That fits perfectly! 29 loc less!
What should I do in default then? It would be a bad idea to return ERROR_SUCCESS
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-11-06 09:11, schrieb Jonathan Vollebregt:
This is unfortunate, I guess we'll have to live with parsing the key twice in this case. RegDeleteKey doesn't help either, it can only delete a subkey.
Do we have tests for the RegCreateKey / RegOpenKey behavior? I did not find any when I looked.
The message with ERROR_INVALID_DATATYPE says it indicates the *data* has the wrong type, nothing about the type itself.
Huh, I am not sure I understand what this means or what the difference is :-\ .
Agreed, ERROR_ACCESS_DENIED is a bit unfortunate. This is the behavior of Windows, not Wine's implementation, right?
For code that should be unreachable an ERR() would be enough. The completely unexpected already happened, any attempt to clean up after that will come up short anyway. The idea is that when ERRs are removed at compile time the compiler can remove the condition as well since it just covers an empty block of code.
If you want to make sure you abort you can add an assert(0).
Though if you put the conversion function into the type table you avoid the issue altogether.
On 11/06/2014 03:54 PM, Stefan Dösinger wrote:
I ran a quick one on testbot myself that confirmed it, but didn't write up a proper one.
Huh. After looking up the error message I can't find it. Perhaps I was looking at a different error. Yeah ERROR_INVALID_DATATYPE looks good, I'll use that one.
Speaking of - since the type_get_wchar and wchar_get_type functions return 2 states like path_get_rootkey, should I have them return pointers too? (Or DWORD -1 for error in the case of wchar_get_type)
Yeah, both windows and wine return ERROR_ACCESS_DENIED
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-11-06 17:33, schrieb Jonathan Vollebregt:
I ran a quick one on testbot myself that confirmed it, but didn't write up a proper one.
Please add one. It answers why the code is in here and not another place.
I'm OK if you send the advapi32 test after the reg.exe patches.
Yes. Generally speaking, don't use more parameters / return pointers than necessary. If there's only one reason why a pointer-returning function can fail you don't need another channel to communicate why it failed.
-
Jonathan Vollebregt -
Stefan Dösinger