Re: [PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Stefan Dösinger]

-----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 SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUOoj8AAoJEN0/YqbEcdMwBI8P/1CgdHX0yt2Z6iZqn61fA1wt
M89EKguHgXoQmorrtpMYtZJNoMd6f0kIv4yH+corcZwz01iGAuu4Zq7v3grD3SIz
uv6S+QNiysw8wT5tTSFVu+DiGuxQXreb1BryjMXgmoOLbmPLPbcYX+yEj1DaiRqg
gh1hXYpVoqCjKGed8Wukk+Cq481azQz41e2vUqd2pP8mux0BToWaBU8H9HufTxYg
RyHYaKksstGhQLINm2FOAtq2S8eZN5TRdELcTEuQoDiE6c6YNIepLLJ5f6MiH4lt
6Biq2CUeIOhkqEWjWGzGmfom2rBq1lcSWuLnV9N7coRKdCOC+wWpeEvUBjvFTqMW
JcbdZxusE3Rp80scp7YFnUBVXfOlQr4IofKkNeJkVUjoAIGOuSAcr6pHnO8KTpMI
K/9nkFb/cOF8IQi6Gc3PxJxXYBBQFw578aeNpUk6QEmimXcyBvmCe/K2Oki1Qa7x
CoU7MIMvtE1oSL2kN5o4yzJ+YrIZ4CGfg8yhvl9M54tWHvLMLe+Bw+5pMefwgDXP
QIYCdKKIWf+hsTOJCYHLgD5wU8euvTvcgmBklI5cGivwLQqrLfZMucmnsG4jmXH4
pLdZwJsC1XXRRej13YofPzfRSZbK3pcjGwpiq7sxJTrWH05pTm+kBOGId1ChelcS
jW7y6q/ecery5/13atxt
=PXew
-----END PGP SIGNATURE-----

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Stefan Dösinger]

-----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.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUOqgiAAoJEN0/YqbEcdMwEuIP/iy3swubF5W7ewy/Q1zZK3Zr
VApEOGOQM6vGaSp2hjFcbbwwaHV8OU9ebz7MFYrMCaGyhVrPt0WoCt2SZetZQPlJ
Uu6nIR2zhtIxRjYyiOI3b8w53EChUiUkOLMWPGcFpjXgB7Jf6G5OVuyIJONDJzDi
XyyMXdDujj/JUHgWOErGk8j/Jvk7+G7nUn+IQiTZvn6rCdgZY7AZJE9VbNDiggBj
8kEG5Rl6LSq2iuaxubXbnzwkExPen0+tRIZH1N16vkf9yDJXdPLNOFXvLzruRHqn
4f50JHJ+wwzH2Es/a4SOS9f/DMCsWCBFqwNK1oKkAVVDnXeBbZvMdzL/1KxZ90We
aqgSm4q9QvzRvSMIe8QY3YA1YbQ3pnrLEop3wgUqJJa0ngVLuRi/TJwecHmQX9XE
LeMFgtGG4Kkq+WDNXlUeMavVw5vBMDR2QQR4GMMreX3IZ9UYlyTSZbmZmVic3u8K
NoVwbsI2nrO9dv9/Bp/xh20wIlVjJoDjWhjP9fhf7kdPZmN9U90Qihv/zhB0UQbz
gKC/vvn0IcuJRrhycxq2h8qd0dvYWbdXUMqpNgFnQeTN6R7X2MbVvIeYzgWPBdEj
21acS3aUwJwaDu+9FwGeK4uuK4eFF3bhn84WtbwpVq9HH+Bzxg1BLoh+9sDM8udC
jcsfeBYrK8zv9ebH4LLU
=Qa8/
-----END PGP SIGNATURE-----

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Ken Thomases]

On Oct 12, 2014, at 11:11 AM, Stefan Dösinger <stefandoesinger(a)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

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Stefan Dösinger]

-----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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUOrYnAAoJEN0/YqbEcdMw0rYP/jYwnZgkNnIhAQVHxjCNhXyY
qBssmIY316J8w+NdPryAsI9PhE7F3VQAZVIL7nI/zuipfn7B0clM0lKqax+JFSYV
utQL33nJRaZO8ybwemLRcxGqwQYOUMxliTXU2ZBdEo7sRyoLBka41NVjA8E2afXk
9by8NhCKzQVUs4K/TNtvdFlT/0WUVNWcNtDHNbXsBmadGXpQlIu1TjHaMmKd2Irw
fPzWB11GFZ3JWaSdO3p8hNaqHuCb5tUVcewPm/mTf7robshVrzoA2jQXCU2ueAks
0mz938A/c+mMUH5ui/bqbIwTryHlHQsbeJHfrvrhgqPzbVnISJqMdC3dJtlndYlw
NO2eqTp04Q7gYJ0QSdMVkhF1vydMN422iuCiW1DH3sTqNqVHs+CAUb9AG0A1WTdr
bmM3VMIsN8zt7pLknukVGV5vez8dZbyyt9fJq9jtmnoQSq63cxIVGRgn/66FbUW8
uyRuZi3v53wsj6g1JuGFcvzh8Zc1SzSRPnJKWE/Kgj64r2pigwSRsS9El1v0n9PV
ibAAf/GpMpmSy+n1k8chJH996SDx0bEdWCAbIAwBxDBaaJ0uf3QUXmvH06OcaU1W
2iGCzpc0Kw8WgaKxluOrhLigXcyB/9cu8xk2iCSIOcqnjcJ5bewOVapeQHixkLvk
BEJDyBDsDFZncXyuYTCG
=tt1w
-----END PGP SIGNATURE-----

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Jonathan Vollebregt]

