FreeTDS Development Group

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

> 
> ZIGLIO, Frediano, VF-IT wrote:
> > > Freddy, I would especially like your opinion on a new function:
> > > net.c::tds_select().  
> > 
> > Well, I forget... why tdserror and not tds_error?? Why 
> TDSECLOS and not
> > TDS_ECLOS (or TDS_ERR_CLOSE) ?
> 
> tdserror is patterned on perror and dbperror.  I've been 
> reading a lot of
> C and Unix stuff lately.  
> 
> TDSECLOS is patterned on SYBECLOS.  A better example is TDSETIME,
> patterned on SYBETIME, patterned on ETIME in errno.h.  
> 
> I think it's a good idea to stick close to Sybase and Unix conventions
> with these names, old-fashioned though they are.  Someone 
> familiar with
> Unix network programming (or even db-lib programming) will 
> recognize them
> easily.  
> 
>       "If I had to do it over again?  Hmm...  I guess I'd 
> spell 'creat' with an
> 'e'." 
>       -- Ken Thompson (on the Unix operating system)
> 
> You could be right.  
> 

I think that libTDS have to support dblib/ctlib/odbc but IMHO should not
follow them patterns or syntax. Perhaps the real questions are:
Is underscore part of libTDS function prefix?
Is underscore part of libTDS enumeration prefix?

> > Looking at diff it appear that there are many place where we catch
> > tdserror result and handle it. 
> 
> Part of the idea behind tds_select.  ;-)  By using it, I hope 
> to simplify
> the code.  
> 

Looking at current code I can say that now I see tds_select advantages.

> > Also in many places the only options are
> > exiting and close socket (think at connection error or read socket
> > error) but on timeout/continue request we call exit. I 
> would prefer a
> > connection close instead of a exit program default. 
> Personally I think
> > that only dblib can return a TDS_INT_EXIT to I think it 
> would be easier
> > to call exit directly from dblib (writing 
> exit(EXIT_FAILURE) is as fast
> > as writing return TDS_INT_EXIT).
> 
> Good suggestion.  If the application's error handler returns INT_EXIT,
> db-lib can exit.  No need to tell libtds.  
> 
> Also I think tds_select (or maybe tdserror) should close the 
> connection
> and mark it DEAD if there's no other choice.  No need to 
> leave that to the
> caller.  
> 
> Do you think it's possible for tds_select to call 
> tds_send_cancel?  If so,
> tds_select need return only >1 (OK) or <1 (FAIL).  
> 

Mmmm... I think you should check what happen if tds_send_cancel is
called inside tds_goodwrite writing another packet... I think that in
this situation we could send garbage...
I was thinking also about async cancellation. Perhaps if we are sending
some information instead of sending another package we should just set a
variable like in_cancel that tell "we need to send a cancel". This cause
if a signal is called while we are sending a packet we should support
calling tds_send_cancel inside the signal (this is required by
ctlib/dblib).

> > On tds_check_socket_write now the loop is not correct. As I 
> know only
> > Linux (perhaps *BSD) update selecttimeout so is not correct to avoid
> > timeout computation again.
> 
> I will change it.  The comments say it should look more like 
> goodread()
> and I agree.  
> 

I see you call tds_select before write. Well... this was the way some
months ago. I changed to call write before in an (paranoid) attempt to
reduce system calls using operating system tcp buffers. The idea is to
use always unblocking sockets and call select only when needed. 

> > sleep?? why sleep??
> 
> My bad.  I should gotten some sleep instead.  
> 

Honesty I think that most of the time sleep is used wrongly... why a
computer should just sleep some seconds... 90% of the time we sleep to
wait something but if you are not just waiting for time is better to
wait something else (this at select...).
The same thing apply to console output, a library should notify caller,
not write to console. console output is better for debug.

> > > The idea is libtds needs to be able to call back to the 
> > > client library for two purposes:
> > > 
> > > 1.  While waiting for a read from the server (cf. dbsetinterrupt).
> > > 2.  When the user-defined timeout expires (cf. dberrhandle)
> > > 
> > > The callback routines determine what libtds should do.  Docs say
> > > dbsetinterrupt applies only to reads.  Not sure why writes 
> > > wouldn't apply, too.  
> > > 
> > 
> > About tds_select
> > - do we really need another handler, this would be third 
> timeout handler
> > as we have?
> 
> dbsetinterrupt and dberrhandle are defined by Sybase.  Those 
> are the only
> ones we need.  query_timeout_func is wrong and going away.  
> 

I think that setting query_timeout to 1 and selecting interrupt/error
handler in dblib could lead to a unique libTDS handler. But perhaps now
is better to finish stuff with libTDS interrupt handler.

> >  or perhaps query_timeout_func should be removed? I though you would
> > remove query_timeout_func using just tdserror
> 
> Patience, Horatio.  
> 
> > - minor: end_ms should be unsigned
> 
> Grazie.
> 

