Skip to Content.
Sympa Menu

freetds - RE: [freetds] errno and threads

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: Frediano Ziglio <freddyz77 AT tin.it>
  • To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
  • Subject: RE: [freetds] errno and threads
  • Date: Wed, 06 Aug 2003 05:59:53 -0000

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






Archive powered by MHonArc 2.6.24.

Top of Page