Re: [v3 PATCH] odbccp32: Handle ODBC_CONFIG_DRIVER request in SQLConfigDriver/W

[Huw Davies]

On Thu, Apr 13, 2017 at 01:06:50AM +0000, Alistair Leslie-Hughes wrote:
> +static BOOL write_config_value(const WCHAR *driver, const WCHAR *args)
> +{
> - long ret;
> - HKEY hkey;
> -
> - if (!args)
> -
>    return FALSE;
> -
> - if ((ret = RegOpenKeyW(HKEY_LOCAL_MACHINE, odbcini, &hkey)) == ERROR_SUCCESS)
> - {
> -
>    HKEY hkeydriver;
> -
> -
>    if ((ret = RegOpenKeyW(hkey, driver, &hkeydriver)) == ERROR_SUCCESS)
> -
>    {
> -
>        WCHAR *divider, *value;
> -
> -
>        WCHAR *name = heap_alloc( (strlenW(args) + 1) * sizeof(WCHAR));
> -
>        if(!name)
> -
>        {
> -
>            push_error(ODBC_ERROR_OUT_OF_MEM, odbc_error_out_of_mem);
> -
>            return FALSE;

You're returning without closing the registry keys.  You may want to change
the error handling to a 'goto fail;' type behaviour.
> diff --git a/dlls/odbccp32/tests/misc.c b/dlls/odbccp32/tests/misc.c
> index ed8e6f1..e5a8896 100644
> --- a/dlls/odbccp32/tests/misc.c
> +++ b/dlls/odbccp32/tests/misc.c

...
> - ret = SQLInstallDriverEx("WINE ODBC Driver Path\0Driver=sample.dll\0Setup=sample.dll\0\0", "c:\temp", path, MAX_PATH, &size, ODBC_INSTALL_COMPLETE, NULL);
> - ok(ret, "SQLInstallDriverEx failed\n");
> sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL);
> - ok(sql_ret && error_code == SQL_SUCCESS, "SQLInstallDriverEx failed %d, %u\n", sql_ret, error_code);
> -
>
> ok(sql_ret == SQL_NO_DATA || (sql_ret && error_code == SQL_SUCCESS), "SQLInstallDriverEx failed %d, %u\n", sql_ret, error_code);
> ok(!strcmp(path, "c:\temp"), "invalid path %s\n", path);
> -
>
> ret = SQLConfigDriver(NULL, ODBC_CONFIG_DRIVER, "WINE ODBC Driver Path", "NoWrite=60;xxxx=555", error, sizeof(error), NULL);
> -
>
> ok(ret, "SQLConfigDriver failed\n");
> -
>
> sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL);
> -
>
> ok(sql_ret == SQL_NO_DATA || (sql_ret && error_code == SQL_SUCCESS), "SQLConfigDriver failed %d, %u\n", sql_ret, error_code);
> -
> -
>
> ret = SQLConfigDriver(NULL, ODBC_CONFIG_DRIVER, "WINE ODBC Driver Path", "empty", error, sizeof(error), NULL);
> -
>
> ok(!ret, "SQLConfigDriver successed\n");
> -
>
> sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL);
> -
>
> ok(sql_ret && error_code == ODBC_ERROR_INVALID_KEYWORD_VALUE, "SQLConfigDriver failed %d, %u\n", sql_ret, error_code);
> -
>
> if (ret)
>    {

This entire block is now skipped if things proceed as expected...

Also, 'successed' is not a word ;-)

Huw.