Hi,
attached is patch to fix ticket: https://fedorahosted.org/sssd/ticket/2785
And one additional patch to add DEBUG message.
Michal
On (19/10/15 16:03), Michal Židek wrote:
Hi,
attached is patch to fix ticket: https://fedorahosted.org/sssd/ticket/2785
And one additional patch to add DEBUG message.
Michal
From 3227027c3680b4503477135608969ca904e491c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 19 Oct 2015 15:38:08 +0200 Subject: [PATCH 1/2] 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..ea7a1cc 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 compatible* C locale. */
I'm not sure wehter "compatible" is the best explanation. I think that "continue with the default" would suit better.
BTW: man setlocale(3) 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, "");
Would you also mind to write integration test? I can test on command line with:
[root@host ~]# LC_ALL="adasd" sss_cache -E Error setting the locale [root@host ~]# echo $? 5
LS
On 10/19/2015 04:21 PM, Lukas Slebodnik wrote:
On (19/10/15 16:03), Michal Židek wrote:
Hi,
attached is patch to fix ticket: https://fedorahosted.org/sssd/ticket/2785
And one additional patch to add DEBUG message.
Michal
From 3227027c3680b4503477135608969ca904e491c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 19 Oct 2015 15:38:08 +0200 Subject: [PATCH 1/2] 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..ea7a1cc 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 compatible* C locale. */I'm not sure wehter "compatible" is the best explanation. I think that "continue with the default" would suit better.
BTW: man setlocale(3) 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, "");Would you also mind to write integration test? I can test on command line with:
[root@host ~]# LC_ALL="adasd" sss_cache -E Error setting the locale [root@host ~]# echo $? 5
LS
Thank you Lukas!
I added the integration test for this. I added new file for tests that require LOCAL domain only (no LDAP). I plan to add more tests here later.
As we already discussed offline. I temporarily worked around the issue with failing memcache test by not removing the memcache files.
Nick asked me to provide some explanation on the issue so I put it here, please correct me if I say something wrong.
The problem seems to be that the nss client code only checks validity of the memcache when the memcache context is created. Long living application only check the validity once and then use file descriptor to manipulate the cache until they are finished. Pytest is one such application. If we use the python pwd and grp wrappers, we initialize memcache context in Pytest. If we later remove the memcache files as part of teardown and create new memcache files for new tests, Pytest still uses the old file descriptors so calls to pwd and grp wrappers will work with the deleted memcache files.
See the attached patches.
Thanks!
Michal
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!
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
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
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
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@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@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.
2.1.0
From 2b15daa0871ac7f3abadbeb33f115146c5cd1859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@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); \--
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.
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.
From 7462496bf237dd067335ecf084b24bb1e353ea45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@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@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
+import pwd +import time +import config +import signal +import subprocess +import pytest +import sssd_id
another unused import
LS
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@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@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
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@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@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@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); \--
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@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@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
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@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@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 :-)
From 2b15daa0871ac7f3abadbeb33f115146c5cd1859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@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); \--
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.
LS
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@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@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@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); \--
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).
Michal
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@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@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@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); \--
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
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@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@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@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); \--
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
master: * 586f512ab8b6e5a03349598846141f43c1d505b8 * f1b9f9370b50a3d001722737f2538f5d3bb40e9c * a0c8aae6b31867f29e83e4f8a2a7ef037a82569e * 43e06ff39584570817949dc5de118d2b7ca854c1
LS
On (04/11/15 09:13), Lukas Slebodnik wrote:
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@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@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@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
master:
- 586f512ab8b6e5a03349598846141f43c1d505b8
- f1b9f9370b50a3d001722737f2538f5d3bb40e9c
- a0c8aae6b31867f29e83e4f8a2a7ef037a82569e
- 43e06ff39584570817949dc5de118d2b7ca854c1
sssd-1-13: * 03f6667741bf111f0e50c8f2c4323e45ce53f707 * 46a4ce2c853af464f24de63283fb8aa8a8460540 * 76ab3eb947f4d6fe6555d8ea0ae97dc3966f02ac * 4815471669a25566f6772c228c104a206ffa37f7
LS
On 10/21/2015 07:28 PM, Michal Židek wrote:
Nick asked me to provide some explanation on the issue so I put it here, please correct me if I say something wrong.
The problem seems to be that the nss client code only checks validity of the memcache when the memcache context is created. Long living application only check the validity once and then use file descriptor to manipulate the cache until they are finished. Pytest is one such application. If we use the python pwd and grp wrappers, we initialize memcache context in Pytest. If we later remove the memcache files as part of teardown and create new memcache files for new tests, Pytest still uses the old file descriptors so calls to pwd and grp wrappers will work with the deleted memcache files.
Thanks, Michal!
However, how come there is a difference when the test has/doesn't have the LDAP enumeration before clearing the cache?
See the last message in "intg: Add more LDAP tests" thread.
Nick
P.S. I tried not removing the cache file and it doesn't seem to help much - now it passes sometimes, but still fails most of the time. You can try the test in the message mentioned above. This is the addition I made for keeping the cache:
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index cdd8c8d..86d0854 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -30,6 +30,7 @@ import ldap import pytest import ds_openldap import ldap_ent +import re from util import *
LDAP_BASE_DN = "dc=example,dc=com" @@ -191,7 +192,8 @@ def cleanup_sssd_process(): pass subprocess.call(["sss_cache", "-E"]) for path in os.listdir(config.DB_PATH): - os.unlink(config.DB_PATH + "/" + path) + if re.match("^cache_.*\.ldb$", path) is None: + os.unlink(config.DB_PATH + "/" + path) for path in os.listdir(config.MCACHE_PATH): os.unlink(config.MCACHE_PATH + "/" + path)
On 10/21/2015 06:48 PM, Nikolai Kondrashov wrote:
On 10/21/2015 07:28 PM, Michal Židek wrote:
Nick asked me to provide some explanation on the issue so I put it here, please correct me if I say something wrong.
The problem seems to be that the nss client code only checks validity of the memcache when the memcache context is created. Long living application only check the validity once and then use file descriptor to manipulate the cache until they are finished. Pytest is one such application. If we use the python pwd and grp wrappers, we initialize memcache context in Pytest. If we later remove the memcache files as part of teardown and create new memcache files for new tests, Pytest still uses the old file descriptors so calls to pwd and grp wrappers will work with the deleted memcache files.
Thanks, Michal!
However, how come there is a difference when the test has/doesn't have the LDAP enumeration before clearing the cache?
See the last message in "intg: Add more LDAP tests" thread.
Nick
P.S. I tried not removing the cache file and it doesn't seem to help much - now it passes sometimes, but still fails most of the time. You can try the test in the message mentioned above. This is the addition I made for keeping the cache:
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index cdd8c8d..86d0854 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -30,6 +30,7 @@ import ldap import pytest import ds_openldap import ldap_ent +import re from util import *
LDAP_BASE_DN = "dc=example,dc=com" @@ -191,7 +192,8 @@ def cleanup_sssd_process(): pass subprocess.call(["sss_cache", "-E"]) for path in os.listdir(config.DB_PATH):
os.unlink(config.DB_PATH + "/" + path)
if re.match("^cache_.*\\.ldb$", path) is None:os.unlink(config.DB_PATH + "/" + path) for path in os.listdir(config.MCACHE_PATH): os.unlink(config.MCACHE_PATH + "/" + path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is the one :)
Hi Nick!
Not the sysdb cache. You need to keep the memcache files (in config.MCACHE_PATH). The sysdb cache can (and should be) deleted.
Michal
sssd-devel@lists.fedorahosted.org
-
Lukas Slebodnik -
Michal Židek -
Nikolai Kondrashov -
Pavel Reichl