FreeTDS Development Group

["James K. Lowden" <jklowden AT freetds.org>]

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.  

> 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.  

> 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).  

> 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.  

> sleep?? why sleep??

My bad.  I should gotten some sleep instead.  

> > 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.  

>  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.

> - 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.  

Having looked at this in daylight hours, I see Sybase is consistent:  

While waiting, libtds invokes chkintr.  I've currently set that to happen
every 1000 ms.  libtds calls a client library function which invokes the
function installed by dbsetinterrupt(), if any.  Sybase defines the
application's chkintr function to return one of these values:

        INT_EXIT -- call exit(3)
        INT_CANCEL -- cause the API function to fail 
        INT_CONTINUE -- continue waiting

On timing out, libtds invokes errhandle indirectly by calling a client
library function that calls the application's errhandle (if installed). 
errhandle returns a value to the caller; that value is defined by Sybase
as one of:

        INT_EXIT -- call exit(3)
        INT_CANCEL -- cause the API function to fail 
        INT_TIMEOUT -- send a cancel packet
        INT_CONTINUE -- continue waiting one more period

So: INT_CANCEL always means FAIL.  It doesn't always mean "close the
connection"; it means that only when we've given up waiting for the
server.  tds_iconv is an example of a libtds function that might respond
to INT_CANCEL without closing the connection.  

> - minor: perror is terrible :)

Probably.  I think we should use it to highlight FreeTDS bugs, like
assert(3).  

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

Grazie.

> > 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!)  

> 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. 

Thanks for the critique.  

Regards, 

--jkl