-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
I think this patch has the potential to improve code readability if improved a bit.
Am 2014-10-09 10:55, schrieb Jonathan Vollebregt:
+#define MAX_ROOT_KEY_NAME_LENGTH 20 +#define NUM_ROOT_KEYS 5
+static const WCHAR short_HKEY_name[NUM_ROOT_KEYS][5] = { ... +};
+static const WCHAR long_HKEY_name[NUM_ROOT_KEYS][MAX_ROOT_KEY_NAME_LENGTH] = { ... +};
+static const HKEY HKEYs[NUM_ROOT_KEYS] = { ... +};
Why not use one table instead of 3 separate arrays with magically matching indices?
Something like static const struct { HKEY key; const WCHAR *long_name; const WCHAR *short_name; } keys[] = { {HKEY_LOCAL_MACHINE, {'H','K','E','Y','_','L','O','C','A','L','_','M','A','C','H','I','N','E',0}, {'H','K','L','M',0}}, ... };
static HKEY path_get_rootkey(const WCHAR *path) { unsigned int i; for (i = 0; i < sizeof(keys) / sizeof(*keys); i++) { if (!strcmpi(keys[i].short_name, name) || !strcmpi(keys[i].long_name, name)) return keys[i].key; } return NULL; }
if (strncmpiW(path, short_HKEY_name[i], strlenW(short_HKEY_name[i])) == 0 ||strncmpiW(path, long_HKEY_name[i], strlenW(long_HKEY_name[i])) == 0)
I don't think strncmpi(x, y, strlen(y)) has any advantage over strcmpi(x, y). strlen just counts the number of characters to the first 0 byte, where strcmpi would stop anyway.
Are you sure there is not some other behavior of CompareStringW that str(n)cmpi does not have? I have not found any, but I am not sure about the CP_ACP part.
The original code probably has a bug if the user passes a string like HKEY_LOCAL_MACHINE_GARBAGE. It's worth writing a test for this.
+static HKEY path_get_rootkey(const WCHAR *path) +{
- if (path_get_rootkey_name(path))
return HKEYs[(path_get_rootkey_name(path) - long_HKEY_name[0]) / MAX_ROOT_KEY_NAME_LENGTH];
This searches the array twice.
- path = strchrW(path, '\');
- if (!path)
return k;
I expect that this causes trouble with RegCloseKey.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-12 15:58, schrieb Stefan Dösinger:
Something like static const struct { HKEY key; const WCHAR *long_name; const WCHAR *short_name; } keys[] = { {HKEY_LOCAL_MACHINE, {'H','K','E','Y','_','L','O','C','A','L','_','M','A','C','H','I','N','E',0}, {'H','K','L','M',0}}, ... };
As Jonathan pointed out in a private message this does not work. Is there a canonical way to handle this? All solutions I found are somewhat ugly or I'm not sure if they are correct.
On Oct 12, 2014, at 11:11 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 2014-10-12 15:58, schrieb Stefan Dösinger:
Something like static const struct { HKEY key; const WCHAR *long_name; const WCHAR *short_name; } keys[] = { {HKEY_LOCAL_MACHINE, {'H','K','E','Y','_','L','O','C','A','L','_','M','A','C','H','I','N','E',0}, {'H','K','L','M',0}}, ... };
As Jonathan pointed out in a private message this does not work. Is there a canonical way to handle this? All solutions I found are somewhat ugly or I'm not sure if they are correct.
It should work to make the long_name and short_name fields arrays instead of pointers, although that brings back the use of fixed lengths.
The other approach is to define the strings like:
static const WCHAR short_name_HKLM[] = {'H','K','L','M',0}; … static const WCHAR long_name_HKEY_LOCAL_MACHINE[] = {'H','K','E','Y','_','L','O','C','A','L','_','M','A','C','H','I','N','E',0}; …
and then build the table as:
static const struct { HKEY key; const WCHAR *long_name; const WCHAR *short_name; } keys[] = { {HKEY_LOCAL_MACHINE, long_name_HKEY_LOCAL_MACHINE, short_name_HKLM}, … };
That's not especially compact, but at least it makes a single table and avoids fixed-length arrays.
Regards, Ken
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-12 18:38, schrieb Ken Thomases:
It should work to make the long_name and short_name fields arrays instead of pointers, although that brings back the use of fixed lengths.
The other approach is to define the strings like:
static const WCHAR short_name_HKLM[] = {'H','K','L','M',0};
Those are the two of the three options I found. The third suggestion I found was to cast the initializer to a const WCHAR [], like this (just using char here for testing outside of wine):
static const struct { const char *foo; int bar; } test[] = { {(const char []){'a', 'b', 'c', 0}, 5}, };
But that seems to be a C99 feature (http://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html).
The other thing I noticed is that even my big fat C/C++ book is completely clueless about this and (somewhat relatedly) initializes a (non-const) char * with a string literal and later sprintf()s to it.
if (strncmpiW(path, short_HKEY_name[i], strlenW(short_HKEY_name[i])) == 0 ||strncmpiW(path, long_HKEY_name[i], strlenW(long_HKEY_name[i])) == 0)I don't think strncmpi(x, y, strlen(y)) has any advantage over strcmpi(x, y). strlen just counts the number of characters to the first 0 byte, where strcmpi would stop anyway.
The function takes a full key path so we can't just flat compare it to the root key strings - this way it stops checking for differences after the root key name.
- path = strchrW(path, '\');
- if (!path)
return k;I expect that this causes trouble with RegCloseKey.
Assuming you expect errors using RegCloseKey on a predefined key like HCKU: In practice it works just fine. I imagine it's supposed to work this way since testbot doesn't throw any errors either.
I don't think this is really an improvement. The old code first looked at argvW[1], then argvW[2], ..., with clear checks that the argument count is big enough. The new code jumps around between the arguments without any clear order. As a result it still isn't clear if argv[2] is supposed to be a registry key at all when you check it for remoting. While it may be true in the current form of reg.exe, it won't work if support for e.g. reg import is added.
I've replaced that commit with a simpler one using a function to sanitize the path and leaving the flow control as it is in master:
https://github.com/jnvsor/wine/commit/25acdd930847d86022193ce4cc9cb83be32880...
+static HKEY path_get_rootkey(const WCHAR *path) +{
- if (path_get_rootkey_name(path))
return HKEYs[(path_get_rootkey_name(path) - long_HKEY_name[0]) / MAX_ROOT_KEY_NAME_LENGTH];This searches the array twice.
I've also fixed the duplicate call to path_get_rootkey_name in path_get_rootkey, changed the seperator>separator variable spelling, and added more detailed commit messages.
What else was there to deal with before re-submitting or is the WCHAR* in array of structs issue all that's left? (Oh for a char* to WCHAR* macro!)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-13 15:25, schrieb Jonathan Vollebregt:
The function takes a full key path so we can't just flat compare it to the root key strings - this way it stops checking for differences after the root key name.
Right, I didn't keep that in mind. Still there's the question of what should happen if the user passes a key path like HKEY_LOCAL_MACHINE_IBREAKYOU\Software\Wine .
Assuming you expect errors using RegCloseKey on a predefined key like HCKU: In practice it works just fine. I imagine it's supposed to work this way since testbot doesn't throw any errors either.
According to the RegCloseKey implementation in dlls/advapi32/registry.c you are correct. I can't find any tests for this behavior though. A comment may be in order if you intentionally relying on this behavior.
I've replaced that commit with a simpler one using a function to sanitize the path and leaving the flow control as it is in master:
https://github.com/jnvsor/wine/commit/25acdd930847d86022193ce4cc9cb83be32880...
This
patch is easier to read, but I am still wondering what the
- if (key[0] == '\') + key++;
and
- i = strlenW(key) - 1; + while (i >= 0 && key[i] == '\') +
key[i--] = 0;
parts are for. What do leading backslashes in a key do if they don't specify a remote host? Can there really be any number of trailing backslashes? What about trailing spaces like " \remote_host\HKLM\foo\bar"?
I've also fixed the duplicate call to path_get_rootkey_name in path_get_rootkey, changed the seperator>separator variable spelling, and added more detailed commit messages.
What else was there to deal with before re-submitting or is the WCHAR* in array of structs issue all that's left? (Oh for a char* to WCHAR* macro!)
There are plenty of things that need additional tests.
Right, I didn't keep that in mind. Still there's the question of what should happen if the user passes a key path like HKEY_LOCAL_MACHINE_IBREAKYOU\Software\Wine .
Assuming you expect errors using RegCloseKey on a predefined key like HCKU: In practice it works just fine. I imagine it's supposed to work this way since testbot doesn't throw any errors either.
According to the RegCloseKey implementation in dlls/advapi32/registry.c you are correct. I can't find any tests for this behavior though. A comment may be in order if you intentionally relying on this behavior.
Fixed and commented:
https://github.com/jnvsor/wine/commit/1f6012dcee7ee58e7206c1a6510cc779727555...
This patch is easier to read, but I am still wondering what the
- if (key[0] == '\') + key++;
and
- i = strlenW(key) - 1; + while (i >= 0 && key[i] == '\') +
key[i--] = 0;
parts are for. What do leading backslashes in a key do if they don't specify a remote host? Can there really be any number of trailing backslashes? What about trailing spaces like " \remote_host\HKLM\foo\bar"?
The examples at the following URL indicate that even if a single leading backslash isn't a valid key name, it's a valid reg.exe input.
https://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en...
Trailing spaces are valid key names (At least in wine, I imagine in native too)
There are plenty of things that need additional tests.
The closest thing I have to a native windows system is testbot and I doubt you'd want me clogging it up just to find out what the proper reg inputs are.
If anyone can write tests for the rest, that would be great!
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-13 16:51, schrieb Jonathan Vollebregt:
Trailing spaces are valid key names (At least in wine, I imagine in native too)
Sorry, I meant (and the example used it that way) leading spaces. That would affect the way you check for remote keys. Obviously trailing spaces need tests too, but if advapi32 handles them you don't need extra handling in the implementation.
The closest thing I have to a native windows system is testbot and I doubt you'd want me clogging it up just to find out what the proper reg inputs are.
If anyone can write tests for the rest, that would be great!
That's exactly what the testbot is there for. No excuses ;-) . But you'll be happier with your own windows machine (or VM) because it has a way faster response time than the testbot.
There's no point in changing the argument handling if we don't know that the changes are actually correct.
I'm going to bring back the question of how to statically assign structs with WCHAR strings inside them.
The options are:
* Cast the array notation:
(const WCHAR []){'S','T','R','I','N','G',0}
Which is a C99 feature.
* Assign each string to a variable and assign each variable to the struct: [1]
This has a lot of lines of code, and personally I think it almost looks worse than what was there before.
* Make the structs use arrays: [2]
This brings the fixed length issue, but it seems much cleaner than [1]
Is there any reason to prefer [1] to [2]? Is there a better way of doing this?
[1] https://github.com/jnvsor/wine/commit/9efaba8c3 [2] https://github.com/jnvsor/wine/commit/80ee795c1
On Fri, Oct 24, 2014 at 06:58:22PM +0200, Jonathan Vollebregt wrote:
- Assign each string to a variable and assign each variable to the
struct: [1]
This has a lot of lines of code, and personally I think it almost looks worse than what was there before.
- Make the structs use arrays: [2]
This brings the fixed length issue, but it seems much cleaner than [1]
Is there any reason to prefer [1] to [2]? Is there a better way of doing this?
[1] https://github.com/jnvsor/wine/commit/9efaba8c3 [2] https://github.com/jnvsor/wine/commit/80ee795c1
I'd pick [1]. I think it reflects the situation better: you're pointing to strings of arbitrary length, not 5- or 20-char arrays. It also means you don't have to update the array length if you were to add a longer string in the future.
First similar example I found was in test_StrStrW() in <dlls/shlwapi/tests/string.c>.
Andrew
-
Andrew Eikum -
Jonathan Vollebregt -
Ken Thomases -
Stefan Dösinger