FreeTDS Development Group

[Frediano Ziglio <freddyz77 AT tin.it>]

Il mar, 2003-08-05 alle 23:13, Lowden, James K ha scritto:
> > From: Frediano Ziglio [mailto:freddyz77 AT tin.it]
> > Sent: August 5, 2003 3:26 PM
> > Il mar, 2003-08-05 alle 19:06, Lowden, James K ha scritto:
> > > 
> > > Freddy, I think we might be working at cross purposes.  I 
> > > don't see any
> > > value in isolating iconv_t or the effects of iconv().  On 
> > > the contrary, I think we should embrace it.  
> > 
> > Well:
> > - win32 don't have iconv
> > - Mac OS X don't have iconv
> > - older unix do not have iconv
> > so why embrace iconv ??
> 
> Because it's a good abstraction.  
> 
> For ISO-8859-1, replacements/iconv.c.  
> 
> For real conversion, GNU installs on any Unix.  
> 
> For Mac, M$, and VMS, I think it will be easier to write iconv based on the
> OS's native functions than to adapt FreeTDS to each of them.  Mark Lilback,
> for one, certainly agrees.  
> 
> > > I see value in tds_iconv() primarily to localize the 
> > selection of iconv_t
> > > handles, and as a place to generate client-side messages.  
> > > 
> > > One way to deal with character set conversions is the "old" way:
> > > 
> > > 1.  Determine if conversion is necessary.
> > > 2.  Determine if conversion is possible (HAVE_ICONV).
> > > 3.  Branch accordingly.  
> > 
> > However you cannot include tdsiconv.h in tds.h cause tdsiconv.h need
> > HAVE_ICONV that need config.h. It's very bad including 
> > config.h 
> 
> I agree entirely that too much information is exposed though our headers.  I
> understand your point about including (directly or indirectly) config.h and
> tdsiconv.h in applications.  I would like to go further: tds.h should not be
> included in sybdb.h, for instance.  
> 
> Application shouldn't care what the structure of DBPROCESS is.  They never
> dereference the pointer, so simplest would be to call it a void* in sybdb.h.
> If we proceeded similarly with the handles in ct-lib and ODBC, we would
> drastically reduce the number of typedefs needed by applications.  That
> would be a good step, but it's tedious work.  
> 

Well, ODBC driver do not export any include so it's ok...
I agree to hidden structures in dblib/ctlib.

> > (or have
> > to add -DHAVE_ICONV to all programs compiled with FreeTDS), 
> > so it's bad
> > to include tdsiconv.h in tds.h (cause libraries need to 
> > install tds.h).
> 
> We don't refer to HAVE_ICONV very often:
> 
>       $ grep -l HAVE_ICONV include/*.h src/*/*.c
>       include/tds.h

This should be changed cause is a bug...

>       include/tdsiconv.h
>       src/replacements/iconv.c
>       src/tds/config.c
>       src/tds/iconv.c
> 
> because if it's not there, we use our replacement.  
> 
> > > This sprinkles "if unicode" and "if have iconv" all over the code.  
> > > 
> > > My way, the "new" way (so far):
> > > 
> > > 1.  Convert everything.  
> > > 2.  (Let the converter determine what's feasible/necessary.)
> > 
> > ?? I don't follow you ...
> 
> 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..

> > > > (I found also an old mail with same hints...)
> > > > Mainly we need some functions
> > > > - initialization
> > > > - free
> > > > - getting a conversion (should return just a undefined pointer)
> > > > how to retrieve/cache server/default conversions ??
> > > > - convert (using previous pointer)
> > > 
> > > We have all this.  
> > 
> > Yes, but initialization free is spread between iconv.c and 
> > mem.c. 
> 
> I don't see how that's especially bad.  It could probably be done better.  
> 
> > Using an abstract layer we can use 
> > #ifdef HAVE_ICONV in a single file (iconv.c). 
> 
> You have to determine whether or not to include the replacement, whether or
> not to define iconv_t and EILSEQ.  To succeed in confining HAVE_ICONV to a
> single file, you'll have to export something instead of iconv_t, and you
> won't have gained anything.  
> 
> > We have also 2 convert function (one for buffer and one for
> > file) and function that need to convert a long buffer are 
> > very hard (you can see from all fix we wrote).
> 
> And you propose to simplify them by adding another layer of abstraction and
> callback functions?  I think the complexity is irreducible, and you're just
> moving it around.  
> 
> One of the tricky parts is coping with the partial characters at the end of
> a "chunk" (e.g. UTF-8).  Your abstract function is going to have to do that,
> too.  
> 
> > > Are you sure you want to do this?  
> > 
> > Well. TDSREAD and TDSWRITE are abstract. I take the idea from C++
> > iostream implementation. 
> 
> I understand.  It would be good if we had sockets+streams+iconv all in C++,
> as long as we had a manipulator to turn iconv conversion on & off:
> 
>       socket << iconv << "hello" << noiconv << 103 << endl;
> 

But this require C++ and, from my point of view exceptions and so on...

> Do that and we can toss out a lot of ugly C code!  
> 
> > TDSWRITE can be a writer for packet so instead
> > of converting to a buffer and then copy to packet buffer and send to
> > network you can copy directly to packet buffer and flush. TDSWRITE can
> > be a static buffer so TDSWRITE::write do nothing or it can be 
> > a dynamic
> > buffer so TDSWRITE::write can extend buffer (still a FIXME in
> > tds_get_data...). Same for TDSREAD...
> 
> 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 (unless you count copy from user to
kernel for socket...).

> > In future version (0.63 ??) I think to do a large use of such data
> > (TDSREAD and TDSWRITE), mainly to remove memory problems converting
> > data...
> 
> I understand the motivation.  I accept that we sometimes copy things more
> than we need to.  I just don't think the functions I cited will benefit from
> the treatment you propose.  I think the remedy lies elsewhere.  
> 
> 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...

> > > Most calls to tds_iconv do/will use small 
> > > fix-length buffers and so won't require a loop.  
> > 
> > We use also a lot of dynamic strings (sql statement, text, 
> > xvarchar)...
> 
> They almost always go through tds_put_string() or tds_get_string().  That's
> why I consider the problem mostly solved.  
> 
> 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
??

freddy77