Skip to Content.
Sympa Menu

freetds - [freetds] goodread... not so fine

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: "ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>
  • To: <freetds AT lists.ibiblio.org>
  • Subject: [freetds] goodread... not so fine
  • Date: Wed, 14 Jan 2004 11:04:15 +0100

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




Archive powered by MHonArc 2.6.24.

Top of Page