[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Gracefully handle incompatible locale data
From: |
Carlos O'Donell |
Subject: |
Re: [PATCH] Gracefully handle incompatible locale data |
Date: |
Tue, 13 Oct 2015 09:30:50 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 09/29/2015 04:08 AM, Ludovic Courtès wrote:
> "Carlos O'Donell" <address@hidden> skribis:
>
>> On 09/26/2015 06:24 AM, Ludovic Courtès wrote:
>>> Furthermore, the function in question returns EINVAL in other similar
>>> cases–e.g., when libc 2.22 loads LC_COLLATE data from libc 2.21.
>>
>> If you change this particular case to EINVAL, what does the user see
>> as a result of this change?
>
> The user-visible change is that, if incompatible or broken locale data
> is found, a call like:
>
> setlocale (LC_ALL, "");
>
> returns EINVAL instead of aborting.
Perfect.
>> Do they get a non-zero exit code from `localedef --list-archive` along
>> with an error written out to stderr?
>
> ‘localedef’ starts with:
>
> setlocale (LC_MESSAGES, "");
> setlocale (LC_CTYPE, "");
>
> so it will no longer abort when invalid locale data is found (although
> in the 2.21 → 2.22 transition, only the LC_COLLATE data format differs
> anyway.)
OK.
> Apart from that, ‘localedef --list-archive’ simply opens the locale
> archive (typically /usr/lib/locale/locale-archive, regardless of the
> ‘LOCPATH’ environment variable value), so its behavior is unchanged.
>
> Am I overlooking something?
If the locale-archive is upgraded to the new format with LC_COLLATE changed
what happens when you run localedef --list-archive? Does it list zero locales
and exit with an exit code of zero?
I would hope that it prints something about the broken locale, because after
the removal of the assertion you won't know anything is wrong with the archive?
>> This is the kind of change I'm expecting. If we are removing an assertion,
>> we should be replacing it with something meaningful and verifying that
>> meaningful change.
>
> Yes, agreed.
>
> The function that is changed, ‘_nl_intern_locale_data’, has only two
> callers in libc, and both check whether it returns NULL. So it seems to
> me that the code is not introducing anything new in the API contract.
> WDYT?
It isn't introducing anything new, but you are removing an assert and we need
to make sure that the intent of the design remains: Warn the user something
is wrong with the locale data.
I see two cases:
- What does setlocale() return?
- Verified. You say it returns EINVAL for the affected locale and that's
perfect.
- What does localedef --list-archive return?
- The new LC_COLLATE format will make it's way into the binary locale archive
and that means glibc can't read the locale-archive? Does it fail? exit code?
If we cover those two cases with some kind of error message then I think we're
done.
Cheers,
Carlos.