Re: coverity#735354 SQL_ODBC_SQL_CONFORMANCE "same on both sides"

On Wed, May 14, 2014 at 09:09:10PM +0100, Caolán McNamara wrote:
> connectivity/source/drivers/odbc/ODatabaseMetaData.cxx:1502 is..
>
> 1501 ... SQL_ODBC_SQL_CONFORMANCE ...
> 1502 ... SQL_OSC_CORE || ... SQL_OAC_LEVEL1 || ... SQL_OAC_LEVEL2 ...

> and coverity complains that SQL_OAC_LEVEL1 and SQL_OSC_CORE are both 1.
> Looking at external/unixODBC/inc/odbc/sqlext.h I see that...

> /* SQL_ODBC_SQL_CONFORMANCE values */
> #define SQL_OSC_MINIMUM                     0x0000
> #define SQL_OSC_CORE                        0x0001
> #define SQL_OSC_EXTENDED                    0x0002

> while...

> /* SQL_ODBC_API_CONFORMANCE values */
> #define SQL_OAC_NONE                        0x0000
> #define SQL_OAC_LEVEL1                      0x0001
> #define SQL_OAC_LEVEL2                      0x0002

> i.e. we're randomly comparing a SQL_ODBC_API_CONFORMANCE value
> against a result from SQL_ODBC_SQL_CONFORMANCE

> so it looks like it should either be...
> SQL_OSC_MINIMUM || SQL_OSC_CORE || SQL_OSC_EXTENDED
> which would be a change in logic, but perhaps the intent, or
> SQL_OSC_CORE || SQL_OSC_EXTENDED
> which would keep the status quo (which is 13 years since
> 26f4169ad38df90493149e8c0b2270eb8e47d541).

> Any thoughts on which is preferable?

That's only the first strand of a whole tangle of issues with this
family of functions.

I'll (try to) fix the whole tangle. That's the short version.

The long version:

FYI, the right choice is the latter, since the function is called
supportsCoreSQLGrammar, and "Minimum" is less than "Core" (I mean, not
in numeric values, but in conformance level / amount of stuff one
claims to be conformant with). Or maybe it should just be
 return nValue >= SQL_OSC_CORE;

Then there is just below the function supportsMinimumSQLGrammar, which
is just wrong. *There* it should be (in the else branch of the if)
 SQL_OSC_MINIMUM || SQL_OSC_CORE || SQL_OSC_EXTENDED
and
 GetInfo (..., SQL_ODBC_SQL_CONFORMANCE)
not
 GetInfo (..., SQL_ODBC_INTERFACE_CONFORMANCE)

And that's just the beginning... The code you mention is a branch in
an if. In the other branch, we have
 GetInfo(... SQL_ODBC_INTERFACE_CONFORMANCE)
which is wrong: this tests the driver's interface (API) conformance,
not the conformance of the SQL language. OTOH, these "Minimum", "Core"
and "Extended" grammars are a notion from ODBC 2.x, they have
disappeared from ODBC 3.x (replaced by references to the SQL92
standard and levels Entry / Intermediate / Full); Microsoft doesn't
even document anymore what "Core" and "Extended" grammars are (only
Minimum, because *all* driver/DBMS *must* implement Minimum). So
actually, except as a deprecated backwards-compatibility feature, an
ODBC3 driver does *not* even say if the ODBC Core/Extended SQL
grammars are supported.

OTOH, JDBC still has these metadata information, and we (SDBC)
inherited it from JDBC. How ironic, JDBC exposes information about an
ODBC conformance level that ODBC does not expose anymore! How the
author of a "new" JDBC driver is supposed to fathom that information
since IT IS NOT DOCUMENTED ANYMORE AT THE SOURCE OF THE SPECIFICATION
(Microsoft), I'm really not sure.

I think I'll just use the deprecated backwards compatibility stuff and
hope it somehow works out.

Then there is the supportsANSI92* family of functions. Which cleanly
correspond to an ODBC 3.x information value provided by the
driver. Which according to their values and some examples on the
Internet
http://developer.mimer.com/documentation/html_92/Mimer_SQL_Mobile_DocSet/ODBC_API8.html
http://www.databasteknik.se/boken/odbc/odbc-test-05.c
are a *bit* *mask*. But the documentation (and some other examples on
the Internet
http://msdn.microsoft.com/en-us/windows/ms713608%28v=vs.90%29.aspx
) lets one believe they are levels. I'll take my bet for bitmasks.


And the problem with *all* these functions is that they return a
sal_Bool, and should thus be normalised to sal_True or sal_False. But
they are not, and can return any non-zero value (or maybe it is always
"1"? I think that's how C++ coerces a bool to an integer type) rather
than sal_True (which is -1).

-- 
Lionel