[SSSD] [PATCHES] util: Continue if setlocale fails

Lukas Slebodnik lslebodn at redhat.com
Fri Oct 30 11:24:42 UTC 2015


On (30/10/15 09:37), Lukas Slebodnik wrote:
>On (23/10/15 14:23), Michal Židek wrote:
>>On 10/21/2015 07:25 PM, Michal Židek wrote:
>>>On 10/21/2015 07:19 PM, Michal Židek wrote:
>>>>On 10/21/2015 06:40 PM, Pavel Reichl wrote:
>>>>>
>>>>>
>>>>>On 10/21/2015 06:28 PM, Michal Židek wrote:
>>>>>>+
>>>>>>+def test_wrong_LC_ALL(local_domain_only):
>>>>>>+    """
>>>>>>+    Regression test for ticket
>>>>>>+https://fedorahosted.org/sssd/ticket/2785
>>>>>>+
>>>>>>+    """
>>>>>>+    subprocess.call(["sss_useradd", "foo", "-M"])
>>>>>>+    pwd.getpwnam("foo")
>>>>>>+
>>>>>>+    # Change the LC_ALL variable to nonexistent locale
>>>>>>+    oldvalue = os.environ.get("LC_ALL", "")
>>>>>>+    os.environ["LC_ALL"] = "nonexistent_locale"
>>>>>>+
>>>>>>+    # sss_userdel must remove the user despite wrong LC_ALL
>>>>>>+    ret = subprocess.call(["sss_userdel", "foo", "-R"])
>>>>>
>>>>>Please check ret value or use check_call method. Thanks!
>>>>>
>>>>>>+    assert_nonexistent_user("foo")
>>>>>>+    os.environ["LC_LOCAL"] = oldvalue
>>>>>>-- 2.1.0
>>>>>
>>>>>Please run pep8, I think I saw missing line and missing space after #.
>>>>>
>>>>>I haven't tested patches I just noticed this nitpicks.
>>>>>
>>>>>Thanks!
>>>>
>>>>Thank you Pavel, I fixed the nitpicks and
>>>>checked the code with pep8.
>>>>
>>>>New patches are attached.
>>>>
>>>>Michal
>>>>
>>>
>>>New patches attached. I had unused constant
>>>in the code,
>>>
>>>Michal
>>>
>>
>>I added one change in the Makefile.am .
>>
>>New patches attached.
>>
>>Michal
>>
>
>>From a84b7cfc766e1125399a100f28f7565a532c3863 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>>Date: Mon, 19 Oct 2015 15:38:08 +0200
>>Subject: [PATCH 1/4] util: Continue if setlocale fails
>>
>>Fixes:
>>https://fedorahosted.org/sssd/ticket/2785
>>
>>setlocale needs some environment variables
>>to be set in order to work. These variables
>>are not present in some special cases. We
>>should not fail completely in these cases
>>but continue with the compatible C locale.
>>---
>> src/sss_client/ssh/sss_ssh_client.c | 4 +++-
>> src/tools/tools_util.c              | 4 +++-
>> 2 files changed, 6 insertions(+), 2 deletions(-)
>>
>>diff --git a/src/sss_client/ssh/sss_ssh_client.c b/src/sss_client/ssh/sss_ssh_client.c
>>index 0d206ef..a198039 100644
>>--- a/src/sss_client/ssh/sss_ssh_client.c
>>+++ b/src/sss_client/ssh/sss_ssh_client.c
>>@@ -50,7 +50,9 @@ int set_locale(void)
>> 
>>     c = setlocale(LC_ALL, "");
>>     if (c == NULL) {
>>-        return EIO;
>>+        /* If setlocale fails, continue with the default
>>+         * locale. */
>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to set locale\n");
>>     }
>> 
>>     errno = 0;
>>diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c
>>index 68f6588..f9dca72 100644
>>--- a/src/tools/tools_util.c
>>+++ b/src/tools/tools_util.c
>>@@ -259,7 +259,9 @@ int set_locale(void)
>> 
>>     c = setlocale(LC_ALL, "");
>>     if (c == NULL) {
>>-        return EIO;
>>+        /* If setlocale fails, continue with the default
>>+         * locale. */
>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to set locale\n");
>>     }
>> 
>>     errno = 0;
>>-- 
>>2.1.0
>>
>LGTM.
>
>>From ea19184b2c1a6fe130b2346ec8504d181e1312e2 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>>Date: Mon, 19 Oct 2015 15:49:02 +0200
>>Subject: [PATCH 2/4] server_setup: Log failed attempt to set locale
>>
>>Failed setlocale call could cause unexpected
>>behaviour. It is better to generate DEBUG
>>message if this happens.
>>---
>> src/util/server.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>>diff --git a/src/util/server.c b/src/util/server.c
>>index 036dace..03796be 100644
>>--- a/src/util/server.c
>>+++ b/src/util/server.c
>>@@ -458,6 +458,7 @@ int server_setup(const char *name, int flags,
>>     bool dm;
>>     struct tevent_signal *tes;
>>     struct logrotate_ctx *lctx;
>>+    char *locale;
>> 
>>     ret = chown_debug_file(NULL, uid, gid);
>>     if (ret != EOK) {
>>@@ -508,7 +509,12 @@ int server_setup(const char *name, int flags,
>>     }
>> 
>>     /* Set up locale */
>>-    setlocale(LC_ALL, "");
>>+    locale = setlocale(LC_ALL, "");
>>+    if (locale == NULL) {
>>+        /* Just print debug message and continue */
>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to set locale\n");
>>+    }
>>+
>>     bindtextdomain(PACKAGE, LOCALEDIR);
>>     textdomain(PACKAGE);
>Do we rely need to add debug messge to this file?
>man setlocale says:
>
>       On  startup of the main program, the portable "C" locale is selected as
>       default.  A program may be made portable to all locales by calling:
>
>           setlocale(LC_ALL, "");
>
>It does not say anything about checking return value after startup of the main
>program. If you really want to print debug message then change debug level.
>If we ignore such error then it is not aminor failure.
>
BTW I checked source code of other projects and they ignore return value
of initial setlocale
https://github.com/karelzak/util-linux/blob/master/sys-utils/fstrim.c

You might see more examples in the same project.

LS


More information about the sssd-devel mailing list