[PATCH 1/3] PATCHv1: Add generic function to retrieve HKEY string representation.

[Thomas Faber]

Hi, and thanks for your submission.
I've added some comments in-line.


On 2018-01-02 15:14, Piyush Raj wrote:
> ---
>   programs/reg/reg.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
> 

Your sign-off is missing.
Perhaps also add a small explanation of what issue you're fixing or what
feature you're implementing to clarify the goal of the patch(es).
Ideally you'd also start your commit message with the name of the module
("reg: xxx").

Please see
https://wiki.winehq.org/Submitting_Patches#The_commit_message


> diff --git a/programs/reg/reg.c b/programs/reg/reg.c
> index 8d510f7f7e..405cc2a8be 100644
> --- a/programs/reg/reg.c
> +++ b/programs/reg/reg.c
> @@ -814,6 +814,75 @@ static int query_all(HKEY key, WCHAR *path, BOOL recurse)
>       return 0;
>   }
>   
> +static const WCHAR HKCR_WSTR[] = {'H', 'K', 'E', 'Y', '_', 'C', 'L', 'A', 'S', 'S', 'E', 'S', '_', 'R', 'O', 'O', 'T',  0};

Please don't put spaces between the individual characters. All wide
strings in Wine are formatted the same way ('A','B','C') so that it's
possible to grep for strings.

The naming of these variables also seems odd; all uppercase is usually
used for macros.


> +static DWORD HKCR_WSTR_SZ = sizeof(HKCR_WSTR);
> +static const WCHAR HKLM_WSTR[] = {'H', 'K', 'E', 'Y', '_', 'L', 'O', 'C', 'A', 'L', '_', 'M', 'A', 'C', 'H', 'I', 'N', 'E', 0};
> +static DWORD HKLM_WSTR_SZ = sizeof(HKLM_WSTR);
> +static const WCHAR HKCU_WSTR[] = {'H', 'K', 'E', 'Y', '_', 'C', 'U', 'R', 'R', 'E', 'N', 'T', '_', 'U', 'S', 'E', 'R', 0};
> +static DWORD HKCU_WSTR_SZ = sizeof(HKCU_WSTR);
> +static const WCHAR HKU_WSTR[] = {'H', 'K', 'E', 'Y', '_', 'U', 'S', 'E', 'R', 'S', 0};
> +static DWORD HKU_WSTR_SZ = sizeof(HKU_WSTR);
> +
> +/*
> + * Given hkey, return its string representation into buf, who is
> + * assumed to hold at least (buf_sz * sizeof(*buf)) bytes

You compare buf_sz to HKLM_WSTR_SZ and friends, which are in bytes.
Given how you call the function in patch 2, buf_sz really is in bytes,
not characters.


> + *
> + * Return 0 if successful, other value otherwise
> + */
> +static LONG hkey_to_wstring(HKEY hkey, WCHAR *buf, DWORD buf_sz)

This function isn't used, so you're introducing dead code. It's
generally preferred to add code only when it is needed.
In fact from what I can see, all of your code throughout the patch
series seems to be unused. It would be better to introduce it along with
the changes that make use of the new functions.


> +{
> +    if (hkey == HKEY_LOCAL_MACHINE)
> +    {
> +        if (buf_sz >= HKLM_WSTR_SZ)
> +        {
> +            memcpy(buf, HKLM_WSTR, HKLM_WSTR_SZ);

I'd personally find this easier to read if it used sizeof directly,
without the additional size variable, but that's a matter of taste I
suppose.


Best,
Thomas