Re: [PATCH] tds_get_locale() update (was Re: SQL Server version and TDS Version)

On Sun, May 23, 2010 at 06:13:44PM -0500, Craig A. Berry wrote:
> 
> On Apr 30, 2010, at 3:30 PM, [email protected] wrote:
> 
> >On Fri, Apr 30, 2010 at 02:58:12PM -0400, [email protected]  
> >wrote:
> >>
> >>1.  API call e.g. DBSETLCHARSET().
> >>2.  freetds.conf.
> >>3.  nl_langinfo(3) /* query using CODESET parameter */
> >>4.  setlocale(3) /* query using NULL parameter, split on "." */
> >>5.  ISO 8859-1

> Finally got back to this.  The attached patch does two things to src/ 
> tds/locale.c:tds_get_locale():

[excellent summary omitted]

Hi Craig, 

You're looking for a little guidance, which I'll try to provide.  I haven't verified your analysis because 1) it sounds right and 2) if it's not I'm confident you'll get it right in the end.  (And 3: that would take too long.) 

> But late in the day I realized that while this tiny refactor is  
> probably a step in the right direction, it doesn't really address how  
> the client charset is selected because the value determined from the  
> environment in tds_alloc_locale() will always be overridden with a  
> hard-wired ISO-8859-1 in tds_alloc_connection(). This is because TDS  
> applications call tds_alloc_context() before they call  
> tds_read_config_info().  The following calling sequences show how we  
> end up getting what's done in tds_alloc_connection():
> 
> a.)  tds_alloc_locale*
>        tds_get_locale
>          tds_alloc_context
> 
> b.)  tds_alloc_connection*
>        tds_read_config_info
> 
> and what happens in either of these can be overridden from  
> freetds.conf because the following happens later:
> 
> c.)  tds_try_conf_file
>        tds_read_conf_file
>          tds_read_config_info
> 
> 
> The problem in step b above is that tds_alloc_connection() has the  
> following code in it:
> 
>         /* TODO use system default ?? */
>         if (!tds_dstr_copy(&connection->client_charset, "ISO-8859-1"))
>                 goto Cleanup;
> 
> There is no checking to see if the client charset is already set in  
> the locale structure, so in effect, locale->client_charset is an  
> appendage that is never used for anything.  It's already been orphaned  
> so we might as well deprecate it.

That sounds right.  

> So the next step is to move the nl_langinfo(CODESET) call out of  
> tds_alloc_locale() and into tds_alloc_connection(), replacing the hard- 
> wired ISO-8859-1 with what we get by interrogating the locale.

Yes, leaving ISO 8859-1 as a fallback if nl_langinfo/setlocale isn't available. 

> But I think that needs more discussion because anyone with a  
> misconfigured locale will suddenly not be able to construct a valid  
> login packet; basically we'll be doing at the library level what tsql  
> has been doing for a long time at the application level, and which  
> generated the problem report that led us down this road in the first  
> place.

You lost me there.  tsql isn't most applications; most applications use one of the client libraries, which *should* call these things in the same order that tsql does.  If tsql is effectively broken, we can fix it.  

I must be missing something, because "anyone with a misconfigured locale will suddenly not be able to construct a valid login packet" shouldn't be so, for reasons I'm sure you know full well: anyone with a misconfigured locale can resort to the freetds.conf override (level #2 above).  IOW, once step B is fixed, step C still happens.  So, what's the problem again?  :-)

It's great you're working on this.  Would you like to be able to commit these things yourself?  No good deed goes unpunished, you know.  

Regards, 

--jkl