[illumos-Developer] Webrev:: 197 svc.startd may not pick up locale correctly

[Garrett D'Amore]

  On 03/30/11 02:35 PM, Albert Lee wrote:
> On Mon, Mar 28, 2011 at 1:27 PM, shampavman<shampavman.cg at gmail.com>  wrote:
>> Hi All,
>>
>>
>>
>> I needed a review on this please::
>>
>> http://cr.illumos.org/view/98j7c9nh/
>>
>>
>>
>> I have a few concerns on this though. I honestly feel that a fix is not
>> required
>>
>> Reasons:: we already have a check to see if st_locale is NULL.
>>
>> since we have a variable for checking whether locale is known, we would not
>> accidently use st_locale if it is NULL
>>
>> and hence we will not core dump or exit
>>
> The description of the original issue is unclear, and I'm confused as
> to how this addresses the problem. From what I understand:
> * setlocale(LC_ALL, "") will check the environment variables for all
> categories including LC_MESSAGES and changes the values for all of the
> categories in the current locale.
> * setlocale(LC_MESSAGES, "") also checks the environment but only
> changes the LC_MESSAGES category in the current locale.
> * setlocale(LC_MESSAGES, NULL) returns the existing value for
> LC_MESSAGES category without checking the environment.
>
> So this change would have no effect except omitting a second,
> redundant environment check.

Almost, but not quite.  setlocale(LC_MESSAGES, "") will actually attempt 
to *set* the locale from standard environment messages, whereas the 
newer setlocale(LC_MESSAGES, NULL) doesn't examine environment variables 
but simply returns the current locale value (which should normally match 
the environment if setlocale(LC_ALL, "") is called first.)

I just talked with Vineeth (the submitter) about this bug, and I think 
it was a sign of confusion stemming from a mistaken belief that 
setlocale() might incorrectly return a locale even if setting it would 
fail (e.g. if the locale was not installed).

I can assure folks that the current illumos implementation most 
definitely does *not* do this.  On a failure, it will return NULL rather 
than a value that does not match the actual system locale.

I think sham's change is good (it avoids exercising a slightly longer 
code path by skipping the attempt to set the locale *again* for just 
LC_MESSAGES), so I'd bless it, even though he's also right in that we 
don't strictly need this change for correctness.

     - Garrett




> -Albert
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer