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

[Lukas Slebodnik]

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?= 
> 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?= 
> 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?= 
> 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.

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?= 
> 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 
> +#
> +# 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