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

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 4 08:06:22 UTC 2015


On (03/11/15 14:48), Michal Židek wrote:
>On 11/03/2015 12:25 PM, Lukas Slebodnik wrote:
>>On (30/10/15 16:06), Michal Židek wrote:
>>>On 10/30/2015 09:37 AM, Lukas Slebodnik wrote:
>>>>>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.
>>>
>>>I changed the level here to less verbose. Do you want to change
>>>levels in the first patch as well?
>>>
>>Thank you.
>>
>>It depends on how do we want to solve ticket
>>https://fedorahosted.org/sssd/ticket/1372. If we will change it globally then
>>we can. But on the other hand it might be good to inform user that messages
>>will not be localized. So you must decide yourself :-)
>>
>
>I will keep it as it is then.
>
>>>>
>>>>>From 2b15daa0871ac7f3abadbeb33f115146c5cd1859 Mon Sep 17 00:00:00 2001
>>>>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>>>>>Date: Tue, 20 Oct 2015 18:18:01 +0200
>>>>>Subject: [PATCH 3/4] tests: Run intgcheck without libsemanage
>>>>>
>>>>>For now the libsemanage can not be used inside
>>>>>intgcheck tests.
>>>>>---
>>>>>Makefile.am | 1 +
>>>>>1 file changed, 1 insertion(+)
>>>>>
>>>>>diff --git a/Makefile.am b/Makefile.am
>>>>>index 15d99ce..77a325a 100644
>>>>>--- a/Makefile.am
>>>>>+++ b/Makefile.am
>>>>>@@ -2647,6 +2647,7 @@ intgcheck:
>>>>>	    --prefix="$$prefix" \
>>>>>	    --with-ldb-lib-dir="$$prefix"/lib/ldb \
>>>>>	    --enable-intgcheck-reqs \
>>>>>+   	    --without-semanage \
>>>>>	    $(INTGCHECK_CONFIGURE_FLAGS); \
>>>>>	$(MAKE) $(AM_MAKEFLAGS); \
>>>>>	: Force single-thread install to workaround concurrency issues; \
>>>>>--
>>>>
>>>>A) there is a git warning
>>>>/dev/shm/sssd/.git/rebase-apply/patch:13: space before tab in indent.
>>>>             --without-semanage \
>>>>                 warning: 1 line adds whitespace errors.
>>>
>>>Fixed.
>>>
>>>>
>>>>B)
>>>>Why does it need to be in separate patch?
>>>>and the statement in commit message is not true:
>>>>"For now the libsemanage can not be used inside intgcheck tests"
>>>>We are able to run intgcheck after applying first two patches.
>>>
>>>The statement in the commit message is true, we can not
>>>use libsemanage in the CI. The libsemanage functions
>>>always fail in CI. The reason why we can run intgcheck
>>>is because so far we have no tests, that would trigger
>>>code path with libsemanage functions. But I am adding
>>>such test in the next patch. If you prefer, I can
>>>squash the two patches, but I do believe it should be
>>>in separate patch, because it is change on its own
>>>that may be reverted in the future and it is not
>>>related to the purpose of the next patch.
>>>
>>The main problem is that after few weeks you might not remeber
>>why this line was added. The commit message does not have enought details
>>about problems with libsemanage. Which test is affected by this isssue,
>>We do not know when this "workaround" might be reverted ...
>>
>>Usually, one-liner should be explain much more then big patch.
>>because it's not obvius from a patch why we need it.
>>
>>BTW At least tracking ticket for this issue should be mentioned in commit
>>message.
>
>Ok. I created a tracking ticket for it and added it to the
>commit message. Not sure if we should deffer it or move to
>some more distant milestone. We will probably not spend time
>with it soon.
>
>>
>>LS
>
>New patches attached (only the commit message is changed).
>
Thank you.

ACK

http://sssd-ci.duckdns.org/logs/job/31/92/summary.html

LS


More information about the sssd-devel mailing list