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.