FreeTDS Development Group

["Brian Bruns" <camber AT ais.org>]

> Brian,
> 
> I did a clean build from CVS head sources, fails to build due to
> missing tdsiconv.h, I assume thats a new one you built but didn't
> check into CVS yet. (Either that or I still haven't figured out
> cvs properly :)

Ah yes, my bad.

Re: your patch.  I was pretty limited on time when I put this in (always a
bad idea), so I had been going down one path, got your patch, and
basically jammed it in.  I'm going to revisit it this weekend.  Your
observations are correct though.

Brian

> I few comments in support of my iconv changes that were lost in
> the patch tho :)
> 
> 1. I think that keeping the implementation details opaque within
>    iconv.c and accessing via function calls, rather than by
>    exposing through headers, is a "good thing". Its a good way
>    to avoid future compatability problems. (No need for a
>    tdsiconv.c, for a start!)
>    For example if the code in mem.c doesn't know anything about
>    the contents of the void pointer to the TDSICONVINFO structure
>    then iconv.c can play with the details without anything else
>    needing to be changed.
>
> 2. The current code references unchecked malloc()d memory in
>    several places. I know this is done throughout the code,
>    but I had tidied up this area! [My background in small,
>    embedded, limited memory systems makes me shudder at the
>    sight of such code :) ]
> 
> 3. Current code also reintroduces the possibility of iconv_open()
>    without a matching iconv_close() which I had removed. (If the
>    first iconv_open succeeds and the second fails. use_iconv is 
>    only set if BOTH succeed, but tds_iconv_close is only called
>    if use_iconv is set. Another example where its better to call
>    tds_iconv_close and let it figure out the details :)
> 
> Cheers,
> 
> David
> 
> 
> > -----Original Message-----
> > From: Brian Bruns [mailto:camber AT ais.org]
> > Sent: Wednesday, 14 August 2002 8:17 PM
> > To: TDS Development Group
> > Subject: [freetds] Re: Crash in tsql
> > 
> > 
> > 
> > I checked in a fix (hopefully it's a fix ;-)  I had had one 
> > brewing when I 
> > got your patch, but the final was a blend of the two.  
> > Someone want to 
> > give it a try?
> > 
> > Cheers,
> > 
> > Brian
> > 
> > On Wed, 14 Aug 2002, Varley, David(CBorn at Alcoa) wrote:
> > 
> > > 
> > > Dave,
> > > 
> > > I agree with your proposed solution, in fact I'd suggest taking
> > > it a step further and replacing the entry in the TDSSOCKET structure
> > > with a void pointer for the use of the iconv functions. This enables
> > > us to hide all the iconv details within functions in iconv.c, and
> > > gets the conditional stuff right out of tds.h. The 
> > functions themselves
> > > then take the appropriate actions depending whether the 
> > system supports
> > > it. The only files needing modification would be tds.h, iconv.c and
> > > mem.c.
> > > 
> > > Brian, I'm happy to make those mods if you agree with this.
> > > 
> > > David
> > > 
> > > > -----Original Message-----
> > > > From: Coleman, Dave [mailto:DColeman AT US.TIAuto.com]
> > > > Sent: Tuesday, 13 August 2002 10:14 PM
> > > > To: TDS Development Group
> > > > Subject: [freetds] Re: Crash in tsql
> > > > 
> > > > 
> > > > It's a day late, but if you're still looking for suggestions 
> > > > to help with
> > > > the ICONV mis-match, I think this might work.  What does 
> > > > anyone else think?
> > > > 
> > > > Set up the ICONV stuff be a small structure pointer in 
> > TDSSOCKET.  
> > > > 
> > > > Something like:
> > > > 
> > > > struct tds_iconv_info
> > > > {
> > > >         int use_iconv;
> > > >         iconv_t cdto;
> > > >         iconv_t cdfrom;
> > > > }       TDSICONVINFO;
> > > > 
> > > > then TDSSOCKET would look something like this
> > > > 
> > > > ...
> > > >         TDSICONVINFO *tds_iconv;
> > > >         char *date_fmt;
> > > >         TDSCONTEXT *tds_ctx;
> > > > } TDSSOCKET;
> > > > 
> > > > modify tds_alloc_socket(...)
> > > > 
> > > > 371:            tds_socket->parent = (char*)NULL;
> > > >         +       tds_socket->tds_iconv = (char*)NULL;
> > > > 372:            tds_socket->env = tds_alloc_env(tds_socket);
> > > > 
> > > > add a few functions:
> > > > 
> > > > TDSICONVINFO* tds_alloc_iconv(TDSSOCKET *tds);
> > > > void tds_free_iconv(TDSSOCKET *tds);
> > > > and
> > > > TDSICONVINFO* tds_geticonv(TDSSOCKET *tds)
> > > >         {
> > > >         /*
> > > >         1) do some environment stuff to find if can use iconv on the
> > > > system...
> > > >                 might be sloppy, but keeps ICONV out of TDSSOCKET.
> > > >                 Someone with a better knowledge of 
> > > > configuration stuff than
> > > >                 myself may know a good way to get this information out
> > > >                 of the system at run time.
> > > >         */
> > > >         if( /* has iconv stuff */)
> > > >                 {
> > > >                 tds->tds_iconv = tds_alloc_iconv(tds);
> > > >                 }
> > > >         /*
> > > >         do what ever else you need to do to set things up
> > > >         */
> > > >         return tds->tds_iconv;
> > > >         }
> > > > 
> > > > add a call to tds_free_iconv(...) from tds_free_socket(...) 
> > > > and you should
> > > > be done unless I've missed something dumb.
> > > > 
> > > > It doesn't solve the root problem of needing to determine if 
> > > > a system has
> > > > ICONV support at run-time, but it does allow the TDSSOCKET 
> > > > structure (and
> > > > the library) to be independent of ICONV.  It could simplify 
> > > > the functions
> > > > dealing with it as well.  The prototpe would not change at 
> > > > all, they would
> > > > still get a TDSSOCKET.  Compile the library with the 
> > > > tds_iconv[xxx](...)
> > > > functions if the headers exist on the system, and the 
> > > > functions can test for
> > > > if ( tds_socket->tds_iconv ) and as long as 
> > > > tds_alloc_socket(...) sets the
> > > > tds_iconv pointer to NULL, any tds_iconvblah(...) function 
> > > > can fail safely
> > > > if no support is present, or it has not been initialized. 
> >  This could
> > > > prevent people from trying to use the features if they had 
> > > > not been compiled
> > > > into the library without including config.h.  The library 
> > > > would compile but
> > > > the application would not if it tried to link in the ICONV 
> > > > functions on a
> > > > system with an ICONV-less OS or freeTDS distribution.  It's 
> > > > still a link
> > > > error and not completely clean, but that's a heck of a lot 
> > > > better than a
> > > > seg-fault.
> > > > 
> > > > Thoughts, comments?
> > > > 
> > > > Dave
> > > > 
> > > > > -----Original Message-----
> > > > > From: Brian Bruns [SMTP:camber AT ais.org]
> > > > > Sent: Monday, August 12, 2002 10:37 AM
> > > > > To:   TDS Development Group
> > > > > Subject:      [freetds] Re: Crash in tsql
> > > > > 
> > > > > Actually, this fix is bad, since we don't distribute 
> > > > config.h (nor should
> > > > > we) things won't build.  If we don't do it in the header 
> > > > file however, we
> > > > > are in trouble of having mismatched structure definitions.  
> > > > We didn't
> > > > > really notice it before because iconv_t cdto, cdfrom was 
> > > > the last entry.
> > > > > 
> > > > > We simply can't be checking HAVE_ICONV in tds.h (or any 
> > > > distributable
> > > > > header).  We could substitute from configure but that 
> > strikes me as
> > > > > sloppy.  Any other options?
> > > > > 
> > > > > Brian
> > > > > 
> > > > > > Ok, I was on the right track, sizeof(*tds) differed 
> > > > between the two, I 
> > > > > > just didn't have time to finish it off.  config.h should 
> > > > be included
> > > > > from
> > > > > > tds.h (tds.h.in) otherwise we'll have this problem again 
> > > > in the future.
> > > > > > 
> > > > > > Checking the fix in now.
> > > > > > 
> > > > > > Brian
> > > > > > 
> > > > > > On Sun, 11 Aug 2002 freddyz77 AT tin.it wrote:
> > > > > > 
> > > > > > > > This has been the topic of discussion for the past 
> > > > couple of days.
> > > > > For
> > > > > > > > some reason tds->tds_ctx is NULL after the call to 
> > > > tds_connect(),
> > > > > although
> > > > > > > > it is set within tds_connect().  I haven't had a 
> > > > chance to track it
> > > > > any
> > > > > > > > further.
> > > > > > > > 
> > > > > > > > Brian
> > > > > > > > 
> > > > > > > 
> > > > > > > Ok, after a bit debug (assembly level at the end) I 
> > > > found the problem.
> > > > > > > You do not include config.h in tsql, so tds_socket 
> > > > appear to tsql
> > > > > without
> > > > > > > iconv stuff.
> > > > > > > 
> > > > > > > Add "#include <config.h>" on top of tsql.c and it 
> > run correctly.
> > > > > > > Perhaps is best to include config.h from tds.h.in ...
> > > > > > > 
> > > > > > > freddy77
> > > > > > >
> > > > > 
> > > > > ---
> > > > > You are currently subscribed to freetds as: 
> > [dColeman AT us.tiauto.com]
> > > > > To unsubscribe, forward this message to
> > > > > $subst('Email.Unsub')
> > > > 
> > > > ---
> > > > You are currently subscribed to freetds as: 
> > > > [David.Varley AT alcoa.com.au]
> > > > To unsubscribe, forward this message to 
> > > > $subst('Email.Unsub')
> > > > 
> > > 
> > > ---
> > > You are currently subscribed to freetds as: [camber AT ais.org]
> > > To unsubscribe, forward this message to 
> > $subst('Email.Unsub')
> > > 
> > > 
> > 
> > 
> > 
> > ---
> > You are currently subscribed to freetds as: 
> > [David.Varley AT alcoa.com.au]
> > To unsubscribe, forward this message to 
> > $subst('Email.Unsub')
> >