FreeTDS Development Group

[Frediano Ziglio <freddy77 AT gmail.com>]

2009/3/10 James K. Lowden <jklowden AT freetds.org>:
> Frediano Ziglio wrote:
>>        if (host_term == NULL && host_termlen > 0
>>         || host_term != NULL && host_termlen == 0) {
>>                 dbperror(dbproc, SYBEVDPT, 0);  /* "all
>> variable-length data must have either a length-prefix ..." */
>>                 return FAIL;
>>         }
>>
>> I removed a similar warning in dblib.c, see
>> http://freetds.cvs.sourceforge.net/viewvc/freetds/freetds/src/dblib/dblib.c?r1=1.343&r2=1.344&sortby=date
>> which IMHO is more readable.
>
> Indeed!
>
>> Probably code should be
>>
>>         if (host_term == NULL && host_termlen > 0
>>          || host_term != NULL && host_termlen <= 0) {
>
> That looks better.  I wonder, does
>
>        host_term == NULL && host_termlen > 0
>
> matter?  Sybase says, "To avoid using a terminator, set this parameter to
> NULL."  What harm if the user sets host_termlen to something other than 0
> or -1?
>
> It's a choice between protecting the library and protecting the
> programmer.  On one hand, if either host_term is NULL or host_termlen is
> [0,-1], there's no terminator.  On the other hand, if the programmer
> provides inconsistent information, maybe he's confused and should get an
> error.  I don't know what the right answer is.
>
> Regarding the nanny worrywart gcc recommendation about parenthesis, I
> guess you know my opinion.  One compiler issues that warning if certain
> options are used.  I don't see that as a reason to litter the code with
> parentheses in expressions that are perfectly clear without them.
>
> I wrote to gcc about it, and I know they agree that finer-tuned warnings
> would be an improvement.  I hope they'll remove this one from -Wall, but
> I'm not sure I won that argument.
>

Mumble mumble.... it seems that MS use termlen == 0 for no terminator
while Sybase use -1. Honestly I don't understand the difference
between NULL terminator and terminator with length == 0.... looking at
the code behavior is the same. I would test initially that termlen >=
-1 so to avoid -2 or so on. And I would change test with

if (host_termlen <= 0)
  host_term = NULL;
if (host_termlen > 0 && host_term == NULL) {

also look at error

dbperror(dbproc, SYBEVDPT, 0);  /* "all variable-length data must have
either a length-prefix ..." */

this has nothing to do with length-prefix !!! Here we just test to
check host_termlen/host_term consistency !!!

Test should be

if (host_term == NULL && host_prefixlen <= 0) {
   dbperror(dbproc, SYBEVDPT, 0);  /* "all variable-length data must
have either a length-prefix ..." */
   return FAIL;
}

another problem in bcp_colfmt is that there's no check for
host_colnum, looking at

hostcol = dbproc->hostfileinfo->host_columns[host_colnum - 1];

host_colnum should be <= dbproc->hostfileinfo->host_colcount but this
test is missing

Another issue is the way we manage terminator copy, consider memory error:

hostcol->host_column = host_colnum;
hostcol->datatype = host_type;
hostcol->prefix_len = host_prefixlen;
hostcol->column_len = host_collen;
if (host_term && host_termlen >= 0) {
        free(hostcol->terminator);
        if ((hostcol->terminator = malloc(host_termlen)) == NULL) {
                dbperror(dbproc, SYBEMEM, errno);
                return FAIL;
        }
        memcpy(hostcol->terminator, host_term, host_termlen);
}
hostcol->term_len = host_termlen;
hostcol->tab_colnum = table_colnum;

this code would initialize partially hostcol and free
hostcol->terminator leaving a dundling pointer. I would allocate
memory at first line and manage error ASAP so to leave hostcol in a
consistent state.

freddy77