FreeTDS Development Group

[Frediano Ziglio <freddyz77 AT tin.it>]

Il mar, 2003-08-05 alle 19:06, Lowden, James K ha scritto:
> > From: ZIGLIO Frediano [mailto:Frediano.Ziglio AT vodafone.com]
> > Sent: August 5, 2003 10:42 AM
> > > 
> > > > Another issue about iconv. I'm going to change iconv 
> > > > stuff using an abstract interface...
> > > 
> > > You mean something along the lines of funopen(3), with I/O 
> > > function pointers instead of buffers?  That might be OK.  I 
> > > looked at it, but I didn't see much leverage.  Everything 
> > > ultimately comes down to translating buffers, and not all the 
> > > data are subject to conversion.  Something has to interpret 
> > > the data stream and separate the character data from nonchar 
> > > data, so there's no notion of plugging iconv() into a socket.  
> > > 
> > > Could you give me a clearer idea of what you have in mind?
> > > 
> > 
> > What I aim most is to separate iconv code. tdsiconv.h and 
> > iconv_t system type should be used onlt by iconv.c.
> 
> 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 ??

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

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

> I wrote replacements/iconv.c especially so we wouldn't have to decide how to
> treat each and every character buffer.  Just convert it.  If no conversion
> was actually necessary, iconv() will copy it.  Yes, that means extra buffer
> copies for like-to-like "conversions", but it makes the code smaller and
> simpler over all.  
> 
> > (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. Using
an abstract layer we can use #ifdef HAVE_ICONV in a single file
(iconv.c). 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).

> > About funopen... I don't know... I have to resolve some 
> > problems before
> > starting coding
> > 1- how to get ratio transformation (token.c)
> > 2- avoid iconv_t use (bcp.c)
> > 2 suggest my an idea similart to funopen. Only slightly different
> > structures
> > typedef struct tds_read
> > {
> >     /** pointer to position to read */
> >     TDS_CHAR *ptr;
> >     /** pointer to buffer end */
> >     TDS_CHAR *end;
> >     /** fill again buffer */
> >     int (*read)(struct tds_read* rd);
> > } TDSREAD;
> > 
> > typedef struct tds_write
> > {
> >     /** pointer to position to write */
> >     TDS_CHAR *ptr;
> >     /** pointer to buffer end */
> >     TDS_CHAR *end;
> >     /** flush buffer */
> >     int (*write)(struct tds_read* rd);
> > } TDSWRITE;
> > 
> > int tds_conv(TDSSOCKET*, TDSICONVINFO*, TDSREAD*, TDSWRITE*); 
> > direction, others ??
> 
> Well, OK.  You're really just passing two more pointers (the read/write
> functions) and using *end instead of inbytesleft (or outbytesleft).  You
> will have to set up those structures.  Then, eventually you'll have to talk
> to iconv(3).  When you do, you'll have to:
> 
> 1.  Unpack the structures.  
> 2.  Calculate inbytesleft and outbytesleft.

Just 2 subtraction...

> 3.  Call iconv().
> 4.  Update tds_write::end and tds_read::end
> 
> Are you sure you want to do this?  
> 

Well. TDSREAD and TDSWRITE are abstract. I take the idea from C++
iostream implementation. 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...

> I think you're setting up a layer of abstraction that is opaque, making the
> code less accessible.  Anyone interested in this issue will know or can
> learn about iconv.  By placing our abstraction over it, we risk making it
> harder to understand.  I wouldn't mind that especially, except that I still
> see no gain.  I just don't think you're going to achieve any code reduction.
> 

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 think what you're trying to avoid is the loop that several functions use
> to call tds_iconv.  I think there are few such loops, all slightly
> different.  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)...

> > (why iconv ? IMHO conv it's a more portable name)
> 
> I think it's important to distiguish between datatype conversion
> (tds_convert) and character set conversion (tds_iconv).  "tds_conv" doesn't
> suggest either one clearly.  
> 

You are right. However iconv is too similar to... iconv :)

bye
  freddy77