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(a)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(a)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