FreeTDS Development Group

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

On 06 Aug 2003 07:59:51 +0200, Frediano Ziglio <freddyz77 AT tin.it> wrote:
> Il mar, 2003-08-05 alle 23:13, Lowden, James K ha scritto:
> >     $ grep -l HAVE_ICONV include/*.h src/*/*.c
> >     include/tds.h
> 
> This should be changed cause is a bug...

Acknowledged. 

> > Before:
> >     if (TDS7_PLUS(tds)) {
> >     #       if HAVE_ICONV
> >                     iconv
> >     #       else
> >                     strip/add high byte
> >     #       endif
> >     }
> > 
> > Now tds_iconv() makes all the decisions.  
> 
> Ok. However test are still in tds_put_string, tds_get_data and so on..

Granted.  I'm not done, but I think this is the right road.  

I got a little gun-shy after breaking things in a big way.  I'll stir
things up again after my vacation. ;-)

> >     socket << iconv << "hello" << noiconv << 103 << endl;
> 
> But this require C++ and, from my point of view exceptions and so on...

I think those are good things, but it would be a big undertaking.  

> > read.c::read_and_convert() works very much that way.  It provides its
> > local buffer to tds_get_n(), which fills it, and passes that buffer to
> > tds_iconv(), passing it the caller's output buffer.  I think that's
> > the minimum number of copies.  write.c::tds_put_string() does much the
> > same: it hands its input buffer to tds_iconv(), providing a local
> > buffer for iconv's output, then passes that buffer to tds_put_n(). 
> > One copy, unavoidable.  I don't really understand the memcpy() in
> > tds_put_n(), but that's a different matter.  
> 
> The minimun number of copy ?? Using TDSWRITE for writing directly to
> packet buffer don't do any copy

I think I understand you.  tds_put_string() tells tds_iconv() to write to
its local staging buffer, outbuf.  Then it calls tds_put_n(), which copies
the data to the socket's output buffer (TDSSOCKET::out_buf).  That's an
extra copy.  When out_buf fills up, tds_put_n() writes the packet and
continues.  

(You care about these efficiencies more than I do.)  

To avoid calling tds_put_n() and its memcpy(), teach tds_put_string() to
write directly to tds->out_buf, calling tds_write_packet() and
tds_init_write_buf() itself.  See the attached patch, which adds a net of
2 lines of code.  (Note: not compiled.  Just an illustration.)  Also note
the IS_TDS7_PLUS test is gone, too, because we might as well have
tds_iconv() write to tds->out_buf unconditionally.  

> > I assert you can't make read_and_convert() substantially more
> > efficient, certainly not via the abstraction you've described. 
> > Perhaps you can show me I'm wrong about that, or pick something else
> > that would benefit more?  
> 
> IMHO main benefit is you write conversion stuff in a single place. No
> more loops spread in read.c, write.c, bcp.c or other...

I think the number of loops will be limited.  Debugging a callback system
is no picnic, either.   

> > A lot of the buffer-to-buffer conversions happen in login.c, and those
> > are small predictable strings.  
> 
> However are dynamic. 0.53 had limitation of 30 characters for these
> buffer. Some time later someone required a longer password so we used
> dynamic. I agree these fields are limited (128 for hostname, 128 for
> database and so on).
> Perhaps two convert functions like
>   tds_iconv1(TDSSOCKET*, TDSICONVINFO*, const char* src, size_t src_len,
> char* dest, size_t* dest_len);
> for simple conversions
>   tds_iconv2(TDSSOCKET*, TDSICONVINFO*, TDSREAD* , TDSWRITE* dest);
> for longer ones

Remember, I was attracted to "tds_iconv2" in April, too.  It *seems*
reasonable to provide a function instead of a buffer.  Having worked over
the code, though, I'm convinced it's an illusory gain because 1) there
aren't many places to use it, 2) the setup and teardown are significant,
and 3) it's a complex abstraction that makes the employment of iconv(3)
harder to understand and debug.  The KISS principle tells me, Don't do it.
 

My approach removes the buffer copy by adding 2 lines of code, and
removing a layer in the call stack.  If you can do better, please show me!
:-)

--jkl

Attachment: write.c.diff
Description: Binary data