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

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 4 08:13:32 UTC 2015


On (04/11/15 09:06), Lukas Slebodnik wrote:
>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
>
master:
* 586f512ab8b6e5a03349598846141f43c1d505b8
* f1b9f9370b50a3d001722737f2538f5d3bb40e9c
* a0c8aae6b31867f29e83e4f8a2a7ef037a82569e
* 43e06ff39584570817949dc5de118d2b7ca854c1

LS


More information about the sssd-devel mailing list