FreeTDS Development Group

["Bill Thompson" <thompbil AT exchange.uk.ml.com>]

Hi Freddy,

I'm sorry you're disappointed with my patch.
However, I think it's a little harsh to say that we are "back to the
drawing board". Let me go through your points, and try and answer them...

> I see that many work are shifted from tds to dblib/ctlib library. But many
> conversion can be done in tds_convert (such as same type or other).
> 

The work that has shifted from tds_convert to dblib/ctlib is:

a) like-to-like conversions
b) post conversion assignment of data back to the calling program

I moved like-to-like conversions out of tds_convert, because I saw it as
unnecessary to go into the conversion code unless strictly necessary. At
the end of the day, It's not integral to the architecture of my patch to
do this - tds_convert() will still handle like-to-like conversions.
 
> I think that we must pass to tds_convert pointer to final char buffer and
> destination length (as before). tds_convert should fill destination buffer
> until filling them. Library should then zero-terminate or padding (as
> necessary) or return error. This solve the double copy, unnecessary memory
> allocation and library problems. Some can disappoint that dblib/ctlib can be
> called with destlen -1 or -2 (there is sufficient space). This is not a
> problem, just fill destlen with maximun allowed (2GB in x86 architecture).

We had long discussions about this before. My view was firmly that ALL
responsibility for touching the calling programs variables should be
devolved to the API layer. This was because each API has different
standards about how this should be done, especially in the case of
CHAR/VARCHAR/TEXT and BINARY data. Your suggestion is a halfway house,
whereby tds_convert() writes to the calling program's variables and the
API clears up the mess afterwards. I think that's wrong architecturally,
messy and harder to debug when something goes wrong. As an example of what
could go wrong, you can't possibly have the case where "tds_convert should
fill destination buffer until filling them" If you pass in a desten of
2Gb!


> My intention for 0.6 release was a convert.c without any malloc in it (as
> you can note applying my patches). I disappoint with this patch. It
> reintroduce a lot of untested malloc (with relative core dumps...). It also
> lower performance by copy 2 time buffer for CHAR/VARCHAR...
> 

I'm not sure about performance issues. You may be right, but I would have
thought the overhead was minimal. Untested ? all I can say is that I've
tried my best to test all the possibilities. tds_convert() mallocs storage
where the target type is CHAR/VARCHAR/TEXT/BINARY/IMAGE . As long as the
API always frees the pointers when this is the case, I think it's pretty
watertight. Hang on - I've found one case where this doesn't happen - I'll
fix that today :-)

> 
> Someone also should write tests for all library conversion.
> 

They should, but it's a big job! we need a unittest for dblib, ctlib and
odbc! and the combinations are MANY.

Thanks for your feedback, and Regards,

Bill