FreeTDS Development Group

["ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>]

Before starting separating code in net.c I would like to discuss this
function it doesn't seem so good....

@ /**
@  * Loops until we have received buflen characters 
@  * return -1 on failure 
@  */

>From this documentation it seems if we are not able to read all buf len
we should just return -1 and close connection (we are not able to detect
a partial read...). However we handle case where we are not able to read
all characters, so needs some adjustement

@ static int
@ goodread(TDSSOCKET * tds, unsigned char *buf, int buflen)
@ {
@       int got = 0;
@       int len, retcode;
@       fd_set fds;
@       time_t start, now;
@       struct timeval selecttimeout;
@ 
@       FD_ZERO(&fds);
@       start = time(NULL);
@       now = start;
@       /* nsc 20030326 The tds->timeout stuff is flawed and should
probably just be removed. */

I not agree with this comment, if chkintr == NULL and hndlintr == NULL
we should use a proper timeout, not a 1 second polling...

@       while ((buflen > 0) && ((tds->timeout == 0) || ((now - start) <
tds->timeout))) {
@               assert(tds);

Well, we already test tds->timeout so we test tds == NULL after the
core.. not that useful

@               if (IS_TDSDEAD(tds))
@                       return -1;
@ 
@               FD_SET(tds->s, &fds);
@               selecttimeout.tv_sec = 1;
@               selecttimeout.tv_usec = 0;
@ 

Here we should add test for chkintr/hndlintr. We should not comsume CPU
if not necessary... well.. it's just a paranoia...

@               retcode = select(tds->s + 1, &fds, NULL, NULL,
&selecttimeout);        /* retcode == 0 indicates a timeout, OK */
@ 
@               if( retcode != 0 ) {
@                       if( retcode < 0 ) {
@                               if (sock_errno != TDSSOCK_EINTR) {
@                                       char *msg =
strerror(sock_errno);
@                                       tdsdump_log(TDS_DBG_NETWORK, "%L
goodread select: errno=%d, \"%s\", returning -1\n", errno, (msg)? msg :
"(unknown)");
@                                       return -1;
@                               }
@                               goto OK_TIMEOUT;
@                       } 
@                       /*
@                        * select succeeded: let's read.
@                        */
@ #                     ifndef MSG_NOSIGNAL
@                       len = READSOCKET(tds->s, buf + got, buflen);
@ #                     else
@                       len = recv(tds->s, buf + got, buflen,
MSG_NOSIGNAL);
@ #                     endif
@ 
@                       if (len < 0) {
@                               char *msg = strerror(sock_errno);

Does this works under windows... I don't this so, however it's used only
for log...

@                               tdsdump_log(TDS_DBG_NETWORK, "%L
goodread: errno=%d, \"%s\"\n", errno, (msg)? msg : "(unknown)");
@                               

sock_errno, not errno... still used for log...

@                               switch (sock_errno) {
@                               case EAGAIN:            /* If O_NONBLOCK
is set, read(2) returns -1 and sets errno to [EAGAIN]. */
@                               case TDSSOCK_EINTR:             /* If
interrupted by a signal before it reads any data. */
@                               case TDSSOCK_EINPROGRESS:       /* A
lengthy operation on a non-blocking object is in progress. */
@                                       /* EINPROGRESS is not a
documented errno for read(2), afaict.  Remove following assert if it
trips.  --jkl */
@                                       assert(sock_errno !=
TDSSOCK_EINPROGRESS);
@                                       break;  /* try again */
@ 

However some lines below we decrement buflen using len... this can cause
a buffer underflow, we should just jump to OK_TIMEOUT (very rare case
but can happen)

@                               case EBADF:
@                               /*   EBADMSG: not always defined */
@                               case EDEADLK:
@                               case EFAULT:
@                               case EINVAL:
@                               case EIO:
@                               case ENOLCK:
@                               case ENOSPC:
@                               case ENXIO:
@                               default:
@                                       return -1;
@                                       break;
@                               }
@                       }
@ 
@                       if (len == 0) 
@                               return -1;
@ 

This mean that client disconnect (so is correct to return -1) or socket
no-blocking and no data however is impossible since select detect
data.... IMHO a better implementation is to use no-blocking socket
always and call select if needed, so if we have data we call only a
system call (another paranoia :O ).

@                       buflen -= len;
@                       got += len;
@               } 
@ 
@               /* When we get a timeout on select(), return 0. Don't
return -1, because
@                * that would lead to a disconnect 
@                * OTOH, do not let this pass to prevent an infinite
loop when there is
@                * no data on the wire */
@               if (retcode == 0)
@                       return 0;
@               

This is quite bad. If we are not able to read data in 1 second we just
return 0 (while from documentation we should never return 0). Also
probably caller disconnect detecting a error (see tds_read_packet) so
this code do not solve the problem... also returning 0 we discard
already readed data (got bytes) and we lose syncro with server.

@               OK_TIMEOUT:
@               now = time(NULL);
@               if (tds->longquery_func && tds->queryStarttime &&
tds->longquery_timeout) {
@                       if ((now - (tds->queryStarttime)) >=
tds->longquery_timeout) {
@                               (*tds->longquery_func)
(tds->longquery_param);
@                               return got;
@                       }
@               }

Well.. on query timeout we just return readed data... does caller handle
this case in a correct way ??

@               if ((tds->chkintr) && ((*tds->chkintr) (tds)) &&
(tds->hndlintr)) {
@                       switch ((*tds->hndlintr) (tds)) {
@                       case TDS_INT_EXIT:
@                               exit(EXIT_FAILURE);
@                               break;
@                       case TDS_INT_CANCEL:
@                               tds_send_cancel(tds);
@                               break;

Should FreeTDS process cancel or just send it to server?

@                       case TDS_INT_CONTINUE:
@                       default:
@                               break;
@                       }
@               }
@ 
@       }                       /* while buflen... */
@ 
@       if (tds->timeout > 0 && now - start < tds->timeout && buflen >
0)
@               return -1;
@ 

Mmm... 
tds->timeout > 0 --> we have a timeout
now - start < tds->timeout --> timeout DO NOT occur... is this test
correct ? I don't understand why ...
buflen > 0 --> some data left. Here we return fail if we are not able to
read all data... different behaviur from query timeout...

while test exit if (buflen <= 0) || ((tds->timeout > 0) && ((now -
start) >= tds->timeout)) so this test should always fails...

@       assert(buflen == 0);

??? I don't think this case is impossible....

@       return (got);
@ }
@ 

freddy77