On 09/18/2015 11:27 AM, Lukas Slebodnik wrote:
>On (18/09/15 11:18), Michal Židek wrote:
>>On 09/18/2015 10:19 AM, Lukas Slebodnik wrote:
>>>On (16/09/15 13:41), Michal Židek wrote:
>>>>On 09/14/2015 05:49 PM, Jakub Hrozek wrote:
>>>>>On Mon, Sep 07, 2015 at 03:38:32PM +0200, Michal Židek wrote:
>>>>>>Hi,
>>>>>>
>>>>>>patch for ticket
https://fedorahosted.org/sssd/ticket/2773
>>>>>>is attached.
>>>>>>
>>>>>>Michal
>>>>>
>>>>>> From 96215f618f61b8b2b303f0398a41af94292ccf57 Mon Sep 17
00:00:00 2001
>>>>>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
>>>>>>Date: Mon, 7 Sep 2015 15:19:53 +0200
>>>>>>Subject: [PATCH] PAM: Make p11_child timeout configurable
>>>>>>
>>>>>>Ticket:
>>>>>>https://fedorahosted.org/sssd/ticket/2773
>>>>>>---
>>>>>> Makefile.am | 2 +-
>>>>>> src/confdb/confdb.h | 1 +
>>>>>> src/config/SSSDConfig/__init__.py.in | 1 +
>>>>>> src/man/sssd.conf.5.xml | 12 ++++++++++++
>>>>>> src/responder/pam/pamsrv_cmd.c | 35
++++++++++++++++++++++++++++++-----
>>>>>> 5 files changed, 45 insertions(+), 6 deletions(-)
>>>>>
>>>>>Please also add the new parameter to src/config/etc/sssd.api.conf.
>>>>>
>>>>>One style-question inline (but really, more of a question..)
>>>>>
>>>>>> static errno_t
>>>>>>@@ -1126,8 +1125,21 @@ static int pam_forwarder(struct cli_ctx
*cctx, int pam_cmd)
>>>>>> }
>>>>>>
>>>>>> if (may_do_cert_auth(pctx, pd)) {
>>>>>>+ int p11_child_timeout;
>>>>>
>>>>>I wonder if the block starts getting so big that it makes sense to
add
>>>>>variables to its scope..isn't it better to just split the block
into tis
>>>>>own function?
>>>>>
>>>>>Especially if the block is unit-tested?
>>>>
>>>>Yes, the block grew a little too much with the added
>>>>option. See the new version.
>>>>
>>>>Michal
>>>
>>>>From d094c0f9814b23a8d669940e4c6a7cd8c7077164 Mon Sep 17 00:00:00 2001
>>>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
>>>>Date: Mon, 7 Sep 2015 15:19:53 +0200
>>>>Subject: [PATCH 1/2] PAM: Make p11_child timeout configurable
>>>>
>>>>Ticket:
>>>>https://fedorahosted.org/sssd/ticket/2773
>>>>---
>>>>Makefile.am | 2 +-
>>>>src/confdb/confdb.h | 1 +
>>>>src/config/SSSDConfig/__init__.py.in | 1 +
>>>>src/config/etc/sssd.api.conf | 1 +
>>>>src/man/sssd.conf.5.xml | 12 +++++++
>>>>src/responder/pam/pamsrv_cmd.c | 64
+++++++++++++++++++++++-------------
>>>>6 files changed, 57 insertions(+), 24 deletions(-)
>>>>
>>>>diff --git a/Makefile.am b/Makefile.am
>>>>index dc0670a..1298522 100644
>>>>--- a/Makefile.am
>>>>+++ b/Makefile.am
>>>>@@ -1893,7 +1893,7 @@ pam_srv_tests_SOURCES = \
>>>>pam_srv_tests_CFLAGS = \
>>>> -U SSSD_LIBEXEC_PATH
-DSSSD_LIBEXEC_PATH=\"$(abs_builddir)\" \
>>>> $(AM_CFLAGS) \
>>>>- -DSSS_P11_CHILD_TIMEOUT=30 \
>>>>+ -DSSS_P11_CHILD_TIMEOUT_DEFAULT=30 \
>>>Do we need this compile time option if it is possible to
>>>change it at runtime? We should be able to use the option p11_child_timeout
>>>in test as well and not just in sssd.conf
>>>
>>>LS
>>
>>It is not necessary.
>My intention was to simplify build system part.
>
>>OTOH I do not think it is bad way
>>to do it. This will force the higher default for all
>>future tests that use p11 child globally without the
>>need to modify each test. I would prefer to keep
>>it this way, but I can change it if you insist.
>>
>This can be achieved also in cleaner way
>which will be visible directly in test.
>We can use setup/teardown functions for configuring
>p11_child timeout to 30 seconds.
Ok. Will do.
>
>BTW setting SSS_P11_CHILD_TIMEOUT to 30 seconds was
>just a temporary solution for a broken test.
>It would be good to identify why 5 seconds was not enough.
>There might be either bug in code or in test and increasing
>the timeout just hid the problem.
>
>Do we need a ticket for this?
I am not sure. We can ask Sumit (CCing), but IIRC the test
was only failing when running under Valgrind. I would investigate
only if we find out it is slow also outside Valgrind, not
sooner.
I agree, the delay might be caused by the environment (valgrind +
virtual machine). Maybe we can use RUNNING_ON_VALGRIND
(
)
in the test and increase the timeout at runtime if it runs on valgrind?
bye,
Sumit