Re: [PATCH] odbccp32: Implement SQLGetPrivateProfileStringW/SQLGetPrivateProfileString (resend)

[Nikolay Sivov]

On 26.10.2015 11:56, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/odbccp32/odbccp32.c   | 182 ++++++++++++++++++++++++++++++++----
>   dlls/odbccp32/tests/misc.c | 223 ++++++++++++++++++++++++++++++++++++++++++++-
>   include/odbcinst.h         |   4 +-
>   3 files changed, 389 insertions(+), 20 deletions(-)

This looks mostly fine. Let me suggest some cleanups.
> -int WINAPI SQLGetPrivateProfileStringW(LPCWSTR lpszSection, LPCWSTR lpszEntry,
> -
>           LPCWSTR lpszDefault, LPCWSTR RetBuffer, int cbRetBuffer,
> -
>           LPCWSTR lpszFilename)
>
> +static HKEY get_privateprofile_sectionkey(LPCWSTR section, LPCWSTR filename)
>   {
> - static const WCHAR odbcW[] = {'S','o','f','t','w','a','r','e','\','O','D','B','C','\',0};
> - HKEY hkey, hkeyfilename, hkeysection;
> - LONG ret;
> -
> - if (RegOpenKeyW(HKEY_CURRENT_USER, odbcW, &hkey))
> -
>    return NULL;
> -
> - ret = RegOpenKeyW(hkey, filename, &hkeyfilename);
> - RegCloseKey(hkey);
> - if (ret)
> -
>    return NULL;
> -
> - ret = RegOpenKeyW(hkeyfilename, section, &hkeysection);
> - RegCloseKey(hkeyfilename);
> - if (ret)
> -
>    return NULL;

These 2 lines are redundant, you can remove them, or turn return line to 
always return hkeysection.
> -
> - return ret ? NULL : hkeysection;
>
> +}
> -
> - if (!buff_len)
> -
>   return 0;
> -
> - if(!defvalue)
> -
>    buff[0] = 0;

Please use consistent formatting, 4 leading spaces, and space after 'if'.
> -
> - if (!section || !defvalue || !buff)
> -
>    return 0;
> -
> - if (buff)
> -
>    buff[0] = 0;
> -
> - sectionkey = get_privateprofile_sectionkey(section, filename);
> - if (sectionkey)
> - {
> -
>    DWORD type, size;
> -
> -
>    if(entry)
> -
>    {
> -
>        size = buff_len * sizeof(*buff);
> -
>        if (!RegGetValueW(sectionkey, NULL, entry, RRF_RT_REG_SZ, &type, buff, &size))
> -
>        {
> -
>            usedefault = FALSE;
> -
>            ret = (size / sizeof(*buff)) - 1;
> -
>        }
> -
>    }
> -
>    else
> -
>    {
> -
>        WCHAR name[MAX_PATH];
> -
>        DWORD index = 0;
> -
>        DWORD namelen;
> -
> -
>        usedefault = FALSE;
> -
> -
>        namelen = sizeof(name);
> -
>        while (RegEnumValueW(sectionkey, index, name, &namelen, NULL, NULL, NULL, NULL ) == ERROR_SUCCESS)
> -
>        {
> -
>            if ((ret +  namelen+1) > buff_len)
> -
>                break;
> -
> -
>            lstrcpyW(buff+ret, name);
> -
>            ret += namelen+1;
> -
>            namelen = sizeof(name);
> -
>            index++;
> -
>        }
> -
>    }

What I don't understand is why temporary fixed size buffer is needed. 
This loop immediately copies to return buffer, it should be possible to 
enumerate directly to this buffer, and if return buffer is not large 
enough to hold all values it should be identical to this break from the 
loop you have. Is there a test for test by the way?

For consistency it's better to turn RegEnumValueW to !RegEnumValueW, 
like RegGetValueW does, or the other way around, it depends on which 
style we use for the rest of dll already. Also not space at the end of 
parameter list.

All of that applies to A-function too.
> -
> - ret = SQLGetPrivateProfileString("wineodbc", "testing" , "defaultX", buffer, 256, "ODBC.INI");
> - ok(ret == 8, "SQLGetPrivateProfileString returned %d\n", ret);
> - ok(!strcmp(buffer, "defaultX"), "incorrect string '%s'\n", buffer);
> -
> - ret = SQLGetPrivateProfileString("wineodbc", "testing" , "defaultX", buffer, 4, "ODBC.INI");
> - ok(ret == 3, "SQLGetPrivateProfileString returned %d\n", ret);
> - ok(!strcmp(buffer, "def"), "incorrect string '%s'\n", buffer);
> -
> - ret = SQLGetPrivateProfileString("wineodbc", "testing" , "defaultX", buffer, 8, "ODBC.INI");
> - ok(ret == 7, "SQLGetPrivateProfileString returned %d\n", ret);
> - ok(!strcmp(buffer, "default"), "incorrect string '%s'\n", buffer);

Please replace all magic numbers with strlen/strlenW/sizeof.
> -
> -
>    strcpy(buffer, "wine");
> -
>    ret = SQLGetPrivateProfileString("wineodbc", NULL , "", buffer, 256, "abcd.ini");
> -
>    ok(ret == 14, "SQLGetPrivateProfileString returned %d\n", ret);
> -
>    if(ret >= 14)
> -
>    {
> -
>        ok(!strcmp(buffer, "testing"), "incorrect string '%s'\n", buffer);
> -
>        ok(!strcmp(buffer+8, "value"), "incorrect string '%s'\n", buffer+8);
> -
>    }

Why testing this separately? It looks easier to test whole buffer at 
once, with memcmp if some embedding nulls are expected.

With all that fixed, I think it's good to go.