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?= <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. > 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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
