FreeTDS Development Group

["Varley, David(CBorn at Alcoa)" <David.Varley AT alcoa.com.au>]

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 :)

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')
>