> > +        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!)

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Stefan Dösinger]

-----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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUO9wcAAoJEN0/YqbEcdMwiPoQAIYTSNR3sFrM2lnHaRonipG4
0MFZDkN4DmeyJNmUofPq1r6d+bzmLuugyjT/oBkcTHbyVpdEvded2/prmu1Ol5pt
WXhjLfETTESz0HYwrfO4Hi64ktdWBev8bM6Bh6UYV6TvELHoo3Bv4HiIbYtpn7eo
5tgM9cLfSqjOjLUFICsyJ19spOBl1c+qxRHaJ1q0aq3DGJt+QXm7q9A+1kd9Y249
y68dt1ep2PJ6Wkg6UovuD0cmVsVHyX8wJ8podNzm15OBIgVwyM21mb+FnbLi6ToD
n1NcDG1zfljqwfCDaRZwxfmctinJs2EW3hzJe+izP+4soL0dwzNo93JK/zJ7ODaD
j+8Y//86anLderhFSEnIP6tzkHd+SqhMazb4uEJcJsMTvWoT6hdJhP+Loyt0YiCG
goud4Z5en2PqUe76mjJ8SaEDt3jSFIYqXv0TWd5YiEy+S3Xhin79iS2njUDVzYva
7QNtJ79bkimTZomXmFTS1boRwW4UxwutyBC+TAt0ovSM7dD0A0dqQyMPLbTqeEUF
DTogckODdR8/ZL48r3/PoqOrpRBHGkxj9gbkz9xapkZ8aP+OoeCksO1JM3d2hU+n
ouwD32kv+76+zymAVlTbkb+HfUCMilv768KpIHl4qYjwBWxQvs8cas/W0v58lB2d
eAPvsggwnFeHkoJKs9hy
=nUBR
-----END PGP SIGNATURE-----

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Jonathan Vollebregt]

> 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!

[PATCH v4 2/8] reg.exe: Add path_get_key() to remove boilerplate

[Stefan Dösinger]

-----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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUO+ptAAoJEN0/YqbEcdMwO9sQAJa6f93jGH7QXeu+HBE9YcIf
HVA4LL8weSW8SxR0LuHuEfyqWtsoTC0+gBTEznZ9FdLeE0XhCzzFVaNKtZzAk5A7
3RpotewlDWa+sIsOi/ZoXxpFUX/qXqNPKJ+xC8rmPx2AtCisAwlYUcJcK5CotGzG
K+L/Rbc8geb68MKOebjSVwNEXLDUqsgKehZA6DHh7NX7f/UhkIv30ahnpVAgDY5f
GVrpvS8lCoi0kl5V1vCEi0uA3EFIqLocqUpn36lCYWhJdR0acV1oHOOEqBnUfQFN
o1WTCT4d4raOBLEz72HwRLe9JiuYgrr7YZdk6LPlw8u4dSXKqyDF73aR5AiHsXYI
7z1iN5mM1SkZ06elLsMXfCoaXsHPGFHBNgj2lUh+19aWJPLg7ZUaM1QBvq/2upSL
aCRQvAVldmETKJbEkuRDwcblzZUzUo5vxr3Pdjk9Mf+Owu1r+nDONR7Q498seAdH
eR6FFt26gDpdoJzl2RUmioAEZKAHSRgRD/pGaOX0WfuKTGp/m/hv0hrami0ZygYi
v9IBgUZuk7Yn+n8ngo0TttTrtfYZuTB2sPcanIEp4QJVcdvesvz8JOX7dUCAbeHh
NPzM4R/X44To2doqjfmLyn0egCTNy4elS/GqAZ5X1VI9WfcUexRGdmBQfWImdEYh
0NjdSUEYGc5oVp4xzEcg
=VgVy
-----END PGP SIGNATURE-----

Statically assigning structs containing WCHAR strings

[Jonathan Vollebregt]

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

Statically assigning structs containing WCHAR strings

[Andrew Eikum]

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