-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
This patch is a lot more readable than the past version IMO. I'll look at the other patches tomorrow.
Am 2014-10-27 13:10, schrieb Jonathan Vollebregt:
+static WCHAR *sanitize_path(WCHAR *key){
Inconsistent curly bracket placement. The other patches have more cases of that.
elsereg_message(STRING_INVALID_KEY);
Doesn't get_rootkey() implicitly take care of this? I don't see why you need an extra check for a single leading backslash.
There's one (intended?) side effect of this check though: It ensures that the code below doesn't crash on the input string "\".
- i = strlenW(key);
- if (key[i - 1] == '\')
key[i - 1] = 0;
Why do you need this? The existing code and the todo_wine test suggests that RegCreateKey accepts any number of trailing slashes. Adding a test for this in dlls/advapi32/tests/registry.c would be a good idea.
This is the only case where you modify the input string. If it indeed is not necessary you can change the parameter to a const WCHAR * and return a BOOL.
Inconsistent curly bracket placement. The other patches have more cases of that.
Should the arrays also have the opening brace on it's own line? A quick grep shows both styles for arrays in wine.
elsereg_message(STRING_INVALID_KEY);Doesn't get_rootkey() implicitly take care of this? I don't see why you need an extra check for a single leading backslash.
There's one (intended?) side effect of this check though: It ensures that the code below doesn't crash on the input string "\".
The tests show that having any backslash at the start results in an error. That particular if just picks which error message to show.
- i = strlenW(key);
- if (key[i - 1] == '\')
key[i - 1] = 0;Why do you need this? The existing code and the todo_wine test suggests that RegCreateKey accepts any number of trailing slashes. Adding a test for this in dlls/advapi32/tests/registry.c would be a good idea.
reg/tests/reg.c:109 shows only the winXP version accepts multiple trailing backslashes.
The reason for stripping a single trailing backslash was for output consistency with query (As yet unimplemented) Specifically whether query outputs "HKEY_CURRENT_USER\key" or "HKEY_CURRENT_USER\key\" as the key name.
Should I still make it const and handle the backslash stripping in a different function?
+static inline int path_rootname_cmp(const WCHAR *path, const WCHAR *name) +{
- DWORD length = strlenW(name);
- return (strncmpiW(path, name, length) == 0 && (path[length] == 0 || path[length] == '\'));
+}
I don't like the names "path" and "name". I wondered why you're relying on the length of name. A better name for "path" could be "input", but I can't think of anything for "name".
Because the input is the full path we have to stop comparing after the root key or it will always return 0 unless it's passed a root key.
I think the "path" parameter name makes this more obvious, but I could throw a comment in if that helps.
- path = strchrW(path, '\');
- if (!path)
return k;I see that this is why you need the trailing backslash elimination in the previous patch. But still, do you need this? What happens when you call e.g. RegOpenKey(HKEY_CURRENT_USER, "\", &k)? If it returns HKEY_CURRENT_USER or a handle that points to HKEY_CURRENT_USER you're fine. Similarly for RegOpenKey(HKEY_CURRENT_USER, "", &k).
Actually this solves 2 separate problems. RegOpenKey takes the path after the root key but the path provided includes the root key, so strchrW skips the root key in the path in advance of passing it to RegOpenKey.
On the other hand, if someone inputs only a root key IE "HKCU" it will return NULL. We already have the key from path_get_rootkey, so we just return it here.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-28 12:21, schrieb Jonathan Vollebregt:
Inconsistent curly bracket placement. The other patches have more cases of that.
Should the arrays also have the opening brace on it's own line? A quick grep shows both styles for arrays in wine.
The one line arrays (e.g. short_hklm) no, the two-line or longer ones yes.
The tests show that having any backslash at the start results in an error. That particular if just picks which error message to show.
Yes, but you could just check for two backslashes and proceed normally otherwise. You'll then write the same error message because you cannot find "\HKEY_CURRENT_USER" in your root key table.
reg/tests/reg.c:109 shows only the winXP version accepts multiple trailing backslashes.
The reason for stripping a single trailing backslash was for output consistency with query (As yet unimplemented) Specifically whether query outputs "HKEY_CURRENT_USER\key" or "HKEY_CURRENT_USER\key\" as the key name.
It'd be better to add this functionality together with query support then. Maybe keep it inside query only.
Is there a advapi32 function go get a string name for an open HKEY handle? You could use that to normalize printed paths. That could also take care of e.g. upper and lower case letters.
Because the input is the full path we have to stop comparing after the root key or it will always return 0 unless it's passed a root key.
I think the "path" parameter name makes this more obvious, but I could throw a comment in if that helps.
Nah, it should be possible to make this function readable without a comment. How about the names "input_path" and "root_key_name"?
Actually this solves 2 separate problems. RegOpenKey takes the path after the root key but the path provided includes the root key, so strchrW skips the root key in the path in advance of passing it to RegOpenKey.
Yes, you need the strchrW call.
On the other hand, if someone inputs only a root key IE "HKCU" it will return NULL. We already have the key from path_get_rootkey, so we just return it here.
I don't think you need the NULL check with the early return, and I don't think you need to strip the trailing \ in sanitize_path. Removing those (not accounting for reg query for now) will remove two code branches that need testing and make the code more difficult to read.
That's assuming RegOpenKey behaves as intended in those cases:
Msdn says RegOpenKey(key_in, NULL, &key) returns key == key_in, so no need for the early return in the case that strchrW returns NULL. If the behavior is the same in RegOpenKey(key_in, "\", &key) there's no need to strip trailing backslashes.
Maybe RegOpenKey(NULL, xxx, &key) returns an error that you can propagate to main() and translate into an error message there. In that case you don't have to write an error message from a helper function.
The RegOpenKey behavior may need additional tests in advapi32.
On 10/28/2014 09:51 PM, Stefan Dösinger wrote:
I don't think you need the NULL check with the early return
RegOpenKey always returns an error if path starts with '\', which is why theres an increment on path right after the strchrW
Of course if path_get_key returns NULL we'll be passing in the equivalent of `(WCHAR *)2` so the NULL check prevents segfaults.
We could replace the return with the path increment so that RegOpenKey gets NULL as it's parameter like this:
path = strchrW(path, '\\'); if (path) path++; if (RegOpenKeyW(k, path, &k) != ERROR_SUCCESS)
I suppose removing a line is worth the extra effort of having root keys go through RegOpenKey
How's this draft?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-29 00:19, schrieb Jonathan Vollebregt:
RegOpenKey always returns an error if path starts with '\', which is why theres an increment on path right after the strchrW
Of course if path_get_key returns NULL we'll be passing in the equivalent of `(WCHAR *)2` so the NULL check prevents segfaults.
Right, my bad.
We could replace the return with the path increment so that RegOpenKey gets NULL as it's parameter like this:
It has one less branch (no early return), but still has the if, so I guess it is an improvement, although not as much as I hoped. I'll leave this up to you.
Another alternative idea, but I don't know how well this can work: path_get_key already looks at the '\' behind the root key. You could change this function to return a root key and a WCHAR that points to the rest of the path sans \. But don't bother too much about this, I think that's a fairly minor detail.
Other than that you're still writing error messages from inside helper functions. Please try to return useful error codes and write the error messages from main() or the top level reg_*() functions. This should be a separate patch though, and it may mean adding some error case tests to advapi32.
The thinking behind this is: There are plenty of reasons why Reg* functions can fail, not just that the key does not exist, e.g. permission denied, maybe others. Thus you will need some code that writes error messages that correspond to the Reg*Key return values anyway.
There will always be errors that are specific to reg.exe though. E.g. it's not possible to pass a negative DWORD to RegSetValue.
Other things:
+static BOOL sane_path(const WCHAR *key) +{
- int i = strlenW(key);
- if (i < 3 || (key[i - 1] == '\' && key[i - 2] == '\'))
- {
reg_message(STRING_INVALID_KEY);return 0;
Please return TRUE or FALSE from a function that returns a BOOL type.
I still don't see why you need this check. If you remove it path_get_key should produce the same error message because it can't find a root node "\FOOBAR".
The remote check ("\FOOBAR") is necessary though - it writes a different error message.
key_name = argvW[2];
if (!sane_path(argvW[2]))return 1;
Minor detail: Pass key_name to sane_path.
Patch 3:
if (strcmpiW(type_rels[i].name, type_name) == 0)return type_rels[i].type;
if (!strcmpiW(...)) is preferred over if (strcmpiW(...) == 0)
reg_type = get_regtype(type);
if (!type)reg_type = REG_SZ;elsereg_type = wchar_get_type(type);
Why not keep the !type check inside get_regtype?
Patch 4:
i = (strlenW(input) + 1) * sizeof(WCHAR);output = HeapAlloc(GetProcessHeap(), 0, i);lstrcpyW((WCHAR *) output, input);
Since you know the length, you can use memcpy. You could also think about a strdupW_heap() helper function if you need this in multiple places, but then reusing the length is more difficult.
Patch 5:
if (!force && RegQueryValueW(key, value_name, NULL, NULL) == ERROR_SUCCESS) {
if (RegQueryValueW(subkey,value_name,NULL,NULL)==ERROR_SUCCESS){/* FIXME: Prompt for overwrite */}
/* FIXME: Prompt for overwrite */ }
Afaics this was and still is dead code. Unless RegQueryValue has a side effect either just keep the comment or write an actual FIXME.
If the subkey does not exist, and invalid data is passed, you're creating the subkey without adding the value and returning an error. This is likely not the correct behavior.
On 10/29/2014 04:38 PM, Stefan Dösinger wrote:
Other than that you're still writing error messages from inside helper functions. Please try to return useful error codes and write the error messages from main() or the top level reg_*() functions. This should be a separate patch though, and it may mean adding some error case tests to advapi32.
The thinking behind this is: There are plenty of reasons why Reg* functions can fail, not just that the key does not exist, e.g. permission denied, maybe others. Thus you will need some code that writes error messages that correspond to the Reg*Key return values anyway.
There will always be errors that are specific to reg.exe though. E.g. it's not possible to pass a negative DWORD to RegSetValue.
How would I add reg.exe specific errors? Just make them ints over the 16k used by system error codes?
In any case implementing this would require a big rewrite of most of the patches in the series, so I'll probably make a patch for the end of this series.
+static BOOL sane_path(const WCHAR *key) +{
- int i = strlenW(key);
- if (i < 3 || (key[i - 1] == '\' && key[i - 2] == '\'))
- {
reg_message(STRING_INVALID_KEY);return 0;I still don't see why you need this check. If you remove it path_get_key should produce the same error message because it can't find a root node "\FOOBAR".
This checks for backslashes at the end of the key not the start - tests show more than one should produce an error.
reg_type = get_regtype(type);
if (!type)reg_type = REG_SZ;elsereg_type = wchar_get_type(type);Why not keep the !type check inside get_regtype?
It's possible for something calling this to want a different default, so I decided to let the caller handle it.
Patch 4:
i = (strlenW(input) + 1) * sizeof(WCHAR);output = HeapAlloc(GetProcessHeap(), 0, i);lstrcpyW((WCHAR *) output, input);Since you know the length, you can use memcpy. You could also think about a strdupW_heap() helper function if you need this in multiple places, but then reusing the length is more difficult.
Does this mean I'm allowed to use malloc too?
Patch 5:
if (!force && RegQueryValueW(key, value_name, NULL, NULL) == ERROR_SUCCESS) {
if (RegQueryValueW(subkey,value_name,NULL,NULL)==ERROR_SUCCESS){/* FIXME: Prompt for overwrite */}
/* FIXME: Prompt for overwrite */ }Afaics this was and still is dead code. Unless RegQueryValue has a side effect either just keep the comment or write an actual FIXME.
The idea is presumably only to prompt for overwrite if the value already exists. That's what the RegQueryValue is for. Presumably a y/n prompt should go here but I have my hands full with the rest of it.
If the subkey does not exist, and invalid data is passed, you're creating the subkey without adding the value and returning an error. This is likely not the correct behavior.
Should it prompt for new key creation as well as overwriting? I'm not sure what the native behavior is
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-29 19:56, schrieb Jonathan Vollebregt:
There will always be errors that are specific to reg.exe though. E.g. it's not possible to pass a negative DWORD to RegSetValue.
How would I add reg.exe specific errors? Just make them ints over the 16k used by system error codes?
That depends. Printing error messages from the top level reg_add, reg_query, etc is fine. What I don't like is printing them from functions at many different levels.
In some cases (e.g. invalid root key) you may be able to reuse some of the existing error values.
In any case implementing this would require a big rewrite of most of the patches in the series, so I'll probably make a patch for the end of this series.
Works for me.
This checks for backslashes at the end of the key not the start - tests show more than one should produce an error.
Ooops, I should have read this more carefully :-\ .
reg_type = get_regtype(type);
if (!type)reg_type = REG_SZ;elsereg_type = wchar_get_type(type);Why not keep the !type check inside get_regtype?
It's possible for something calling this to want a different default, so I decided to let the caller handle it.
I'd say keep it in get_regtype until you find a specific case that needs it.
Since you know the length, you can use memcpy. You could also think about a strdupW_heap() helper function if you need this in multiple places, but then reusing the length is more difficult.
Does this mean I'm allowed to use malloc too?
No, what I meant is write your own strdup-like function that uses HeapAlloc.
The idea is presumably only to prompt for overwrite if the value already exists. That's what the RegQueryValue is for. Presumably a y/n prompt should go here but I have my hands full with the rest of it.
Sure, I'm not saying that you should add a prompt now. I'm saying a /* FIXME: Check if the value exists and prompt for overwrite */ works just as well as calling RegQueryValue for (currently) no reason.
If the subkey does not exist, and invalid data is passed, you're creating the subkey without adding the value and returning an error. This is likely not the correct behavior.
Should it prompt for new key creation as well as overwriting? I'm not sure what the native behavior is
What I meant is that you are doing some error checks *after* you already modified the registry. Leaving modifications behind (or creating them in the first place) if invalid operands are specified is most likely wrong.
-
Jonathan Vollebregt -
Stefan Dösinger