FreeTDS Development Group

["Lowden, James K" <LowdenJK AT bernstein.com>]

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

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

> > > (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;

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.  

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

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

Regards, 

--jkl
-----------------------------------------
The information contained in this transmission may contain privileged and 
confidential information and is intended only for the use of the person(s) 
named above. If you are not the intended recipient, or an employee or agent 
responsible for delivering this message to the intended recipient, any 
review, dissemination, distribution or duplication of this communication is 
strictly prohibited. If you are not the intended recipient, please contact 
the sender immediately by reply e-mail and destroy all copies of the original 
message. Please note that we do not accept account orders and/or instructions 
by e-mail, and therefore will not be responsible for carrying out such orders 
and/or instructions.