On Fri, Sep 18, 2015 at 11:43:26AM +0200, Michal Židek wrote:
> 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
(
http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clie...)
in the test and increase the timeout at runtime if it runs on valgrind?
I am not sure about RUNNING_ON_VALGRIND. We got used to the
fact that things are slow in the CI (sometimes very slow) so
we may hit the timeout in CI even without Valgrind. Valgrind
just makes it more likely. We currently do not have
infrastructure to test performance so I would be less strict
in the tests.
New patch set is attached. The first patch may fail in the CI
due to the timeout. The second one fixes it.
Michal