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

Michal Židek mzidek at redhat.com
Tue Nov 3 13:48:46 UTC 2015


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 at 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 at 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 at 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).

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Continue-if-setlocale-fails.patch
Type: text/x-patch
Size: 1570 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151103/5bb05675/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-server_setup-Log-failed-attempt-to-set-locale.patch
Type: text/x-patch
Size: 1193 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151103/5bb05675/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-tests-Run-intgcheck-without-libsemanage.patch
Type: text/x-patch
Size: 850 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151103/5bb05675/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-tests-Regression-test-with-wrong-LC_ALL.patch
Type: text/x-patch
Size: 4661 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151103/5bb05675/attachment-0003.bin>


More information about the sssd-devel mailing list