-----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 :-) .
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.
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.
-
Jonathan Vollebregt -
Stefan Dösinger