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