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

Michal Židek mzidek at redhat.com
Fri Oct 30 15:06:07 UTC 2015


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?

>
>> 2.1.0
>>
>
>>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.

>
>
>>From 7462496bf237dd067335ecf084b24bb1e353ea45 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>> Date: Tue, 20 Oct 2015 15:03:22 +0200
>> Subject: [PATCH 4/4] tests: Regression test with wrong LC_ALL
>>
>> Ticket:
>> https://fedorahosted.org/sssd/ticket/2785
>>
>> Test local domain tool with wrong LC_ALL
>> environment variable value.
>>
>> NOTE: The memory cache files are not deleted
>> properly in the test teardown to work around the
>> problem described in ticket
>> https://fedorahosted.org/sssd/ticket/2726
>>
>> Once the ticket above is solved, the teardown
>> will be updated to remove the memory cache
>> files.
>> ---
>> src/tests/intg/Makefile.am          |   1 +
>> src/tests/intg/test_local_domain.py | 115 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 116 insertions(+)
>> create mode 100644 src/tests/intg/test_local_domain.py
>>
>> diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
>> index 6819c2f..12a4fc2 100644
>> --- a/src/tests/intg/Makefile.am
>> +++ b/src/tests/intg/Makefile.am
>> @@ -7,6 +7,7 @@ dist_noinst_DATA = \
>>      ent_test.py \
>>      ldap_ent.py \
>>      ldap_test.py \
>> +    test_local_domain.py \
>>      util.py \
>>      test_memory_cache.py \
>>      $(NULL)
>> diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py
>> new file mode 100644
>> index 0000000..d778412
>> --- /dev/null
>> +++ b/src/tests/intg/test_local_domain.py
>> @@ -0,0 +1,115 @@
>> +#
>> +# SSSD LOCAL domain tests
>> +#
>> +# Copyright (c) 2015 Red Hat, Inc.
>> +# Author: Michal Židek <mzidek at redhat.com>
>> +#
>> +# This is free software; you can redistribute it and/or modify it
>> +# under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 only
>> +#
>> +# This program is distributed in the hope that it will be useful, but
>> +# WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +# General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +import os
>> +import stat
>> +import ent
>> +import grp
> two unused imports
>

Fixed.

>> +import pwd
>> +import time
>> +import config
>> +import signal
>> +import subprocess
>> +import pytest
>> +import sssd_id
> another unused import
>

Fixed

> LS

New patches attached.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Continue-if-setlocale-fails.patch
Type: text/x-patch
Size: 1570 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151030/48ea90e3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-server_setup-Log-failed-attempt-to-set-locale.patch
Type: text/x-patch
Size: 1193 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151030/48ea90e3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-tests-Run-intgcheck-without-libsemanage.patch
Type: text/x-patch
Size: 768 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151030/48ea90e3/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-tests-Regression-test-with-wrong-LC_ALL.patch
Type: text/x-patch
Size: 4661 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151030/48ea90e3/attachment-0003.bin>


More information about the sssd-devel mailing list