Some consideration on tds_select
- on timeout we call tdserror than later on caller (tds_goodread or
tds_goodwrite) we call tdserror again
- we can avoid to compute tv if ptv is NULL
- we should avoid to call tds_gettime_ms twice
- tv_usec unit is microseconds so milliseconds should be multiplied by
1000
- I don't understand tv.tv_sec + tv.tv_usec > 0 condition

Perhaps this would be easier

 tv.tv_sec = poll_seconds;
 tv.tv_usec = 0;
 for (;;) {

  rc = select(nfds, readfds, writefds, exceptfds, ptv);
  if (rc > 0)
    return rc;

  if (rc < 0) {
    switch (sock_errno) {
    case TDSSOCK_EINTR:
      continue;
    default: /* documented: EFAULT, EBADF, EINVAL */
      tdsdump_log(TDS_DBG_ERROR, "error: select(2) returned 0x%x,
\"%s\"\n",
                  sock_errno, strerror(sock_errno));
      perror("Terrible error in tds_select");
      return rc;
    }
  }

  assert(rc == 0);

  if (ptv) {
    int diff_time = (int) (end_ms - tds_gettime_ms());
    if (diff_time < 0)
      break;
    tv.tv_sec  = diff_time / 1000;
    tv.tv_usec = (diff_time % 1000) * 1000;
  }

 }

> > - minor: perhaps these lines
> >     case TDS_INT_TIMEOUT:           /* send a cancel packet */
> >       return 0;
> >     case TDS_INT_CANCEL:            /* close socket and 
> abort function
> > */
> >       return -1;
> >   should be
> >     case TDS_INT_CANCEL:           /* send a cancel packet */
> >       return 0;
> >     case TDS_INT_TIMEOUT:            /* close socket and 
> abort function
> > */
> >       return -1;
> 
> No.  INT_CANCEL always mean "cause the API function to fail". 
>  For network
> functions, that means "fail *now* and don't send a cancel 
> packet", meaning
> we have to close the connection and mark the session dead.  
> 
...

Yes, you are right. 

> 
> > - minor: perror is terrible :)
> 
> Probably.  I think we should use it to highlight FreeTDS bugs, like
> assert(3).  
> 

Yes, but you can disable assert with -DNDEBUG while you can't disable
perror.

> > - minor: in "error (cf. errno)" errno should be sock_errno
> 
> Grazie.
> 

Perhaps however when someone will read documentation will ask... What's
sock_errno? Perhaps should we specify errno on POSIX and
WSAGetLastError() on Windows? And under cygwin?

> > > The error handler can return INT_TIMEOUT, meaning "send a 
> > > cancel packet to
> > > the server and wait for it to be acknowledged."  Of 
> course, the cancel
> > > might never be acknowledged, and that read might timeout.  
> > > This leads to a
> > > weird kind of recursion:
> > > 
> > > 1.  dbnextrow()
> > > 2.  timeout, call tdserror()
> > > 3.  tdserror() returns TDS_INT_TIMEOUT
> > > 4.  libtds sends cancel packet
> > > 5.  libtds waits for cancel to be acknowledged
> > > 6.  timeout, call tdserror()
> > > 7.  goto #3.  
> > 
> > I see no loop... wait for cancel it's just a continue. 
> dbnextrow calls
> > tds_process_tokens which timeout, calls tdserror return than
> > tds_process_tokens handle (this is automatic!) cancellation (that is
> > waits for cancel) timeout again but it's the same tds_process_tokens
> > which dbnextrow calls so I don't see the recursion.
> 
> Maybe it's OK then.  (Dear Reader: More eyeballs wanted!)  
> 

Well... now I understand the recursion... a timeout inside
tds_send_cancel (however tds_send_cancel can't recurse). I'm more aware
of a tds_send_cancel while sending a packet. Does this work correctly?
Does even a tds_send_cancel inside a signal is correct?

> > from query.c
> > 
> > int
> > tds_send_cancel(TDSSOCKET * tds)
> > {
> >         CHECK_TDS_EXTRA(tds);
> > 
> >         /* one cancel it's sufficient */
> >         if (tds->in_cancel || tds->state == TDS_IDLE)
> >                 return TDS_SUCCEED;
> > 
> > so 2 cancel sent are not possible
> 
> Code is wrong.  What if first cancel is lost in the network?  (Cf.
> TIME_WAIT.)  On timeout waiting for a cancel acknowledgement, 
> we must send
> another cancel request if tdserror returns TDS_INT_TIMEOUT.  What the
> server does with two cancels is up to the server.  Eventually 
> we'll get
> either an acknowledgement or EOF.  
> 
> The errhandle knows if it requested a cancel.  It can maintain its own
> in_cancel flag if it wants to.  There's no need for one in libtds. 
> IMNSHO. 
> 

As Peter said cancel can't be lost. I would add that sending cancel make
a sort of keep-alive for broken links. If packet got lost tcp stack
would repeat sending (cause ack/nack is never received) but at this
point the second timeout will catch this (timeout after cancel sent to
our kernel tcp stack) and close (correctly) the connection.

freddy77