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

[Albert Lee]

On Wed, Mar 30, 2011 at 6:31 PM, Garrett D'Amore <garrett at nexenta.com> wrote:
>  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.
>

I agree with this change, just wasn't certain about the intent. Thanks.

-Albert