Re: [PATCH 3/7] reg: Add path/key conversion functions
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-11-04 21:15, schrieb Jonathan Vollebregt:
+typedef struct +{ + HKEY key; + const WCHAR *short_name; + const WCHAR *long_name; +} hkey_rel; 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 *.
+static LSTATUS path_get_key(const WCHAR *path, HKEY *out) +{ + HKEY k; + LONG err = path_get_rootkey(path, &k); + if (err != ERROR_SUCCESS) + return err; + + path = strchrW(path, '\\'); + if (path) + path++; + + err = RegOpenKeyW(k, path, &k); + if (err != ERROR_SUCCESS) + return err; + + *out = k; + return ERROR_SUCCESS; } 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.
+ err = path_get_key(key_name, &subkey); + if (err != ERROR_SUCCESS) { - reg_message(STRING_INVALID_KEY); - return 1; - } - p++; - - root = get_rootkey(key_name); - if (!root) - { - reg_message(STRING_INVALID_KEY); + reg_print_error(err); return 1; }
@@ -306,21 +341,19 @@ static int reg_delete(WCHAR *key_name, WCHAR *value_name, BOOL value_empty, /* Delete subtree only if no /v* option is given */ if (!value_name && !value_empty && !value_all) { - if (RegDeleteTreeW(root,p)!=ERROR_SUCCESS) + HKEY root; + err = path_get_rootkey(key_name, &root); + err = RegDeleteTreeW(root, strchrW(key_name, '\\') + 1); 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 :-) .
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUWUmSAAoJEN0/YqbEcdMwjmQP/iuYzlfloARtQOxq6zCyHAsT bBFIHVwZoiB3/e4UvDcnRvcdJGvTM/LNu2LBczsFhOEfidSGsMYbbPAm2bEP5cJE FAcy2bMTpiaPJx957dbimhEQuSreRx1OOdzH6jK74g+d/WJ3fHy0RHh9QXY0Rpe+ JLsPDOpau2gDs6HoKNvF9yVPM32sa/BWJGtLqGFhQngmzIye9WmBEVMX+c6w9q9N gxc7uiDBYXMSi4D2rGE5HwQJq3AM6InTOKkyAkDZv6EObvUeZ0JQKtFlC6tW3z0x aXTzs9RjvzS2er5ULPyxYV3QxBynRzI80VoWq5p3Z4M34dIaSsJeuWlhTWiIktzl U+M7G1Ukk3owmmXtYC0nBOIBq2IEzRzL28nXwbz3h8EUp1kYzwsZ9VpP6NitOP+6 E2yywBEDIrJL22RNSt79GLKlnLKpBNitWq5xCoWdXE625pBDSEH4cKlE5Afwi7j7 /jNGjxRv4Rre6+PcgrPLq3JSaEDHAs+tKuBvkJWgCKCc45KkaGiiwS5Tp32odqdA Q75F+8Epolx00aA948PwJ827VMtMT6U6z79ypluI5tgHuldhS73pmg9PvLtVfYG6 TlexSFUC1eq0FJWwS8yxY2HO20ZbQpOjpmrxR0mu2ACcFfdH8Ut6dspB5g2ttrGU XgXrPCumoV8HxXzwDcMu =B2eH -----END PGP SIGNATURE-----
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.
+static BOOL sane_path(const WCHAR *key) +{ + int i = strlenW(key); + + if (i < 3 || (key[i - 1] == '\\' && key[i - 2] == '\\')) + { + reg_print_error(ERROR_BADKEY); + return FALSE; + } I think I have asked this a few times, please forgive me if I forgot the answer: Did you test if RegAddKey and friends handle this for you? dlls/advapi32/tests/registry.c would profit from some tests for the error cases you have in programs/reg/tests/reg.c .
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.
+#define ERROR_INVALID_TYPE 20001 What does RegSetValueEx return if you pass an invalid type? The API makes this error condition possible, so there's likely a return value for it. (Yeah, I know, it's not your fault advapi32/tests has no test for this. If I understand server/registry.c correctly such a call succeeds and creates a blob with an undefined type. Still needs tests)
ERROR_INVALID_DATATYPE may be a candidate.
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!
+ default: + return ERROR_UNSUPPORTED_TYPE; I think this is a bad place to catch unsupported data types. "type" comes from one of your own functions, namely wchar_get_type(). If reg.exe shouldn't support any of the data types not explicitly handled here then wchar_get_type should already return an error in this case. Otherwise you can make this a FIXME("Add support for type %u\n" type);
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:
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. 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.
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. 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 :-\ .
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 Agreed, ERROR_ACCESS_DENIED is a bit unfortunate. This is the behavior of Windows, not Wine's implementation, right?
I think this is a bad place to catch unsupported data types. "type" comes from one of your own functions, namely wchar_get_type(). If reg.exe shouldn't support any of the data types not explicitly handled here then wchar_get_type should already return an error in this case. Otherwise you can make this a FIXME("Add support for type %u\n" type);
What should I do in default then? It would be a bad idea to return ERROR_SUCCESS 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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUW4uuAAoJEN0/YqbEcdMw4JQP/1h4Cg5/PlzgJCsKcEOzxlPc +A3M5St152rhcFQPmTM58QcwlGUBiZLMB1h/wmFYk+7fvADjLhv64BzFMvnhQ/B+ vIYDJBhcfAE0571W2H31bx76Rfl7BxviCWMTH+mUvI26okRBPbaQLC3TtwxaOaPO +UlRVq1F8y8q6fYqQ3qL/fHjml+swFf5n0vCyHMHFIDP+cBGzFvJmveW+HQowqTQ W/+c0thhj0+lekcZXQMNr4btfamhOcqU4D2YTEcFIRVg7+7QG0U89nri1C7S4DyW ZQh2c7T4P5VD+bv8KUgpowJmB91oZ3hv7RMyrLKdPKk1JKPs9UniA2T4BvPcEt1m i7k5QXOTIzYoaEccjaMEVnhOHExkhaiIZoJsMSOCoPF2jvzIDYL9f/4uCiLCKbnq 2lMilQ2k9PWxFI2JiuEeci8htAZVssyXtMZXUMjuWzoc2dkRAmhYOwWgSaSuZp+N chklFB+jwJLPtb3hcdzn4xAyx4ytECyknwoSt5b26iIx2Xlq6M4YYlVFzhpQ3P7M m/srFGITzSs26vo6loTAfBRrcRIAHVvvGxum5SLGvdf5ofPM6oeYfb6ejJ3bA06R JuShvacl0YdhTxwD+uziogVB3NbLPji0o2l3O7tCxHeI82NIihm09fOZi8G6f9PM BsrC/GG8E7Bn43Cv+j4A =4Il4 -----END PGP SIGNATURE-----
On 11/06/2014 03:54 PM, Stefan Dösinger wrote:
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. Do we have tests for the RegCreateKey / RegOpenKey behavior? I did not find any when I looked.
I ran a quick one on testbot myself that confirmed it, but didn't write up a proper one.
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 :-\ .
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)
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 Agreed, ERROR_ACCESS_DENIED is a bit unfortunate. This is the behavior of Windows, not Wine's implementation, right?
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.
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) 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.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUW6exAAoJEN0/YqbEcdMwdu0P/0dVyml7/3S4I191P3H5XGn/ SA9f9Goolqw8SYi/aSi+K9DlvLaXjyLZEm0Pf3Vmbgg6uWDm/GV+OPqiV/ILxoRl crNlHzz8vfT57bXck4K+Qi/xapTWoJy+F2xL4SScShuR82UMbUlYfNlBYx0t9upf R1tBtsLRNjjkE9mZ7y+Qyipi9lYij7WuEuzC+YIlbR6hUuZ24U4nEbUtZDY3B1Z8 7lOdfriyUge4gjTWbmqC1MQJD9BbDvlW/4s33q5PO+JTtDbrlqRz0zygMY5uxASz zq8uz7/u3ff9UKmqyWWGxuwLajDdzqYkKCEvnzcjX78euDBhwdyoCVkJ0h67+JEz NZtdbnTDe+pdqNWslklf9vYuaxH3IuTgPz1FPJ8gih1oSlCNWWNEDj1VYny/F8mK QeC4vinN7KimqC+0GTKKT353hFIWpmA0ug6ochSdVJMi0S8APbbmE/phInOBzAGQ LKFa9MxbAEUDQyp3tVtqaHmqBjPExuJtLKd/ZwozhPmRG+CChLTA4ikhNgbqyOe3 OnwpiO4/nW/tUulZTkZ9Jq7tB6Lj43UBdLlLc2td0YrL7ARZ8sHde2KYiUMaXnIn q8jxc8uvXMCgdZcOqmfSiV8Ot8vY8USyKYvWfSdosf9uZDymxY6CI0FmpQB2KAbc nZ3aaFyVQDXwzd+trpgx =NBtH -----END PGP SIGNATURE-----
participants (2)
-
Jonathan Vollebregt -
Stefan Dösinger