Re: Compiler warnings in psqloodbc 08.03.0200

Oops, the actual fix is attached.

Zoltan Boszormenyi írta:
> Hi,
>
> here's the fix for all non-pointer-signedness warnings,
> against 08.03.0300 that was released meanwhile. Now
> the compilation only emits 246 "differ in signedness"
> warnings, which is still too much noise. I agree with
> Tom Lane that those should be cleaned up if for nothing
> else, than the real bugs don't get lost in the noise.
>
> The patch cleans up such warnings in several files:
> "label 'cleanup' defined but not used" and
> "unused variable 'func'"
>
> In convert.c::convert_escape():
> "'param_consumed' may be used uninitialized in this function"
> The variable "param_consumed" was not assigned a value
> in all cases by processParameters() upon returning,
> so I tried to fix it there. Now it does, please review.
>
> In descriptor.c::TI_Constructor():
> "the address of 'qual' will always evaluate as 'true'"
> STRX_TO_NAME() has to be used instead of STR_TO_NAME.
>
> In drvconn.c::dconn_get_attributes():
> "'last' may be used uninitialized in this function"
> The first parameter to strtok_r() may be NULL if
> strdup() returns NULL. In that case strtok_r() may
> corrupt unknown memory areas.
>
> In pgapi30.c, two instances of
> "cast from pointer to integer of different size"
>
> In psqlodbc.c()::finalize_global_cs() is only used inside
> "#ifdef WIN32" but was defined outside causing a
> "defined but not used" warning.
>
> Some notes:
> - Instead of using 'CSTR func = "funcname";' everywhere,
>   it would be better to use the __FUNCTION__ pre-processor
>   macro. This way, the "unused variable 'func'" is eliminated
>   once and for all as __FUNCTION__ is available everywhere.
> - The sea of "differ in signedness" warnings are caused by
>   the difference of "{SQL|U}CHAR *" and plain "char *".
>   Many ODBC API calls expect "SQLCHAR *" input.
>   Using strcpy(), strcmp(), etc. on them causes warnings.
>   Many internal psqlODBC functions expect a character input
>   and they are also used inconsistenly with different
>   signed and unsigned parameters.
>   Either the API parameters has to be treated internally
>   as "char *" and keep a local variable for this purpose,
>   or pass the SQLCHAR * down in other functions which
>   have to be declared with the appropriate header.
>   Fixing it either way would be quite invasive in terms
>   of patch size. The latter would mean smaller and
>   cleaner C source.
>
> Best regards,
> Zoltán Böszörményi
>
> Hiroshi Saito írta:
>
>> Hi.
>>
>> Surely, we have not made offer sufficient about x86_64.... However,
>> The check of operation
>> was performed by the temporary rental machine. Moreover, there is also
>> a situation of taking
>> time in the check of the present BIGENDIAN. Then, late work may be
>> blamed....
>> Anyway, In order to avoid a problem, to be warning should clear.
>>
>> BTW, FreeBSD x86 is this.
>> inet% gmake socket.o
>> if gcc -DHAVE_CONFIG_H -I. -I. -I.   -I/usr/local/include
>> -I/usr/local/pgsql/include -Wall -g -O2 -MT socket.o -MD -MP -MF
>> ".deps/socket.Tpo" -c -o socket.o socket.c; \
>> then mv -f ".deps/socket.Tpo" ".deps/socket.Po"; else rm -f
>> ".deps/socket.Tpo"; exit 1; fi
>> socket.c: In function `SOCK_wait_for_ready':
>> socket.c:468: warning: 'no_timeout' might be used uninitialized in
>> this function
>>
>> Regards,
>> Hiroshi Saito
>>
>> ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us>
>>
>>
>>
>>> I wrote:
>>>
>>>> BTW, isn't anyone paying attention to compiler warnings in this code
>>>> base?
>>>>
>>> To be concrete, attached is a list of warnings I see when building .0200
>>> using gcc -Wall on an x86_64 Fedora 9 machine.  If I were in charge of
>>> this project I'd insist on every one of these being cleaned up --- they
>>> are at least evidence of sloppy programming, and a significant fraction
>>> look like they mean certain crashes if the code ever gets executed.
>>>
>>> BTW, I've omitted 261 "pointer targets differ in signedness" warnings...
>>> those are probably not interesting, but I'd still recommend cleaning
>>> them up, if only so that more important warnings don't get lost in the
>>> noise.  The core Postgres project has maintained a zero-tolerance policy
>>> on gcc warnings for years, and I think it's served us well.
>>>
>>> regards, tom lane
>>>
>>
>
>
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

diff -durpN psqlodbc-08.03.0300.orig/bind.c psqlodbc-08.03.0300/bind.c
--- psqlodbc-08.03.0300.orig/bind.c    2008-09-22 12:13:05.000000000 +0200
+++ psqlodbc-08.03.0300/bind.c    2008-09-29 10:25:18.000000000 +0200
@@ -620,7 +620,9 @@ reset_a_iparameter_binding(IPDFields *se
 int
 CountParameters(const StatementClass *self, Int2 *inputCount, Int2 *ioCount, Int2 *outputCount)
 {
+#if 0
     CSTR func = "CountParameters";
+#endif
     IPDFields    *ipdopts = SC_get_IPDF(self);
     int    i, num_params, valid_count;

diff -durpN psqlodbc-08.03.0300.orig/convert.c psqlodbc-08.03.0300/convert.c
--- psqlodbc-08.03.0300.orig/convert.c    2008-09-25 14:25:03.000000000 +0200
+++ psqlodbc-08.03.0300/convert.c    2008-09-29 09:45:04.000000000 +0200
@@ -4157,18 +4157,23 @@ processParameters(QueryParse *qp, QueryB
         size_t *output_count, SQLLEN param_pos[][2])
 {
     CSTR func = "processParameters";
-    int retval, innerParenthesis, param_count;
+    int retval, innerParenthesis, param_count, return_count;
     BOOL stop;

     /* begin with outer '(' */
     innerParenthesis = 0;
     param_count = 0;
+    return_count = 0;
     stop = FALSE;
     for (; F_OldPos(qp) < qp->stmt_len; F_OldNext(qp))
     {
         retval = inner_process_tokens(qp, qb);
         if (retval == SQL_ERROR)
+        {
+            if (output_count)
+                *output_count = return_count;
             return retval;
+        }
         if (ENCODE_STATUS(qp->encstr) != 0)
             continue;
         if (qp->in_identifier || qp->in_literal || qp->in_escape)
@@ -4203,8 +4208,7 @@ processParameters(QueryParse *qp, QueryB
                     param_pos[param_count][0] =
                     param_pos[param_count][1] = -1;
                 }
-                if (output_count)
-                    *output_count = F_NewPos(qb);
+                return_count = F_NewPos(qb);
                 break;

             case '}':
@@ -4215,6 +4219,8 @@ processParameters(QueryParse *qp, QueryB
         if (stop) /* returns with the last } position */
             break;
     }
+    if (output_count)
+        *output_count = return_count;
     if (param_pos[param_count][0] >= 0)
     {
         mylog("%s closing ) not found %d\n", func, innerParenthesis);
diff -durpN psqlodbc-08.03.0300.orig/descriptor.c psqlodbc-08.03.0300/descriptor.c
--- psqlodbc-08.03.0300.orig/descriptor.c    2007-11-26 13:24:10.000000000 +0100
+++ psqlodbc-08.03.0300/descriptor.c    2008-09-29 10:50:49.000000000 +0200
@@ -33,7 +33,7 @@ void    TI_Constructor(TABLE_INFO *self, co

         STR_TO_NAME(self->bestitem, OID_NAME);
         sprintf(qual, "\"%s\" = %%u", OID_NAME);
-        STR_TO_NAME(self->bestqual, qual);
+        STRX_TO_NAME(self->bestqual, qual);
         TI_set_hasoids(self);
         TI_set_hasoids_checked(self);
     }
@@ -584,7 +584,9 @@ static struct

 static    PG_ErrorInfo    *DC_create_errorinfo(const DescriptorClass *desc)
 {
+#if 0
     CSTR func = "DC_create_erroinfo";
+#endif
     PG_ErrorInfo    *error;
     ConnectionClass    *conn;
     EnvironmentClass    *env;
diff -durpN psqlodbc-08.03.0300.orig/drvconn.c psqlodbc-08.03.0300/drvconn.c
--- psqlodbc-08.03.0300.orig/drvconn.c    2008-09-22 12:13:12.000000000 +0200
+++ psqlodbc-08.03.0300/drvconn.c    2008-09-29 11:10:17.000000000 +0200
@@ -432,6 +432,8 @@ dconn_get_attributes(copyfunc func, cons

     our_connect_string = strdup(connect_string);
     strtok_arg = our_connect_string;
+    if (strtok_arg == NULL)
+        return;

 #ifdef    FORCE_PASSWORD_DISPLAY
     mylog("our_connect_string = '%s'\n", our_connect_string);
diff -durpN psqlodbc-08.03.0300.orig/environ.c psqlodbc-08.03.0300/environ.c
--- psqlodbc-08.03.0300.orig/environ.c    2007-11-26 13:24:10.000000000 +0100
+++ psqlodbc-08.03.0300/environ.c    2008-09-29 10:25:47.000000000 +0200
@@ -531,7 +531,7 @@ EN_Constructor(void)
 #endif /* WIN32 */

     rv = (EnvironmentClass *) malloc(sizeof(EnvironmentClass));
-cleanup:
+
     if (rv)
     {
         rv->errormsg = 0;
diff -durpN psqlodbc-08.03.0300.orig/info.c psqlodbc-08.03.0300/info.c
--- psqlodbc-08.03.0300.orig/info.c    2008-09-25 14:25:05.000000000 +0200
+++ psqlodbc-08.03.0300/info.c    2008-09-29 10:24:34.000000000 +0200
@@ -824,7 +824,6 @@ mylog("CONVERT_FUNCTIONS=" FORMAT_ULEN "

     if (pcbInfoValue)
         *pcbInfoValue = (SQLSMALLINT) len;
-cleanup:

     return result;
 }
@@ -969,7 +968,6 @@ inolog("serial in\n");
         }
     }

-cleanup:
 #undef    return
     /*
      * also, things need to think that this statement is finished so the
@@ -3484,7 +3482,7 @@ PGAPI_ColumnPrivileges(
     extend_column_bindings(SC_get_ARDF(stmt), 8);
     /* set up the current tuple pointer for SQLFetch */
     result = SQL_SUCCESS;
-cleanup:
+
     /* set up the current tuple pointer for SQLFetch */
     stmt->status = STMT_FINISHED;
     stmt->currTuple = -1;
diff -durpN psqlodbc-08.03.0300.orig/pgapi30.c psqlodbc-08.03.0300/pgapi30.c
--- psqlodbc-08.03.0300.orig/pgapi30.c    2008-09-25 14:25:06.000000000 +0200
+++ psqlodbc-08.03.0300/pgapi30.c    2008-09-29 10:54:39.000000000 +0200
@@ -398,7 +398,9 @@ PGAPI_GetConnectAttr(HDBC ConnectionHand
             SQLINTEGER Attribute, PTR Value,
             SQLINTEGER BufferLength, SQLINTEGER *StringLength)
 {
+#if 0
     CSTR func = "PGAPI_GetConnectAttr";
+#endif
     ConnectionClass *conn = (ConnectionClass *) ConnectionHandle;
     RETCODE    ret = SQL_SUCCESS;
     SQLINTEGER    len = 4;
@@ -1664,7 +1666,7 @@ PGAPI_SetConnectAttr(HDBC ConnectionHand
             unsupported = TRUE;
             break;
         default:
-            ret = PGAPI_SetConnectOption(ConnectionHandle, (SQLUSMALLINT) Attribute, (SQLLEN) Value);
+            ret = PGAPI_SetConnectOption(ConnectionHandle, (SQLUSMALLINT) Attribute, CAST_PTR(SQLLEN, Value));
     }
     if (unsupported)
     {
@@ -1870,7 +1872,7 @@ inolog("set ard=%p\n", stmt->ard);
             SC_get_ARDF(stmt)->size_of_rowset = CAST_UPTR(SQLULEN, Value);
             break;
         default:
-            return PGAPI_SetStmtOption(StatementHandle, (SQLUSMALLINT) Attribute, (SQLULEN) Value);
+            return PGAPI_SetStmtOption(StatementHandle, (SQLUSMALLINT) Attribute, CAST_PTR(SQLULEN, Value));
     }
     return ret;
 }
diff -durpN psqlodbc-08.03.0300.orig/psqlodbc.c psqlodbc-08.03.0300/psqlodbc.c
--- psqlodbc-08.03.0300.orig/psqlodbc.c    2008-05-11 20:42:24.000000000 +0200
+++ psqlodbc-08.03.0300/psqlodbc.c    2008-09-29 11:32:20.000000000 +0200
@@ -92,6 +92,7 @@ int    initialize_global_cs(void)
     return 0;
 }

+#ifdef WIN32
 static void finalize_global_cs(void)
 {
     DELETE_COMMON_CS;
@@ -104,7 +105,6 @@ static void finalize_global_cs(void)
 #endif /* _DEBUG */
 }

-#ifdef WIN32
 HINSTANCE NEAR s_hModule;        /* Saved module handle. */
 /*    This is where the Driver Manager attaches to this Driver */
 BOOL        WINAPI