On 26.10.2015 11:56, Alistair Leslie-Hughes wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@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.