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@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@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@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); \--
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
master: * 586f512ab8b6e5a03349598846141f43c1d505b8 * f1b9f9370b50a3d001722737f2538f5d3bb40e9c * a0c8aae6b31867f29e83e4f8a2a7ef037a82569e * 43e06ff39584570817949dc5de118d2b7ca854c1
LS