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(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.
>>>>
>>>>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(a)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
>
>http://sssd-ci.duckdns.org/logs/job/31/92/summary.html
>
master:
* 586f512ab8b6e5a03349598846141f43c1d505b8
* f1b9f9370b50a3d001722737f2538f5d3bb40e9c
* a0c8aae6b31867f29e83e4f8a2a7ef037a82569e
* 43e06ff39584570817949dc5de118d2b7ca854c1
sssd-1-13:
* 03f6667741bf111f0e50c8f2c4323e45ce53f707
* 46a4ce2c853af464f24de63283fb8aa8a8460540
* 76ab3eb947f4d6fe6555d8ea0ae97dc3966f02ac
* 4815471669a25566f6772c228c104a206ffa37f7
LS