Re: reg.exe: Add query function

[Nikolay Sivov]

On 8/1/2013 17:44, Hugh McMaster wrote:
> This patch builds significantly on the query function stub in programs/reg.exe. The patch includes recursion for subkeys.
>
> Some examples of usage:
>
> wine reg.exe query "HKCU"
> wine reg.exe query "HKCU\Console"
> wine reg.exe query "HKCU\Console" /v "ScreenSize"
> wine reg.exe query "HKCU\Software" /s
>
> programs/reg/reg.c |  232 +++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 221 insertions(+), 11 deletions(-)
>
> +static LPVOID WINAPI mem_alloc(size_t size)
> +{
> - return HeapAlloc(GetProcessHeap(), 0, size);
>
> +}
> -
>
> +static void WINAPI mem_free(LPVOID mem)
> +{
> - HeapFree(GetProcessHeap(), 0, mem);
>
> +}

Why WINAPI? It's better to follow naming from existing module, there's 
plenty of them with similar helpers.
> - case REG_SZ:
> -
>    case REG_MULTI_SZ:
> -
>    case REG_EXPAND_SZ:

No tabs please.
> +static void reg_output_subkey(LPWSTR key_name, LPWSTR subkeyName)
> +{
> - reg_printfW(formatSW, key_name);
> -
> - if (subkeyName > 0)
> - {
> -
>    reg_printfW(formatSW, slashW);
> -
>    reg_printfW(formatSW, subkeyName);
> -
>    reg_printfW(formatSW, newlineW);
> - }
>
> +}

Comparison looks a bit odd. Also why do you need to break it down to 
three printf calls instead of one?
> - p = strchrW(key_name,'\');
> - if (!p)
> - {
> -
>    p = 0;
> - }
> - else p++;

I'm not sure what this is supposed to do.
> -
>    case REG_BINARY:
> -
>    case REG_NONE:
> -
>        pValue = value;
> -
>        for (i=0; i<valueSize; i++, pValue++)
> -
>            reg_printfW(formatXW, *pValue);
> -
>        break;

There's no need for separate variable here nor for incrementing pointer.

reg.exe: Add query function

[Hugh McMaster]

On Thursday, 1 August 2013 11:55 PM, Nikolay Sivov wrote:
> > - p = strchrW(key_name,'\');
> > - if (!p)
> > - {
> > -
> >    p = 0;
> > - }
> > - else p++;
>
> I'm not sure what this is supposed to do.

It is equivalent to the following code;

p = strchrW(key_name, '\');
    if (p != NULL) p++;

I've now modified the patch to represent this.
> > -
> >    case REG_BINARY:
> > -
> >    case REG_NONE:
> > -
> >        pValue = value;
> > -
> >        for (i=0; i<valueSize; i++, pValue++)
> > -
> >            reg_printfW(formatXW, *pValue);
> > -
> >        break;
>
> There's no need for separate variable here nor for incrementing pointer.

The incrementing pointer is needed because the 'value' is a Byte array. But the separate variable is not needed.

reg.exe: Add query function

[Nikolay Sivov]

On 8/2/2013 12:52, Hugh McMaster wrote:
> > > -
> > >    case REG_BINARY:
> > > -
> > >    case REG_NONE:
> > > -
> > >        pValue = value;
> > > -
> > >        for (i=0; i<valueSize; i++, pValue++)
> > > -
> > >            reg_printfW(formatXW, *pValue);
> > > -
> > >        break;
> >
> > There's no need for separate variable here nor for incrementing pointer.
>
> The incrementing pointer is needed because the 'value' is a Byte array. But the separate variable is not needed.

Yes, so value[i] will do the same.
>

reg.exe: Add query function

[Hugh McMaster]

On Friday, 2 August 2013 6:58 PM, Nikolay Sivov wrote:
> > > There's no need for separate variable here nor for incrementing pointer.
> >
> > The incrementing pointer is needed because the 'value' is a Byte array. But the separate variable is not needed.
>
> Yes, so value[i] will do the same.

Yes, you're quite correct.