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?= <mzi...@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?= <mzi...@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?= <mzi...@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?= <mzi...@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 <mzi...@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
>From de207bb827fa989bd7aa5c142534da2233315550 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@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
>From dc4f16bc066d76aad254e0ad763a45e2e57668cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@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..67a2595 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_TRACE_FUNC, "Unable to set locale\n");
+ }
+
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
--
2.1.0
>From f43236e93116eeed7d716a96b046f93bba73d2c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@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..098beb0 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; \
--
2.1.0
>From b62883a0c18090d46bd16807cfe3e99fa9aace17 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@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 | 112 ++++++++++++++++++++++++++++++++++++
2 files changed, 113 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..c62de16
--- /dev/null
+++ b/src/tests/intg/test_local_domain.py
@@ -0,0 +1,112 @@
+#
+# SSSD LOCAL domain tests
+#
+# Copyright (c) 2015 Red Hat, Inc.
+# Author: Michal Zidek <mzi...@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 pwd
+import time
+import config
+import signal
+import subprocess
+import pytest
+from util import unindent
+
+
+def stop_sssd():
+ pid_file = open(config.PIDFILE_PATH, "r")
+ pid = int(pid_file.read())
+ os.kill(pid, signal.SIGTERM)
+ while True:
+ try:
+ os.kill(pid, signal.SIGCONT)
+ except:
+ break
+ time.sleep(1)
+
+
+def create_conf_fixture(request, contents):
+ """Generate sssd.conf and add teardown for removing it"""
+ conf = open(config.CONF_PATH, "w")
+ conf.write(contents)
+ conf.close()
+ os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
+ request.addfinalizer(lambda: os.unlink(config.CONF_PATH))
+
+
+def create_sssd_fixture(request):
+ """Start sssd and add teardown for stopping it and removing state"""
+ if subprocess.call(["sssd", "-D", "-f"]) != 0:
+ raise Exception("sssd start failed")
+
+ def teardown():
+ try:
+ stop_sssd()
+ except:
+ pass
+ subprocess.call(["sss_cache", "-E"])
+ for path in os.listdir(config.DB_PATH):
+ os.unlink(config.DB_PATH + "/" + path)
+ # FIXME: Uncomment this when ticket #2726 is solved
+ # https://fedorahosted.org/sssd/ticket/2726
+ # for path in os.listdir(config.MCACHE_PATH):
+ # os.unlink(config.MCACHE_PATH + "/" + path)
+ request.addfinalizer(teardown)
+
+
+@pytest.fixture
+def local_domain_only(request):
+ conf = unindent("""\
+ [sssd]
+ domains = LOCAL
+ services = nss
+
+ [nss]
+ memcache_timeout = 0
+
+ [domain/LOCAL]
+ id_provider = local
+ min_id = 10000
+ max_id = 20000
+ """).format(**locals())
+ create_conf_fixture(request, conf)
+ create_sssd_fixture(request)
+ return None
+
+
+def assert_nonexistent_user(name):
+ with pytest.raises(KeyError):
+ pwd.getpwnam(name)
+
+
+def test_wrong_LC_ALL(local_domain_only):
+ """
+ Regression test for ticket
+ https://fedorahosted.org/sssd/ticket/2785
+
+ """
+ subprocess.check_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
+ subprocess.check_call(["sss_userdel", "foo", "-R"])
+ assert_nonexistent_user("foo")
+ os.environ["LC_LOCAL"] = oldvalue
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
