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