On 26.10.2015 11:56, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
> ---
> 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.